From ac533d3f619cfcce39a2edc8f2a935fa3f24e16e Mon Sep 17 00:00:00 2001 From: Cursor Agent Date: Mon, 1 Jun 2026 15:46:21 +0000 Subject: [PATCH] PR-D1 (ADR 0008 Phase D): remove ADR 0007 server-side dead code MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit ADR 0008 \u00a76.4 PR-D1 originally proposed a coupled change \u2014 (a) remove ADR-0007-vintage server dead code, (b) refactor the HTTP shim's chat-completions handler onto SessionStore. Implementation revealed (a) is a pure subtraction with no behavior dependence on (b), so they're split (same pattern as PR-A3 / PR-A3b). This PR is (a); (b) becomes PR-D2 (queued, not in this diff). Net diff: -540 deletions, +78 insertions. ADR 0008 \u00a76.4 amended to record the split. Files modified \u2014 production: inference_engine/server/engine.py -12 lines: EngineResult.path_selection / .tokens_skipped / .prefill_duration_seconds fields removed; SpeculativeEngine no longer forwards them from SpeculativeRunResult. inference_engine/server/metrics.py -74 lines: path_selection_total, continuation_tokens_skipped_total, verifier_prefill_duration_seconds, cache_invariant_violations_total metrics removed from Metrics + factory; record_path_selection and record_cache_invariant_violation methods removed. inference_engine/server/app.py -53 lines: _session_acceptance_rate and _emit_path_selection_metric helpers removed; the two call sites in the streaming + non- streaming completion paths now pass acceptance_rate=None to record_completion. The OpenAI response loses its acceptance_rate field as a result \u2014 acceptable on a feature-frozen deprecated shim per ADR 0008 \u00a72.7. Migrate to gRPC for richer telemetry. inference_engine/scheduler/session.py -7 lines: engine_result field on Session removed. The scheduler worker (scheduler.py) no longer stashes engine.generate()'s result on the session \u2014 the only reader was app.py's removed helpers. inference_engine/scheduler/scheduler.py \u00b13 lines (renamed assignment to del): the line that wrote session.engine_result = result is gone. scripts/bench_agentic/bench_long_session.py -211/+30 lines: removed _PATH_SELECTION_METRIC / _CONTINUATION_TOKENS_SKIPPED_METRIC / _CACHE_INVARIANT_VIOLATIONS_METRIC constants, the labeled-line regex, the labeled-metric branch in _parse_prom_text, the _extract_label helper (no callers after PR-D1), the _adr_0007_summary aggregator, the adr_0007 payload field, and the §2.10 block in render_summary. Module docstring updated to point at PR-E1's bench_session_long_run.py for the replacement bench. Files modified \u2014 tests: tests/inference_engine/server/test_metrics.py -107 lines: 4 entries dropped from test_build_registers_all_documented_metrics expected set; 9 tests removed from the 'ADR 0007 \u00a72.10 \u2014 path_selection observability' section. tests/inference_engine/server/test_app_metrics_and_auth.py -98 lines: 4 ADR-0007-specific tests removed (test_metrics_path_selection_metrics_present_on_idle_metrics_scrape, test_metrics_path_selection_recorded_after_completion, test_session_acceptance_rate_returns_none_when_result_missing_rate, test_emit_path_selection_metric_noop_when_path_unset). Local verification (Linux VM, py3.12): PYTHONPATH=.:sdks/python pytest 682 passed (was 695 - 13 ADR-0007-specific tests), TOTAL 1660 stmts 100.00 % coverage (was 1694 - 34 dead stmts). Per ADR 0008 \u00a79: this PR is pure deletion / cleanup on the Linux- runnable surface. Zero MLX runtime code touched. \u00a79 carve-out applies; no Mac M4 report needed. Next PR after merge: PR-D2 (\u00a76.4 amended, queued): HTTP-shim refactor onto SessionStore proper. Each /v1/chat/completions request becomes a single- shot session; PooledVerifier retires; Deprecation / Sunset headers added per \u00a72.7. Co-authored-by: FluffyAIcode --- ...session-bound-runtime-and-grpc-protocol.md | 44 +++- inference_engine/scheduler/scheduler.py | 12 +- inference_engine/scheduler/session.py | 7 - inference_engine/server/app.py | 53 +---- inference_engine/server/engine.py | 12 - inference_engine/server/metrics.py | 74 ------ scripts/bench_agentic/bench_long_session.py | 211 +++--------------- .../server/test_app_metrics_and_auth.py | 98 -------- tests/inference_engine/server/test_metrics.py | 107 --------- 9 files changed, 78 insertions(+), 540 deletions(-) diff --git a/docs/adr/0008-session-bound-runtime-and-grpc-protocol.md b/docs/adr/0008-session-bound-runtime-and-grpc-protocol.md index aeeb7e00..60ee5d94 100644 --- a/docs/adr/0008-session-bound-runtime-and-grpc-protocol.md +++ b/docs/adr/0008-session-bound-runtime-and-grpc-protocol.md @@ -775,12 +775,44 @@ parallelize. ### 6.4 Phase D — Deprecated HTTP+SSE shim -- **PR-D1**: Update `inference_engine/server/app.py` so each - `/v1/chat/completions` request creates a single-shot session under - the new `SessionStore`, prefills, generates, and closes. Removes - any path-selection / cross-request logic (none of which exists on - `main` after C3). Adds `Deprecation` / `Sunset` headers. Updates - the existing 461-test integration suite to match. +*(scope split, recorded 2026-06-01 during implementation of PR-D1.)* + +The original PR-D1 entry conflated two coupled changes: + + (a) Remove the ADR 0007 dead code from the server-side surface + (path_selection metrics, `_emit_path_selection_metric` helper, + `engine_result` field on the scheduler session, etc.). + (b) Refactor the HTTP shim's chat-completions handler onto the new + `SessionStore` so each request becomes a single-shot session + (prefill → generate → close) instead of being driven by the + legacy `PooledVerifier`. + +(a) is a pure subtraction: the dead code was reachable only from the +ADR 0007 path_select stack that PR-A3 already removed from the +verifier side; the server-side metrics and helpers it left behind +are unreachable at runtime in any healthy completion. (b) is a +larger refactor of feature-frozen code (per §2.7), with a +corresponding test-update tail. + +The two are split, same pattern as PR-A3 / PR-A3b: + +- **PR-D1** (this PR, dead-code removal): cleans up §6.6 rows for + `app.py` / `engine.py` / `metrics.py` / `scheduler/session.py` / + `bench_long_session.py`. The HTTP shim continues to use + `PooledVerifier` exactly as before; nothing user-observable + changes except the disappearance of the four ADR 0007 metrics + from `/metrics` and the `acceptance_rate` field from the OpenAI + response (the latter was sourced from `engine_result`, which is + gone). 100% Linux unit coverage. + +- **PR-D2** (queued, not in PR-D1's diff): the HTTP-shim refactor + proper. Each `/v1/chat/completions` request creates a single-shot + session under `SessionStore`, prefills, generates, and closes; + `PooledVerifier` is retired. Adds `Deprecation` / `Sunset` + headers per §2.7. Updates the existing integration suite to + match. Linux-only path; §9 carve-out continues to apply. PR-D2 + is non-blocking for v0.3 GA — the deprecated shim works on + `main` post-PR-D1 in its v0.3.0-rc1 shape, just lighter. ### 6.5 Phase E — Mac M4 integration test marker + CI workflow diff --git a/inference_engine/scheduler/scheduler.py b/inference_engine/scheduler/scheduler.py index dcd48d0d..2f076290 100644 --- a/inference_engine/scheduler/scheduler.py +++ b/inference_engine/scheduler/scheduler.py @@ -358,12 +358,12 @@ def on_token(tok_id: int) -> bool: session.eos_token_ids, on_token, ) - # Out of engine lock — finalize state. - # Stash the engine result on the session so route handlers - # can read path-selection observability fields (ADR 0007 - # §2.10) and acceptance rate. tokens were already streamed - # via on_token. - session.engine_result = result + # Out of engine lock — finalize state. Tokens were already + # streamed via on_token; the engine result is otherwise + # discarded (PR-D1 of ADR 0008 removed the engine_result + # stash that ADR 0007 §2.10 used for path-selection + # observability). + del result if session.state == SessionState.CANCELLED: # Already counted by cancel_session caller; we just # observe the terminal state here. diff --git a/inference_engine/scheduler/session.py b/inference_engine/scheduler/session.py index 219d03ce..1098954f 100644 --- a/inference_engine/scheduler/session.py +++ b/inference_engine/scheduler/session.py @@ -64,13 +64,6 @@ class Session: # the scheduler.iter_tokens() async iterator drain this; the # scheduler's worker pushes into it. token_queue: asyncio.Queue = field(default_factory=lambda: asyncio.Queue()) - # The engine's full result, set by the scheduler worker after - # ``engine.generate()`` returns. Route handlers read this to - # populate ADR 0007 §2.10 path-selection observability metrics - # (path_selection, tokens_skipped, prefill_duration_seconds) and - # acceptance-rate stats. ``None`` until the engine returns — - # callers must check before reading. - engine_result: Optional[object] = None def __post_init__(self) -> None: if not self.prompt_ids: diff --git a/inference_engine/server/app.py b/inference_engine/server/app.py index 88c8eaa8..485b1578 100644 --- a/inference_engine/server/app.py +++ b/inference_engine/server/app.py @@ -442,9 +442,8 @@ async def chat_completions(req: ChatCompletionRequest, request: Request): metrics.record_completion( finish_reason=finish_reason, n_tokens=len(output_token_ids), - acceptance_rate=_session_acceptance_rate(scheduler, session), + acceptance_rate=None, ) - _emit_path_selection_metric(metrics, session) return JSONResponse( content=ChatCompletionResponse( @@ -495,53 +494,6 @@ def _encode_prompt(engine: Engine, req: ChatCompletionRequest) -> List[int]: return prompt_ids -def _session_acceptance_rate( - scheduler: Scheduler, session: Session, -) -> Optional[float]: - """Per-session acceptance rate from the stashed EngineResult. - - The scheduler worker stores ``engine.generate()``'s result on - ``session.engine_result`` after generation completes (PR 7-4). - Returns ``None`` if the result is unavailable (session was - cancelled / failed before the engine returned, or the engine - is a test double that doesn't expose the field). - """ - _ = scheduler # kept for signature stability with existing callers - result = getattr(session, "engine_result", None) - if result is None: - return None - rate = getattr(result, "acceptance_rate", None) - if rate is None: - return None - return float(rate) - - -def _emit_path_selection_metric( - metrics: "Metrics", session: Session, -) -> None: - """Emit ADR 0007 §2.10 path-selection observability for one - completed session, if the engine reported the relevant fields. - - Called from both the streaming and non-streaming completion - paths after the session reaches a terminal state. No-op when - the engine result is unavailable (e.g., test doubles that - don't populate path_selection). - """ - result = getattr(session, "engine_result", None) - if result is None: - return - path = getattr(result, "path_selection", None) - if path not in ("continuation", "new_session"): - return - metrics.record_path_selection( - path=path, - tokens_skipped=int(getattr(result, "tokens_skipped", 0)), - prefill_duration_s=float( - getattr(result, "prefill_duration_seconds", 0.0) - ), - ) - - async def _collect_non_streaming_tokens( *, scheduler: Scheduler, @@ -662,7 +614,6 @@ def envelope(content_delta, role_delta, finish_reason) -> dict: metrics.record_completion( finish_reason=finish_reason, n_tokens=len(session.output_token_ids), - acceptance_rate=_session_acceptance_rate(scheduler, session), + acceptance_rate=None, ) - _emit_path_selection_metric(metrics, session) yield {"data": "[DONE]"} diff --git a/inference_engine/server/engine.py b/inference_engine/server/engine.py index 366eed7f..816f048f 100644 --- a/inference_engine/server/engine.py +++ b/inference_engine/server/engine.py @@ -44,13 +44,6 @@ class EngineResult: proposer_forward_calls: int verifier_forward_calls: int stopped_on_eos: bool - # ADR 0007 §2.10 observability — populated by the speculative - # engine; test doubles default to ``new_session`` / 0 so the - # route layer's metric emission code path is exercisable - # against either backend. - path_selection: str = "new_session" # "continuation" | "new_session" - tokens_skipped: int = 0 - prefill_duration_seconds: float = 0.0 @runtime_checkable @@ -191,11 +184,6 @@ def generate( proposer_forward_calls=int(result.proposer_forward_calls), verifier_forward_calls=int(result.verifier_forward_calls), stopped_on_eos=stopped_on_eos, - path_selection=str(getattr(result, "path_selection", "new_session")), - tokens_skipped=int(getattr(result, "tokens_skipped", 0)), - prefill_duration_seconds=float( - getattr(result, "prefill_duration_seconds", 0.0) - ), ) def kv_state(self) -> int: diff --git a/inference_engine/server/metrics.py b/inference_engine/server/metrics.py index 6372f750..e982cc47 100644 --- a/inference_engine/server/metrics.py +++ b/inference_engine/server/metrics.py @@ -107,13 +107,6 @@ class Metrics: scheduler_pending: Gauge scheduler_kv_live_bytes: Gauge scheduler_admission_total: Counter - # ADR 0007 §2.10 — cross-request KV reuse observability. - # Both ``path`` labels are first-class outcomes; neither is an - # "error" or "fallback" (per ADR 0007 §2.4.c). - path_selection_total: Counter - continuation_tokens_skipped_total: Counter - verifier_prefill_duration_seconds: Histogram - cache_invariant_violations_total: Counter @classmethod def build(cls) -> "Metrics": @@ -194,47 +187,6 @@ def build(cls) -> "Metrics": labelnames=["result"], registry=registry, ), - path_selection_total=Counter( - "path_selection_total", - "Total path-selection decisions made by the verifier " - "for cross-request KV cache reuse (ADR 0007 §2.4). " - "Both 'continuation' and 'new_session' are first-class " - "first-class outcomes; neither is an 'error' or " - "'fallback' (§2.4.c). Healthy long-session agent " - "workloads see continuation rate >= 95%.", - labelnames=["path"], - registry=registry, - ), - continuation_tokens_skipped_total=Counter( - "continuation_tokens_skipped_total", - "Cumulative prompt tokens that the continuation path " - "did not need to re-prefill (ADR 0007 §2.10). Sums " - "ContinuationPlan.skip_n across every continuation-" - "path request the server has handled. The win.", - registry=registry, - ), - verifier_prefill_duration_seconds=Histogram( - "verifier_prefill_duration_seconds", - "Wall time of the prefill phase of a single request, " - "partitioned by path. Continuation-path histogram " - "centers around per-incremental-token cost; " - "new-session-path histogram tracks full-prefill cost " - "(O(history_length)).", - labelnames=["path"], - buckets=( - 0.001, 0.005, 0.01, 0.05, 0.1, 0.5, - 1.0, 5.0, 10.0, 30.0, 60.0, 120.0, 300.0, - ), - registry=registry, - ), - cache_invariant_violations_total=Counter( - "cache_invariant_violations_total", - "Count of ADR 0007 §2.9 INV-1 / INV-2 detections at " - "runtime. Should always read 0; any non-zero value is " - "a critical operational alert (page on it).", - labelnames=["kind"], - registry=registry, - ), ) # ------------------------------------------------------------------ @@ -255,32 +207,6 @@ def record_admission(self, *, admitted: bool) -> None: result="admitted" if admitted else "rejected" ).inc() - def record_path_selection(self, *, path: str, tokens_skipped: int, - prefill_duration_s: float) -> None: - """Record one path-selection decision (ADR 0007 §2.10). - - ``path`` must be ``"continuation"`` or ``"new_session"``. The - method does not validate the label set explicitly because - prometheus-client's ``labels()`` already raises for unknown - labels; we want such a violation to surface loudly per the - no-silent-failure principle. - """ - self.path_selection_total.labels(path=path).inc() - if tokens_skipped > 0: - self.continuation_tokens_skipped_total.inc(tokens_skipped) - self.verifier_prefill_duration_seconds.labels(path=path).observe( - float(prefill_duration_s) - ) - - def record_cache_invariant_violation(self, *, kind: str) -> None: - """Record an INV-1 or INV-2 detection (ADR 0007 §2.9). - - ``kind`` must be ``"inv1"`` or ``"inv2"``. Should never be - called in healthy operation; any increment of this counter - is a critical alert. - """ - self.cache_invariant_violations_total.labels(kind=kind).inc() - def record_completion(self, *, finish_reason: str, n_tokens: int, acceptance_rate: Optional[float]) -> None: self.inference_completions_total.labels( diff --git a/scripts/bench_agentic/bench_long_session.py b/scripts/bench_agentic/bench_long_session.py index aef554eb..194018d2 100644 --- a/scripts/bench_agentic/bench_long_session.py +++ b/scripts/bench_agentic/bench_long_session.py @@ -53,31 +53,27 @@ hundreds of useful turns and never enter the timeout/recovery regime. -This bench scrapes the new ADR §2.10 metrics on every turn: - - * ``path_selection_total{path=continuation}`` Counter - * ``path_selection_total{path=new_session}`` Counter - * ``continuation_tokens_skipped_total`` Counter - * ``cache_invariant_violations_total{kind=...}`` Counter (must be 0) - -The aggregate report now includes an ``adr_0007`` block with: - - continuation_decisions int (count this run) - new_session_decisions int - total_decisions int - continuation_rate float in [0, 1] - tokens_skipped int (total prefill tokens reused) - cache_invariant_inv1_total int (must be 0) - cache_invariant_inv2_total int (must be 0) - -GA gate criteria (per ADR 0007 §6, applied to the 4h Mac M4 re-run): - - 1. ``agg.kv_bounded`` is True (memory bound holds across 4h) - 2. ``agg.n_errors`` < 5 (no sustained timeout/recovery loop) - 3. ``agg.n_turns`` >= 200 (vs 58 in v0.3.0-rc1) - 4. ``agg.latency_drift_p50_s`` <= 5 seconds (vs +39.74s in v0.3.0-rc1) - 5. ``adr_0007.continuation_rate`` >= 0.95 - 6. ``adr_0007.cache_invariant_inv1_total + inv2_total`` == 0 +PR-D1 (ADR 0008) note +--------------------- +The ADR 0007 path-selection metrics +(``path_selection_total``, ``continuation_tokens_skipped_total``, +``verifier_prefill_duration_seconds``, ``cache_invariant_violations_total``) +were removed by PR-D1 along with the ADR-0007-vintage code that +emitted them. The new session-bound architecture (ADR 0008) replaces +those counters; the replacement bench, ``bench_session_long_run.py``, +lands in PR-E1. This script remains in place as the regression +harness for ADR 0006 §2.3.a (memory-bounded long sessions) — its +``agg.kv_bounded`` / ``agg.kv_drift_bytes`` checks are protocol- +agnostic and still meaningful against any v0.3 server. + +Remaining GA gate criteria this script speaks to: + + 1. ``agg.kv_bounded`` is True (memory bound holds across the run) + 2. ``agg.n_errors`` is small (no sustained timeout/recovery loop) + 3. ``agg.latency_drift_p50_s`` (latency-bounded claim — v0.3 + non-claim per ADR 0006 §2.3.b until cross-request KV reuse is + wired through the gRPC AppendTokens path on the deprecated + HTTP shim too). Usage ----- @@ -187,21 +183,6 @@ def _client_rss_bytes() -> Optional[int]: "scheduler_kv_live_bytes", ) -# ADR 0007 §2.10 path-selection metrics — labeled, parsed separately -# from the unlabeled scheduler gauges. continuation_rate is the -# headline KPI for the cross-request KV reuse fix: ≥95% on a healthy -# long-session agent run. -_PATH_SELECTION_METRIC = "path_selection_total" -_CONTINUATION_TOKENS_SKIPPED_METRIC = "continuation_tokens_skipped_total" -_CACHE_INVARIANT_VIOLATIONS_METRIC = "cache_invariant_violations_total" - -# Match a Prometheus exposition line with labels: e.g. -# `path_selection_total{path="continuation"} 5.0` -_LABELED_METRIC_LINE = re.compile( - r'^(?P[a-zA-Z_:][a-zA-Z0-9_:]*)\{(?P[^}]*)\}\s+(?P[-+0-9eE.\.NaNinf]+)\s*$' -) - - _METRIC_LINE = re.compile( r"^(?P[a-zA-Z_:][a-zA-Z0-9_:]*)(\{[^}]*\})?\s+(?P[-+0-9eE.\.NaNinf]+)\s*$" ) @@ -210,18 +191,11 @@ def _client_rss_bytes() -> Optional[int]: def _parse_prom_text(body: str) -> dict[str, float]: """Tiny Prometheus text parser. - Returns a flat dict where unlabeled metrics keep their bare - name and labeled ADR 0007 §2.10 metrics get a synthesized name - of the form ``"{base}__{label_value}"``: - - * ``path_selection_total{path="continuation"}`` → - ``"path_selection_total__continuation"`` - * ``path_selection_total{path="new_session"}`` → - ``"path_selection_total__new_session"`` - * ``cache_invariant_violations_total{kind="inv1"}`` → - ``"cache_invariant_violations_total__inv1"`` - * ``continuation_tokens_skipped_total`` (no label) → - ``"continuation_tokens_skipped_total"`` + Returns a flat dict of unlabeled metric name -> value. Labeled + metrics are silently ignored (the previous ADR 0007 §2.10 + handling — path_selection_total{path=...} etc. — was removed + in PR-D1; this script's headline KPIs are the unlabeled + scheduler gauges in :data:`_METRIC_NAMES`). Values are coerced to float; ``NaN`` and ``inf`` are preserved. """ @@ -229,31 +203,11 @@ def _parse_prom_text(body: str) -> dict[str, float]: for line in body.splitlines(): if not line or line.startswith("#"): continue - # Try labeled match first (more specific). - labeled = _LABELED_METRIC_LINE.match(line) - if labeled is not None: - name = labeled.group("name") - label_str = labeled.group("labels") - try: - value = float(labeled.group("value")) - except ValueError: # pragma: no cover - malformed exporter - continue - if name == _PATH_SELECTION_METRIC: - # Extract the path label (continuation|new_session) - path = _extract_label(label_str, "path") - if path: - out[f"{name}__{path}"] = value - elif name == _CACHE_INVARIANT_VIOLATIONS_METRIC: - kind = _extract_label(label_str, "kind") - if kind: - out[f"{name}__{kind}"] = value - continue - # Unlabeled. m = _METRIC_LINE.match(line) if m is None: continue name = m.group("name") - if name in _METRIC_NAMES or name == _CONTINUATION_TOKENS_SKIPPED_METRIC: + if name in _METRIC_NAMES: try: out[name] = float(m.group("value")) except ValueError: # pragma: no cover - malformed exporter @@ -261,24 +215,6 @@ def _parse_prom_text(body: str) -> dict[str, float]: return out -def _extract_label(label_str: str, key: str) -> Optional[str]: - """Pull a single label value out of a Prometheus label segment. - - Input is the part between the curly braces, e.g. - ``path="continuation"`` or - ``path="continuation",result="ok"``. Returns the value (without - quotes) for the requested key, or ``None`` if absent. - """ - for fragment in label_str.split(","): - fragment = fragment.strip() - if "=" not in fragment: - continue - k, v = fragment.split("=", 1) - if k.strip() == key: - return v.strip().strip('"') - return None - - async def _scrape_metrics( client: httpx.AsyncClient, *, @@ -751,58 +687,6 @@ def _aggregate( } -def _adr_0007_summary(turns: list[dict[str, Any]]) -> dict[str, Any]: - """ADR 0007 §2.10 path-selection summary from the per-turn idle - scrapes (`metrics_idle`). - - The path_selection_total counters are CUMULATIVE across the - server's lifetime, so we take the LAST observed value (= final - counter at end of run) and the FIRST observed value (= counter - at start of run, may be > 0 if the server already handled - requests before this bench started). Difference = decisions - this run made. - - Returns a dict with: - continuation_decisions int - new_session_decisions int - total_decisions int - continuation_rate float in [0, 1] (None if 0 decisions) - tokens_skipped int (delta of the counter) - cache_invariant_inv1_total int (last - first; should be 0) - cache_invariant_inv2_total int (last - first; should be 0) - """ - def _delta(name: str) -> int: - first: Optional[float] = None - last: Optional[float] = None - for t in turns: - idle = t.get("metrics_idle") or {} - v = idle.get(name) - if v is None: - continue - if first is None: - first = v - last = v - if first is None or last is None: - return 0 - return int(round(last - first)) - - cont = _delta(f"{_PATH_SELECTION_METRIC}__continuation") - news = _delta(f"{_PATH_SELECTION_METRIC}__new_session") - skipped = _delta(_CONTINUATION_TOKENS_SKIPPED_METRIC) - inv1 = _delta(f"{_CACHE_INVARIANT_VIOLATIONS_METRIC}__inv1") - inv2 = _delta(f"{_CACHE_INVARIANT_VIOLATIONS_METRIC}__inv2") - total = cont + news - return { - "continuation_decisions": cont, - "new_session_decisions": news, - "total_decisions": total, - "continuation_rate": (cont / total) if total > 0 else None, - "tokens_skipped": skipped, - "cache_invariant_inv1_total": inv1, - "cache_invariant_inv2_total": inv2, - } - - def _build_payload( *, turns: list[dict[str, Any]], @@ -821,7 +705,6 @@ def _build_payload( "turns": turns, "errors": errors, "agg": _aggregate(turns, errors, duration_s), - "adr_0007": _adr_0007_summary(turns), } @@ -880,41 +763,11 @@ def _fmt_bytes(b: Optional[float]) -> str: f"{b['p95_latency_s']:>7.3f}s " f"{(mkb / (1024 * 1024)) if mkb is not None else float('nan'):>7.1f} MiB" ) - # ADR 0007 §2.10 path-selection block (only emitted when the - # payload actually carries the data — backward-compat with old - # checkpoints that pre-date PR 7-6). - adr_0007 = payload.get("adr_0007") - if adr_0007 is not None: - rate = adr_0007.get("continuation_rate") - rate_str = ( - f"{rate * 100:.2f}%" if rate is not None else "n/a (no decisions)" - ) - lines.append("") - lines.append(" ADR 0007 §2.10 — cross-request KV reuse") - lines.append( - f" continuation decisions = " - f"{adr_0007['continuation_decisions']}" - ) - lines.append( - f" new-session decisions = " - f"{adr_0007['new_session_decisions']}" - ) - lines.append(f" continuation rate = {rate_str}") - lines.append( - f" total tokens skipped = " - f"{adr_0007['tokens_skipped']:,}" - ) - inv1 = adr_0007["cache_invariant_inv1_total"] - inv2 = adr_0007["cache_invariant_inv2_total"] - inv_marker = "" if (inv1 == 0 and inv2 == 0) else " ← CRITICAL" - lines.append( - f" INV-1 violations = {inv1}" - + (inv_marker if inv1 > 0 else "") - ) - lines.append( - f" INV-2 violations = {inv2}" - + (inv_marker if inv2 > 0 else "") - ) + # Old ADR 0007 §2.10 path-selection summary block was removed in + # PR-D1 along with the metrics that fed it. Historical reports + # produced by earlier commits retain the ``adr_0007`` payload + # field; we ignore it here. Replacement bench is PR-E1's + # ``bench_session_long_run.py``. lines.append("=" * 78) return "\n".join(lines) diff --git a/tests/inference_engine/server/test_app_metrics_and_auth.py b/tests/inference_engine/server/test_app_metrics_and_auth.py index f8f4c189..c1f4fac6 100644 --- a/tests/inference_engine/server/test_app_metrics_and_auth.py +++ b/tests/inference_engine/server/test_app_metrics_and_auth.py @@ -164,104 +164,6 @@ def kv_state(self) -> int: "scheduler_kv_live_bytes 12345678" in r.text -async def test_metrics_path_selection_metrics_present_on_idle_metrics_scrape( - short_engine, -): - """ADR 0007 §2.10: the new path-selection metrics must be - exposed on /metrics. At idle (no requests have completed) the - counters are 0 and the histogram has no observations — the - contract is just that they exist.""" - app = create_app(short_engine, ServerConfig(max_concurrent=1)) - async with AsyncClient(transport=ASGITransport(app=app), - base_url="http://t") as c: - r = await c.get("/metrics") - assert r.status_code == 200 - text = r.text - assert "# HELP path_selection_total" in text - assert "# HELP continuation_tokens_skipped_total" in text - assert "# HELP verifier_prefill_duration_seconds" in text - assert "# HELP cache_invariant_violations_total" in text - # Counter starts at 0; no completion has happened yet. - assert "continuation_tokens_skipped_total 0.0" in text - - -async def test_metrics_path_selection_recorded_after_completion(short_engine): - """End-to-end: a completed chat-completions request emits the - path_selection metric. The DeterministicEngine test double - defaults path_selection to 'new_session' (its EngineResult uses - the dataclass defaults), so we expect new_session to be - incremented.""" - app = create_app(short_engine, ServerConfig(max_concurrent=1)) - async with AsyncClient(transport=ASGITransport(app=app), - base_url="http://t") as c: - await c.post("/v1/chat/completions", json={ - "model": "m", - "messages": [{"role": "user", "content": "hi"}], - }) - r = await c.get("/metrics") - text = r.text - assert 'path_selection_total{path="new_session"} 1.0' in text - - -# --------------------------------------------------------------------------- -# Defensive None-handling in _session_acceptance_rate and -# _emit_path_selection_metric (reachable when EngineResult is partially -# populated, e.g. an engine that completed without an acceptance rate or -# without a populated path_selection field). These helpers are private but -# must hold their early-return contracts; the route-handler relies on -# either returning None / no-op rather than raising AttributeError or -# emitting a counter with a label outside the documented label set. -# --------------------------------------------------------------------------- - - -async def test_session_acceptance_rate_returns_none_when_result_missing_rate(): - """A completed session whose EngineResult-shaped object has - ``acceptance_rate=None`` (e.g., a fast-aborted engine that did - not measure acceptance) must yield ``None`` from the helper, not - raise. Covers app.py:_session_acceptance_rate None-rate branch. - """ - from types import SimpleNamespace - - from inference_engine.scheduler.session import Session - from inference_engine.server.app import _session_acceptance_rate - - sess = Session(prompt_ids=[1], max_new_tokens=1, eos_token_ids=[2]) - sess.engine_result = SimpleNamespace(acceptance_rate=None) - - assert _session_acceptance_rate(scheduler=object(), session=sess) is None - - -async def test_emit_path_selection_metric_noop_when_path_unset(short_engine): - """An EngineResult whose ``path_selection`` is anything other than - 'continuation' or 'new_session' (commonly ``None`` for engines / - test doubles that never populated it) must produce **no** metric - write — emitting a counter with an undocumented label would - pollute the label set. Covers app.py:_emit_path_selection_metric - early-return branch. - """ - from types import SimpleNamespace - - from inference_engine.scheduler.session import Session - from inference_engine.server.app import _emit_path_selection_metric - - app = create_app(short_engine, ServerConfig(max_concurrent=1)) - metrics = app.state.metrics - - sess = Session(prompt_ids=[1], max_new_tokens=1, eos_token_ids=[2]) - sess.engine_result = SimpleNamespace( - path_selection=None, tokens_skipped=0, prefill_duration_seconds=0.0, - ) - - text_before = metrics.render().decode() - _emit_path_selection_metric(metrics, sess) - text_after = metrics.render().decode() - - # No write happened: counter unchanged, no spurious label appeared. - assert text_before == text_after - assert 'path_selection_total{path="None"}' not in text_after - assert 'path_selection_total{path="unknown"}' not in text_after - - async def test_metrics_kv_live_bytes_zero_when_no_active_session(tokenizer): """Between turns the verifier may hold residual KV (next prefill will reset it, but until then it sits in self.cache). Reporting diff --git a/tests/inference_engine/server/test_metrics.py b/tests/inference_engine/server/test_metrics.py index e1648856..c9ce76ac 100644 --- a/tests/inference_engine/server/test_metrics.py +++ b/tests/inference_engine/server/test_metrics.py @@ -41,11 +41,6 @@ def test_build_registers_all_documented_metrics(metrics): "scheduler_pending", "scheduler_kv_live_bytes", "scheduler_admission_total", - # ADR 0007 §2.10 - "path_selection_total", - "continuation_tokens_skipped_total", - "verifier_prefill_duration_seconds", - "cache_invariant_violations_total", } found = set() for collector in metrics.registry._collector_to_names.values(): @@ -257,105 +252,3 @@ def _get_metric_value( return 0.0 -# --------------------------------------------------------------------------- -# ADR 0007 §2.10 — path_selection observability (PR 7-4) -# --------------------------------------------------------------------------- - - -def test_record_path_selection_continuation_increments_counter(metrics): - metrics.record_path_selection( - path="continuation", tokens_skipped=42, prefill_duration_s=0.05, - ) - assert _get_metric_value( - metrics, "path_selection_total", labels={"path": "continuation"}, - ) == 1.0 - - -def test_record_path_selection_new_session_increments_counter(metrics): - metrics.record_path_selection( - path="new_session", tokens_skipped=0, prefill_duration_s=2.5, - ) - assert _get_metric_value( - metrics, "path_selection_total", labels={"path": "new_session"}, - ) == 1.0 - - -def test_record_path_selection_continuation_increments_tokens_skipped(metrics): - metrics.record_path_selection( - path="continuation", tokens_skipped=100, prefill_duration_s=0.01, - ) - metrics.record_path_selection( - path="continuation", tokens_skipped=50, prefill_duration_s=0.01, - ) - assert _get_metric_value( - metrics, "continuation_tokens_skipped_total", - ) == 150.0 - - -def test_record_path_selection_new_session_does_not_increment_tokens_skipped( - metrics, -): - """New-session path has tokens_skipped=0 by construction; we - only count continuation savings.""" - metrics.record_path_selection( - path="new_session", tokens_skipped=0, prefill_duration_s=5.0, - ) - assert _get_metric_value( - metrics, "continuation_tokens_skipped_total", - ) == 0.0 - - -def test_record_path_selection_observes_prefill_duration_histogram(metrics): - metrics.record_path_selection( - path="continuation", tokens_skipped=0, prefill_duration_s=0.05, - ) - count = _get_metric_value( - metrics, "verifier_prefill_duration_seconds_count", - labels={"path": "continuation"}, - ) - assert count == 1.0 - - -def test_record_path_selection_partitions_histogram_by_path(metrics): - """Continuation and new-session paths must appear in separate - histogram label groups so per-path cost profiles are visible.""" - metrics.record_path_selection( - path="continuation", tokens_skipped=10, prefill_duration_s=0.01, - ) - metrics.record_path_selection( - path="new_session", tokens_skipped=0, prefill_duration_s=2.0, - ) - assert _get_metric_value( - metrics, "verifier_prefill_duration_seconds_count", - labels={"path": "continuation"}, - ) == 1.0 - assert _get_metric_value( - metrics, "verifier_prefill_duration_seconds_count", - labels={"path": "new_session"}, - ) == 1.0 - - -def test_record_cache_invariant_violation_inv1(metrics): - metrics.record_cache_invariant_violation(kind="inv1") - assert _get_metric_value( - metrics, "cache_invariant_violations_total", labels={"kind": "inv1"}, - ) == 1.0 - - -def test_record_cache_invariant_violation_inv2(metrics): - metrics.record_cache_invariant_violation(kind="inv2") - assert _get_metric_value( - metrics, "cache_invariant_violations_total", labels={"kind": "inv2"}, - ) == 1.0 - - -def test_cache_invariant_violations_default_zero(metrics): - """Should always read 0 in healthy operation. Serves as a - 'page-on-non-zero' alert target for operators.""" - # Don't call record_cache_invariant_violation. Counter stays at 0. - assert _get_metric_value( - metrics, "cache_invariant_violations_total", labels={"kind": "inv1"}, - ) == 0.0 - assert _get_metric_value( - metrics, "cache_invariant_violations_total", labels={"kind": "inv2"}, - ) == 0.0