mirror of
https://github.com/open-jarvis/OpenJarvis.git
synced 2026-07-28 14:07:55 +00:00
28ef745384
* 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>