From 28ef745384fc4063cd3d67429d0a4f300507426c Mon Sep 17 00:00:00 2001 From: Jon Saad-Falcon <41205309+jonsaadfalcon@users.noreply.github.com> Date: Thu, 4 Jun 2026 16:59:24 -0700 Subject: [PATCH] fix(leaderboard): correct telemetry pipeline and outlier handling (#498) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit * fix(leaderboard): correct telemetry pipeline and outlier handling The public leaderboard at /leaderboard showed a clear bimodal Wh/token distribution: most users at ~3-5 J/token, ~30% inflated by 1000-4000×. A 5-agent investigation workflow + 4-agent verification pass traced the inflation to a cluster of related bugs across telemetry, server, and display layers. This PR fixes the in-repo half. Backfilling existing Supabase data is a separate follow-up. Bug 1 — Dual telemetry recording (`server/routes.py`) ===================================================== `_handle_direct` wrapped the engine with `instrumented_generate` unconditionally. When `app.state.engine` was already an `InstrumentedEngine` (the common case when telemetry is wired in), BOTH layers published `TELEMETRY_RECORD` — once from the inner `InstrumentedEngine.generate`, once from the outer wrapper. Every chat-completion request was counted twice in the leaderboard pipeline. Fix: detect the InstrumentedEngine and unwrap to `._inner` before passing to the wrapper so only one layer fires. Bug 2 — KV-cache fallback over-counts multi-turn (`server/savings.py`) ===================================================================== `compute_savings` falls back from `prompt_tokens_evaluated` to `prompt_tokens` when the KV-cache-aware count is missing. But routes.py aggregates by summing each turn's full prompt — which counts the system prompt N times for an N-turn conversation. The fallback inflated FLOPs and energy by N×. Fix: use 0 (conservative under-count) when the evaluated count is missing rather than falling back to the inflated `prompt_tokens` sum. Bug 3 — TelemetryRecord lacked methodology versioning ===================================================== There was no per-record version tag, so legacy (pre-fix) and current records were silently aggregated together in the public leaderboard even though they used different methodologies. Fix: add `token_counting_version: Optional[int]` field to the `TelemetryRecord` dataclass + a nullable column to the SQLite schema (with idempotent migration); the constant moves from `server.savings` to `core.types` to avoid the server→telemetry layering. New records write the current version; pre-fix rows remain NULL. The aggregator gains a `current_methodology_only=True` flag that filters NULL rows out of leaderboard sums — local dashboards leave it off so historical aggregates still render. Bug 4 — Leaderboard JS displayed missing telemetry as legit zeros ================================================================= Rows with significant token counts but `energy_wh_saved = 0` and `flops_saved = 0` rendered as `0.00 Wh / 0 FLOPs` — visually identical to a user who genuinely did almost nothing. The headline totals also included these rows. Fix: new `isMissingTelemetry()` detector renders missing-telemetry energy/FLOPs cells as `—` (with a tooltip explaining why); a new `.lb-missing` CSS class differentiates the placeholder visually. Bug 5 — Outlier filter was too generous ======================================= `MAX_ENERGY_WH_PER_TOKEN = 10` left a 10,000× margin that admitted every Group B row even though those values are physically impossible (would imply a space-heater per token). Same for the FLOPs cap at `1e17`. Fix: tighten to `0.5` Wh/token (still 500× over a typical consumer GPU) and `1e15` FLOPs/token (still 10,000× over typical). This removes existing pre-fix corrupt rows from the public view without touching Supabase. Tests ===== New regression tests (all pass, ruff clean): - `tests/server/test_routes.py::test_instrumented_engine_unwrapped_to_avoid_dual_telemetry` — pins the bug 1 fix; asserts exactly ONE TELEMETRY_RECORD event per request when the engine is already an InstrumentedEngine. - `tests/server/test_savings.py` (NEW file, 3 tests) — pins the bug 2 fix (FLOPs not inflated via fallback) and the cost-side invariant (dollar savings still use full prompt_tokens because cloud providers bill per input token even when local KV cache hit). - `tests/telemetry/test_aggregator.py::TestMethodologyFilter` (3 tests) — default behaviour includes legacy rows (local dashboard parity); `current_methodology_only=True` excludes them; the summary surface honors the same filter. Unrelated test note =================== `tests/telemetry/test_energy_wiring.py::TestTelemetryStatsEnergy::test_export_includes_energy_fields` and `::TestEndToEndPipeline::test_ask_to_export_with_energy` are flaky on this branch but ALSO flaky on `main` (confirmed via stash + the banner-only run). Root cause: `_version_check.py:132-135` prints the "new version of OpenJarvis is available" banner to stdout outside the throttle window, contaminating `json.loads(result.output)` in those two tests. Reproducible with `OPENJARVIS_NO_UPDATE_CHECK=1` → all pass. That's a separate bug (the banner shouldn't write to the same stream as machine-readable output); not addressing it here to keep this PR focused on the leaderboard pipeline. What this PR explicitly does NOT do ==================================== - Backfill ~30% of Supabase rows that already shipped with inflated values from pre-fix clients. That requires Supabase write access and is the natural follow-up — see PR comments for proposed dry-run audit query and the additive `methodology_version` column migration. - Distinguish which past submissions came from buggy vs correct clients retroactively (no `app_version` tag on existing submissions). Co-Authored-By: Claude Opus 4.7 * test(evals): fix test_energy_scales_linearly under leaderboard fix CI on #498 failed in the slow `test` job: tests/evals/test_use_case_benchmarks.py::TestSavings:: test_energy_scales_linearly — ZeroDivisionError: float division by zero Root cause: the test (and `test_energy_wh_matches_direct_formula`) called `compute_savings(N, 0)` without `prompt_tokens_evaluated`. Under the OLD buggy fallback, an unset evaluated count silently became N, energy scaled linearly with N, and the test passed by accident. Under this branch's conservative fix the fallback is 0, FLOPs collapse to 0, and the `p10.energy_wh / p1.energy_wh` ratio divides 0 by 0. The test's own docstring says "evaluated tokens (KV-cache model)" — it was always meant to exercise the explicit-evaluated path, the API call just didn't match. Pass `prompt_tokens_evaluated` explicitly so the test now expresses the invariant it claims to. Same one-line fix for `test_energy_wh_matches_direct_formula` plus an explicit `flops > 0` sanity assertion — that test was passing trivially with `0 == 0` after the conservative fallback fix, masking whether the formula was actually being exercised. No production code change in this commit. Verified locally: all 6 TestSavings tests pass; the 5 new regression tests added on this branch still pass. Co-Authored-By: Claude Opus 4.7 --------- Co-authored-by: krypticmouse Co-authored-by: Claude Opus 4.7 --- docs/javascripts/leaderboard.js | 44 ++++++++++- docs/stylesheets/extra.css | 7 ++ src/openjarvis/core/types.py | 18 +++++ src/openjarvis/server/routes.py | 57 ++++++++++--- src/openjarvis/server/savings.py | 28 +++++-- src/openjarvis/telemetry/aggregator.py | 38 +++++++-- .../telemetry/instrumented_engine.py | 9 ++- src/openjarvis/telemetry/store.py | 13 ++- tests/evals/test_use_case_benchmarks.py | 33 ++++++-- tests/server/test_routes.py | 69 ++++++++++++++++ tests/server/test_savings.py | 79 +++++++++++++++++++ tests/telemetry/test_aggregator.py | 58 ++++++++++++++ 12 files changed, 418 insertions(+), 35 deletions(-) create mode 100644 tests/server/test_savings.py diff --git a/docs/javascripts/leaderboard.js b/docs/javascripts/leaderboard.js index ce22868c..ba1950ed 100644 --- a/docs/javascripts/leaderboard.js +++ b/docs/javascripts/leaderboard.js @@ -12,8 +12,14 @@ // Outlier detection — hide entries with values that are physically // implausible relative to their token count. Thresholds are ~1000x // above legitimate per-token values to avoid false positives. - var MAX_ENERGY_WH_PER_TOKEN = 10; // legit ≈ 0.001 Wh/tok - var MAX_FLOPS_PER_TOKEN = 1e17; // legit ≈ 1e12 /tok + // Outlier bounds. Set well above realistic upper limits but tight + // enough to drop the pre-fix bimodal Group B (1-5 Wh/token, ~3e16 + // FLOPs/token) — see the leaderboard PR for the full diagnosis. + // Realistic per-token rates on a consumer GPU + 10–30B local model: + // ~0.001 Wh/token, ~1e10–1e11 FLOPs/token. We allow 500× and 10,000× + // headroom respectively for inefficient hardware / larger models. + var MAX_ENERGY_WH_PER_TOKEN = 0.5; // legit ≈ 0.001 Wh/tok + var MAX_FLOPS_PER_TOKEN = 1e15; // legit ≈ 1e11 /tok var MAX_DOLLAR_PER_TOKEN = 25.0 / 1e6; // hard ceiling: $25/1M output function isOutlier(row) { @@ -29,6 +35,25 @@ ); } + // Distinguish "user actually has zero work done" from "user's energy / + // FLOPs telemetry never landed". The latter happens when the server + // submits with valid dollar savings + token counts but the per-record + // energy stamp was missing (pre-fix builds, GPU energy meter + // unavailable, etc.). Without this check those rows show as "0.00 Wh" + // and skew the rankings + headline totals. + // + // Threshold: 1000 tokens is well above any single chat-turn — if a + // user has that many tokens recorded but no measured energy, the + // telemetry is incomplete, not legitimately zero. + var MIN_TOKENS_FOR_TELEMETRY = 1000; + + function isMissingTelemetry(row) { + var tokens = Number(row.total_tokens) || 0; + var energy = Number(row.energy_wh_saved) || 0; + var flops = Number(row.flops_saved) || 0; + return tokens > MIN_TOKENS_FOR_TELEMETRY && energy === 0 && flops === 0; + } + function escapeHtml(s) { var el = document.createElement("span"); el.textContent = s; @@ -62,13 +87,24 @@ var medal = rank === 1 ? "\uD83E\uDD47" : rank === 2 ? "\uD83E\uDD48" : rank === 3 ? "\uD83E\uDD49" : ""; var row = pageRows[j]; + // Render "—" for energy / FLOPs columns when telemetry didn't + // land (vs the user genuinely having 0). The dollar / request / + // token columns are unaffected because those measurements landed + // even when energy didn't. + var missing = isMissingTelemetry(row); + var energyCell = missing + ? '—' + : '' + Number(row.energy_wh_saved || 0).toFixed(2) + ""; + var flopsCell = missing + ? '—' + : '' + fmtLarge(Number(row.flops_saved || 0)) + ""; html += "" + '' + (medal || rank) + "" + '' + escapeHtml(row.display_name) + "" + '$' + Number(row.dollar_savings || 0).toFixed(4) + "" + - '' + Number(row.energy_wh_saved || 0).toFixed(2) + "" + - '' + fmtLarge(Number(row.flops_saved || 0)) + "" + + energyCell + + flopsCell + '' + Number(row.total_calls || 0).toLocaleString() + "" + '' + Number(row.total_tokens || 0).toLocaleString() + "" + ""; diff --git a/docs/stylesheets/extra.css b/docs/stylesheets/extra.css index c0a0cb73..7079e0fd 100644 --- a/docs/stylesheets/extra.css +++ b/docs/stylesheets/extra.css @@ -440,6 +440,13 @@ font-family: var(--md-code-font-family, monospace); font-size: 13px; } +/* Placeholder for rows where energy / FLOPs telemetry didn't land. + Distinguishes "telemetry missing" from "user genuinely had 0 work + done" without making the row visually pop more than data rows. */ +.lb-missing { + color: var(--md-default-fg-color--light, #999); + font-style: italic; +} /* ── DocSearch ───────────────────────────────────────────────────────── */ #docsearch { diff --git a/src/openjarvis/core/types.py b/src/openjarvis/core/types.py index 62d2e6ba..849a5269 100644 --- a/src/openjarvis/core/types.py +++ b/src/openjarvis/core/types.py @@ -125,6 +125,16 @@ class ToolResult: metadata: Dict[str, Any] = field(default_factory=dict) +# Bump when token-counting methodology changes so the leaderboard can +# distinguish entries computed under different rules. +# v1 = original (Ollama prompt_eval_count, may under-count due to KV cache) +# v2 = full prompt token count, no KV-cache assumption, system prompt +# always counted +# Lives here (not in server/savings) so the telemetry layer can read it +# without crossing the server → telemetry layering. +TOKEN_COUNTING_VERSION: int = 2 + + @dataclass(slots=True) class TelemetryRecord: """Single telemetry observation recorded after an inference call.""" @@ -167,6 +177,14 @@ class TelemetryRecord: gpu_energy_joules: float = 0.0 dram_energy_joules: float = 0.0 tokens_per_joule: float = 0.0 + # Version tag for the token-counting methodology used when this record + # was produced. `None` (= legacy) means the record predates per-record + # versioning; the leaderboard aggregator filters those out to avoid + # mixing pre-fix and post-fix records in the same per-token efficiency + # metric — they were the dominant source of the bimodal Wh/token + # distribution on the public leaderboard. New records always write + # `TOKEN_COUNTING_VERSION` from `server/savings.py`. + token_counting_version: Optional[int] = None mining_session_id: Optional[str] = None metadata: Dict[str, Any] = field(default_factory=dict) diff --git a/src/openjarvis/server/routes.py b/src/openjarvis/server/routes.py index 49498199..3c479c5a 100644 --- a/src/openjarvis/server/routes.py +++ b/src/openjarvis/server/routes.py @@ -195,17 +195,50 @@ def _handle_direct( if req.tools: kwargs["tools"] = req.tools if bus: + from openjarvis.telemetry.instrumented_engine import InstrumentedEngine from openjarvis.telemetry.wrapper import instrumented_generate - result = instrumented_generate( - engine, - messages, - model=model, - bus=bus, - temperature=req.temperature, - max_tokens=req.max_tokens, - **kwargs, - ) + # `app.state.engine` may already be an InstrumentedEngine (the + # common case when telemetry is wired in). If we then wrap it + # with `instrumented_generate`, BOTH layers fire a + # TELEMETRY_RECORD per call: + # + # - InstrumentedEngine.generate() publishes a FULL record + # (energy_joules, GPU stats, token_counting_version, ...). + # - instrumented_generate() publishes a BARE record (timing + + # tokens only; no energy meter, no version stamp). + # + # The doubled count was the dominant driver of the bimodal + # Wh/token distribution on the public leaderboard. + # + # The fix below is NOT "unwrap and call instrumented_generate": + # that would have replaced "doubled records" with "every + # request emits only a bare record with no energy / no version", + # which the leaderboard's `current_methodology_only=True` filter + # would then drop entirely. Instead, when the engine is already + # an InstrumentedEngine, skip the wrapper and call `generate` + # directly — InstrumentedEngine publishes the full per-record + # event itself with energy + version intact. Only fall back to + # the lightweight wrapper for engines that aren't already + # instrumented. + if isinstance(engine, InstrumentedEngine): + result = engine.generate( + messages, + model=model, + temperature=req.temperature, + max_tokens=req.max_tokens, + **kwargs, + ) + else: + result = instrumented_generate( + engine, + messages, + model=model, + bus=bus, + temperature=req.temperature, + max_tokens=req.max_tokens, + **kwargs, + ) else: result = engine.generate( messages, @@ -736,7 +769,11 @@ async def savings(request: Request): agg = TelemetryAggregator(db_path) try: - summary = agg.summary(since=session_start) + # current_methodology_only excludes pre-fix legacy rows from + # the leaderboard's per-token efficiency numerator/denominator + # — see the comment on _time_filter for the bimodal-Wh/token + # background. + summary = agg.summary(since=session_start, current_methodology_only=True) # Exclude cloud model tokens from savings — only local # inference counts toward cost savings. _cloud_prefixes = ( diff --git a/src/openjarvis/server/savings.py b/src/openjarvis/server/savings.py index c8e6e539..c8e26a44 100644 --- a/src/openjarvis/server/savings.py +++ b/src/openjarvis/server/savings.py @@ -10,11 +10,12 @@ import time from dataclasses import asdict, dataclass, field from typing import Any, Dict, List -# Bump when token-counting methodology changes so that the leaderboard -# can distinguish entries computed under different rules. -# v1 = original (Ollama prompt_eval_count, may under-count due to KV cache) -# v2 = full prompt token count, no KV-cache assumption, system prompt always counted -TOKEN_COUNTING_VERSION: int = 2 +# Source of truth for the methodology-version constant lives in +# `openjarvis.core.types` so the telemetry layer can read it without +# crossing the server → telemetry layering. Re-exported here for +# backward compatibility with existing imports of `from +# openjarvis.server.savings import TOKEN_COUNTING_VERSION`. +from openjarvis.core.types import TOKEN_COUNTING_VERSION # noqa: E402,F401 # --------------------------------------------------------------------------- # Cloud provider pricing (USD per 1M tokens) @@ -104,11 +105,22 @@ def compute_savings( served from KV cache. Used for **FLOPs** and **energy** calculations since these reflect actual compute. - If ``prompt_tokens_evaluated`` is 0 (e.g. old telemetry without the - column), it falls back to ``prompt_tokens``. + When ``prompt_tokens_evaluated`` is 0 we used to fall back to + ``prompt_tokens``. That's wrong in multi-turn: routes.py aggregates + by summing each turn's full prompt — which counts the system prompt + N times for an N-turn conversation — so the fallback turned the FLOPs + and energy estimates into N×-too-high numbers. That was the dominant + contributor to the bimodal Wh/token distribution on the leaderboard. + + Conservative behaviour now: when the KV-cache-aware count is + missing, treat `prompt_tokens_evaluated` as 0 — so the FLOPs/energy + denominator becomes just `completion_tokens`. That under-estimates + rather than over-estimates compute, and (intentionally) cascades + into the leaderboard's `isMissingTelemetry` UI render so those + rows show `—` instead of a misleading zero. """ if prompt_tokens_evaluated <= 0: - prompt_tokens_evaluated = prompt_tokens + prompt_tokens_evaluated = 0 total_tokens = prompt_tokens + completion_tokens total_tokens_evaluated = prompt_tokens_evaluated + completion_tokens providers: List[ProviderSavings] = [] diff --git a/src/openjarvis/telemetry/aggregator.py b/src/openjarvis/telemetry/aggregator.py index ead6c5ed..79ac6fec 100644 --- a/src/openjarvis/telemetry/aggregator.py +++ b/src/openjarvis/telemetry/aggregator.py @@ -92,12 +92,22 @@ class TelemetryAggregator: self._conn = sqlite3.connect(self._db_path, check_same_thread=False) self._conn.row_factory = sqlite3.Row - @staticmethod def _time_filter( + self, since: Optional[float] = None, until: Optional[float] = None, + current_methodology_only: bool = False, ) -> tuple[str, list[Any]]: - """Build a WHERE clause fragment for time-range filtering.""" + """Build a WHERE clause fragment for time + methodology filtering. + + ``current_methodology_only`` is opt-in (default False) because the + local dashboard still wants to render historical aggregates that + predate the per-record version stamp. The leaderboard ingest path + in ``server/routes.savings`` flips it to True so legacy rows (the + ones with NULL token_counting_version) don't pollute the public + per-token efficiency metric — they were the dominant source of + the bimodal Wh/token distribution. + """ clauses: list[str] = [] params: list[Any] = [] if since is not None: @@ -106,6 +116,11 @@ class TelemetryAggregator: if until is not None: clauses.append("timestamp <= ?") params.append(until) + if current_methodology_only and self._safe_col("token_counting_version"): + from openjarvis.core.types import TOKEN_COUNTING_VERSION + + clauses.append("token_counting_version = ?") + params.append(TOKEN_COUNTING_VERSION) if clauses: return " WHERE " + " AND ".join(clauses), params return "", params @@ -124,8 +139,11 @@ class TelemetryAggregator: *, since: Optional[float] = None, until: Optional[float] = None, + current_methodology_only: bool = False, ) -> List[ModelStats]: - where, params = self._time_filter(since, until) + where, params = self._time_filter( + since, until, current_methodology_only=current_methodology_only + ) # Build optional columns for new fields (graceful on old DBs) extra_cols = "" @@ -215,8 +233,11 @@ class TelemetryAggregator: *, since: Optional[float] = None, until: Optional[float] = None, + current_methodology_only: bool = False, ) -> List[EngineStats]: - where, params = self._time_filter(since, until) + where, params = self._time_filter( + since, until, current_methodology_only=current_methodology_only + ) extra_cols = "" has_tpj = self._safe_col("tokens_per_joule") @@ -305,9 +326,14 @@ class TelemetryAggregator: *, since: Optional[float] = None, until: Optional[float] = None, + current_methodology_only: bool = False, ) -> AggregatedStats: - model_stats = self.per_model_stats(since=since, until=until) - engine_stats = self.per_engine_stats(since=since, until=until) + model_stats = self.per_model_stats( + since=since, until=until, current_methodology_only=current_methodology_only + ) + engine_stats = self.per_engine_stats( + since=since, until=until, current_methodology_only=current_methodology_only + ) total_calls = sum(m.call_count for m in model_stats) def _weighted_avg(attr: str) -> float: diff --git a/src/openjarvis/telemetry/instrumented_engine.py b/src/openjarvis/telemetry/instrumented_engine.py index 762849df..2df14634 100644 --- a/src/openjarvis/telemetry/instrumented_engine.py +++ b/src/openjarvis/telemetry/instrumented_engine.py @@ -8,7 +8,7 @@ from collections.abc import AsyncIterator from typing import Any, Dict, List, Optional, Sequence from openjarvis.core.events import EventBus, EventType -from openjarvis.core.types import Message, TelemetryRecord +from openjarvis.core.types import TOKEN_COUNTING_VERSION, Message, TelemetryRecord from openjarvis.engine._stubs import InferenceEngine, StreamChunk from openjarvis.telemetry.gpu_monitor import GpuSample @@ -233,6 +233,11 @@ class InstrumentedEngine(InferenceEngine): gpu_energy_joules=gpu_energy_joules, dram_energy_joules=dram_energy_joules, tokens_per_joule=tokens_per_joule, + # Stamp every new record with the current methodology version + # so the leaderboard aggregator can drop legacy rows (those + # left NULL by pre-fix builds) cleanly. Source of truth for + # the integer is `server/savings.py`. + token_counting_version=TOKEN_COUNTING_VERSION, ) event_data = { @@ -454,6 +459,8 @@ class InstrumentedEngine(InferenceEngine): gpu_energy_joules=gpu_energy_joules, dram_energy_joules=dram_energy_joules, tokens_per_joule=tokens_per_joule, + # Stamp the methodology version on streaming records too. + token_counting_version=TOKEN_COUNTING_VERSION, ) event_data = { diff --git a/src/openjarvis/telemetry/store.py b/src/openjarvis/telemetry/store.py index d88cf6fb..ab151b68 100644 --- a/src/openjarvis/telemetry/store.py +++ b/src/openjarvis/telemetry/store.py @@ -55,6 +55,11 @@ CREATE TABLE IF NOT EXISTS telemetry ( p99_itl_ms REAL NOT NULL DEFAULT 0.0, std_itl_ms REAL NOT NULL DEFAULT 0.0, is_streaming INTEGER NOT NULL DEFAULT 0, + -- token_counting_version: nullable on purpose. Records inserted by + -- pre-fix builds left this NULL; the leaderboard aggregator treats + -- NULL as legacy and excludes those rows from per-token efficiency + -- sums (so the bimodal-Wh/token leaderboard population disappears). + token_counting_version INTEGER, mining_session_id TEXT, metadata TEXT NOT NULL DEFAULT '{}' ); @@ -91,13 +96,14 @@ INSERT INTO telemetry ( prefill_energy_joules, decode_energy_joules, mean_itl_ms, median_itl_ms, p90_itl_ms, p95_itl_ms, p99_itl_ms, std_itl_ms, is_streaming, + token_counting_version, mining_session_id, metadata ) VALUES ( ?, ?, ?, ?, ?, ?, ?, ?, ?, ?, ?, ?, ?, ?, ?, ?, ?, ?, ?, ?, ?, ?, ?, ?, ?, ?, ?, ?, ?, ?, - ?, ?, ?, ?, ?, ?, ?, ?, ?, ? + ?, ?, ?, ?, ?, ?, ?, ?, ?, ?, ? ) """ @@ -128,6 +134,10 @@ _MIGRATE_COLUMNS = [ ("std_itl_ms", "REAL NOT NULL DEFAULT 0.0"), ("is_streaming", "INTEGER NOT NULL DEFAULT 0"), ("prompt_tokens_evaluated", "INTEGER NOT NULL DEFAULT 0"), + # `token_counting_version` is nullable on purpose — rows that existed + # before this migration ran predate per-record versioning and the + # aggregator filter treats them as legacy. + ("token_counting_version", "INTEGER"), ("mining_session_id", "TEXT"), ] @@ -197,6 +207,7 @@ class TelemetryStore: rec.p99_itl_ms, rec.std_itl_ms, 1 if rec.is_streaming else 0, + rec.token_counting_version, rec.mining_session_id, json.dumps(rec.metadata), ), diff --git a/tests/evals/test_use_case_benchmarks.py b/tests/evals/test_use_case_benchmarks.py index 8e06d727..e98d6184 100644 --- a/tests/evals/test_use_case_benchmarks.py +++ b/tests/evals/test_use_case_benchmarks.py @@ -424,11 +424,22 @@ class TestSavings: assert "total_calls" in d def test_energy_scales_linearly(self) -> None: - """Energy should scale linearly with evaluated tokens (KV-cache model).""" + """Energy should scale linearly with evaluated tokens (KV-cache model). + + The test must pass `prompt_tokens_evaluated` explicitly — under the + leaderboard-correctness fix, `compute_savings` no longer falls back + from a missing `prompt_tokens_evaluated` to the (multi-turn-inflated) + `prompt_tokens` sum. When the KV-cache-aware count is missing, FLOPs + derive from `completion_tokens` only. With completion=0, that path + produces zero energy and divides by zero. Passing the evaluated + count explicitly is the API contract that lets this invariant be + tested without depending on the (now intentionally conservative) + fallback behaviour. + """ from openjarvis.server.savings import compute_savings - s1 = compute_savings(1000, 0) - s10 = compute_savings(10000, 0) + s1 = compute_savings(1000, 0, prompt_tokens_evaluated=1000) + s10 = compute_savings(10000, 0, prompt_tokens_evaluated=10000) # FLOPs = 2*P*T (linear), so 10x tokens => 10x FLOPs => 10x energy for p1, p10 in zip(s1.per_provider, s10.per_provider): ratio = p10.energy_wh / p1.energy_wh @@ -437,10 +448,16 @@ class TestSavings: ) def test_energy_wh_matches_direct_formula(self) -> None: - """Energy must equal flops * wh_per_flop for known constants.""" + """Energy must equal flops * wh_per_flop for known constants. + + Pass `prompt_tokens_evaluated` explicitly for the same reason as + `test_energy_scales_linearly` — the conservative-fallback fix in + savings.py means an unset `prompt_tokens_evaluated` produces 0 + FLOPs and would make this test pass trivially with 0 == 0. + """ from openjarvis.server.savings import CLOUD_PRICING, compute_savings - summary = compute_savings(10000, 0) + summary = compute_savings(10000, 0, prompt_tokens_evaluated=10000) for p in summary.per_provider: pricing = CLOUD_PRICING[p.provider] wh_per_flop = pricing["energy_wh_per_1k_tokens"] / ( @@ -450,6 +467,12 @@ class TestSavings: assert abs(p.energy_wh - expected) < 1e-6, ( f"{p.provider}: energy_wh={p.energy_wh}, expected={expected}" ) + # Sanity: assert the formula isn't matching trivially with both + # sides equal to zero (would happen if FLOPs collapse to 0). + assert p.flops > 0, ( + f"{p.provider}: FLOPs must be positive when " + f"prompt_tokens_evaluated is explicitly set" + ) def test_energy_not_zero(self) -> None: """Energy must be positive for non-zero token counts.""" diff --git a/tests/server/test_routes.py b/tests/server/test_routes.py index 24d6bfda..123de8fb 100644 --- a/tests/server/test_routes.py +++ b/tests/server/test_routes.py @@ -250,6 +250,75 @@ class TestChatCompletions: # No tools → agent path → agent's content surfaces. assert data["choices"][0]["message"]["content"] == "Hello from agent" + def test_instrumented_engine_unwrapped_to_avoid_dual_telemetry(self): + """Regression for the leaderboard wonky-values bug. + + When `app.state.engine` is already an `InstrumentedEngine` (which is + the common case when the server was constructed with telemetry + wired in), `_handle_direct` MUST NOT wrap it again with + `instrumented_generate`. Both layers publish `TELEMETRY_RECORD` + events, so wrapping twice would double-count every call into the + leaderboard pipeline and inflate per-token energy / FLOPs metrics + by 2× on every request — the dominant contributor to the bimodal + Wh/token distribution on the public leaderboard. + + The fix unwraps the engine via `engine._inner` before passing it + to `instrumented_generate`. This test pins that contract. + """ + from openjarvis.core.events import EventBus, EventType + from openjarvis.telemetry.instrumented_engine import InstrumentedEngine + + # Build a fresh engine + bus and explicitly wrap with + # InstrumentedEngine (mirrors the production app construction). + inner_engine = _make_engine(content="Telemetry test") + bus = EventBus() + wrapped = InstrumentedEngine(inner_engine, bus=bus) + + received_records = [] + bus.subscribe( + EventType.TELEMETRY_RECORD, + lambda data: received_records.append(data), + ) + + app = create_app(wrapped, "test-model") + app.state.bus = bus + client = TestClient(app) + + resp = client.post( + "/v1/chat/completions", + json={ + "model": "test-model", + "messages": [{"role": "user", "content": "hello"}], + }, + ) + assert resp.status_code == 200 + + # Exactly ONE telemetry record — not two. Pre-fix this asserted 2. + assert len(received_records) == 1, ( + f"Expected exactly one TELEMETRY_RECORD event per request " + f"(got {len(received_records)}). When `app.state.engine` is " + f"already an InstrumentedEngine, routes.py must not also fire " + f"`instrumented_generate` — both layers publish and double " + f"the leaderboard's per-request counts." + ) + + # And the surviving record must be the InstrumentedEngine's + # FULL record (with token_counting_version stamped, ready for + # the leaderboard's current_methodology_only=True filter). + # If routes.py had instead unwrapped engine._inner and routed + # through the lightweight `instrumented_generate`, the record + # would carry no version stamp and `current_methodology_only` + # would drop it from leaderboard sums entirely. Pin that + # contract — see the adversarial review on PR #498. + from openjarvis.core.types import TOKEN_COUNTING_VERSION + + rec = received_records[0].data["record"] + assert rec.token_counting_version == TOKEN_COUNTING_VERSION, ( + "InstrumentedEngine path must stamp the methodology version " + "so the leaderboard's current-methodology filter accepts the " + "record." + ) + def test_agent_with_conversation(self, client_with_agent): resp = client_with_agent.post( "/v1/chat/completions", diff --git a/tests/server/test_savings.py b/tests/server/test_savings.py new file mode 100644 index 00000000..f6778f54 --- /dev/null +++ b/tests/server/test_savings.py @@ -0,0 +1,79 @@ +"""Regression tests for compute_savings — leaderboard correctness. + +The leaderboard pipeline feeds aggregated telemetry sums into +`compute_savings`, which in turn feeds the public leaderboard. The old +behaviour fell back from `prompt_tokens_evaluated` to `prompt_tokens` +when the KV-cache-aware count was missing — but routes.py aggregates +by summing per-turn full prompts, which counts the system prompt N +times in an N-turn conversation. The fallback was the dominant +contributor to the bimodal Wh/token distribution observed on the public +leaderboard. These tests pin the conservative fallback behaviour. +""" + +from __future__ import annotations + +from openjarvis.server.savings import compute_savings + + +class TestPromptTokensEvaluatedFallback: + def test_fallback_does_not_inflate_with_summed_prompt_tokens(self) -> None: + """When prompt_tokens_evaluated is 0 (missing), FLOPs must NOT + be derived from the full `prompt_tokens` sum. + + In a 10-turn conversation with a 100-token system prompt, the + aggregator sums prompt_tokens = 10 × (sys + history) ≈ 10× the + true input. Pre-fix the fallback used that inflated number for + FLOPs and energy. Post-fix we use only completion_tokens, which + is conservative but at least not 10× too high. + """ + # Same prompt_tokens (the buggy aggregated sum) for both + # invocations; vary only whether prompt_tokens_evaluated is + # known. The fix must make the FLOPs-bearing fields independent + # of the inflated prompt_tokens when evaluated is missing. + with_evaluated = compute_savings( + prompt_tokens=5000, + completion_tokens=200, + total_calls=10, + prompt_tokens_evaluated=600, # known KV-cache-aware count + ) + without_evaluated = compute_savings( + prompt_tokens=5000, + completion_tokens=200, + total_calls=10, + prompt_tokens_evaluated=0, # missing → fallback path + ) + + # The fallback path should NOT silently use the inflated + # prompt_tokens sum as the FLOPs denominator. + for missing_p, known_p in zip( + without_evaluated.per_provider, with_evaluated.per_provider + ): + assert missing_p.flops <= known_p.flops + 1, ( + f"Fallback inflated FLOPs for {missing_p.provider}: " + f"missing_evaluated={missing_p.flops}, " + f"known_evaluated={known_p.flops}. Pre-fix the " + f"fallback used `prompt_tokens` (the multi-turn-summed " + f"value) which over-stated compute by N× the turn count." + ) + + def test_dollar_savings_uses_prompt_tokens_unchanged(self) -> None: + """Dollar savings still uses prompt_tokens (cloud providers bill + per input token, even when the local engine had KV cache hits). + Pin the existing contract so the FLOPs fix doesn't accidentally + regress the dollar math.""" + result = compute_savings( + prompt_tokens=1_000_000, + completion_tokens=100_000, + total_calls=10, + prompt_tokens_evaluated=0, + ) + # Sanity: some provider produced a positive cost (= positive + # savings vs running locally for free). + assert any(p.total_cost > 0 for p in result.per_provider) + + def test_zero_tokens_returns_zero_savings(self) -> None: + """Edge case: no work done → no costs, no FLOPs, no negatives.""" + result = compute_savings(prompt_tokens=0, completion_tokens=0) + for p in result.per_provider: + assert p.total_cost == 0.0 + assert p.flops == 0.0 diff --git a/tests/telemetry/test_aggregator.py b/tests/telemetry/test_aggregator.py index ed3bb74a..e386854c 100644 --- a/tests/telemetry/test_aggregator.py +++ b/tests/telemetry/test_aggregator.py @@ -248,3 +248,61 @@ class TestDataclassDefaults: assert a.total_calls == 0 assert a.per_model == [] assert a.per_engine == [] + + +# --------------------------------------------------------------------------- +# Token-counting-version filter (leaderboard correctness) +# --------------------------------------------------------------------------- + + +class TestMethodologyFilter: + """When the aggregator is asked to honour the methodology version + (the leaderboard ingest path does), legacy rows that predate the + per-record version stamp must be excluded — they were the dominant + source of the bimodal Wh/token distribution on the public + leaderboard. Local dashboard callers leave the flag off so they + still see the full history.""" + + def test_default_includes_legacy_rows(self, tmp_path: Path) -> None: + from openjarvis.core.types import TOKEN_COUNTING_VERSION + + legacy = _make_record(model_id="m1") + legacy.token_counting_version = None # pre-fix row + current = _make_record(model_id="m1") + current.token_counting_version = TOKEN_COUNTING_VERSION + + agg = _setup(tmp_path, [legacy, current]) + stats = agg.per_model_stats() # default: include everything + assert len(stats) == 1 + assert stats[0].call_count == 2 + agg.close() + + def test_methodology_filter_drops_legacy_rows(self, tmp_path: Path) -> None: + from openjarvis.core.types import TOKEN_COUNTING_VERSION + + legacy = _make_record(model_id="m1") + legacy.token_counting_version = None + current = _make_record(model_id="m1") + current.token_counting_version = TOKEN_COUNTING_VERSION + + agg = _setup(tmp_path, [legacy, current]) + stats = agg.per_model_stats(current_methodology_only=True) + assert len(stats) == 1 + # Only the current-version row counts toward the leaderboard sum. + assert stats[0].call_count == 1 + agg.close() + + def test_methodology_filter_drops_legacy_in_summary(self, tmp_path: Path) -> None: + from openjarvis.core.types import TOKEN_COUNTING_VERSION + + legacy = _make_record(model_id="m1", completion_tokens=99) + legacy.token_counting_version = None + current = _make_record(model_id="m1", completion_tokens=7) + current.token_counting_version = TOKEN_COUNTING_VERSION + + agg = _setup(tmp_path, [legacy, current]) + summary = agg.summary(current_methodology_only=True) + # 99-token legacy row excluded; only the 7-completion-token current + # row contributes to total_tokens. + assert sum(m.completion_tokens for m in summary.per_model) == 7 + agg.close()