mirror of
https://github.com/open-jarvis/OpenJarvis.git
synced 2026-07-28 14:07:55 +00:00
* 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 <noreply@anthropic.com> * 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 <noreply@anthropic.com> --------- Co-authored-by: krypticmouse <herumbshandilya123@gmail.com> Co-authored-by: Claude Opus 4.7 <noreply@anthropic.com>
80 lines
3.6 KiB
Python
80 lines
3.6 KiB
Python
"""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
|