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()