fix(benchmark): Ollama-backed QA harness + correct Recall@k scoring, retract prior figures - #178
Merged
Merged
Conversation
BENCHMARK_PLAN.md already documented an Ollama path (QA_JUDGE_MODEL, QA_READER_MODEL, OLLAMA_BASE_URL) but the code could not honor it: both judgeAnswer() and generateAnswer() hardcoded api.openai.com and threw without OPENAI_API_KEY. Makes the documented contract real and the default, so a run costs nothing and needs no key. Adds benchmarks/lib/llm.ts as the single chat transport for both LLM roles. Which provider serves them is a benchmark-integrity decision, so keeping it in one place stops judge and reader silently drifting onto different backends and lets the resolved endpoint be reported. Any OpenAI-compatible endpoint works — switching to OpenAI is a base-URL change, not a code change. Comparability is enforced rather than trusted. Only a gpt-4o* judge follows the paper protocol (>97% human agreement); with any other judge the runner prints a warning, the generated report replaces "directly comparable to leaderboard entries" with an explicit not-comparable notice, and every manifest records judgeModel + paperProtocolJudge so a number cannot be quoted later without the judge that produced it. Previously that comparability claim was emitted unconditionally, so switching the default would have auto-generated false credibility. Also fixes 21 pre-existing type errors that made this harness uncompilable despite being marked "Implemented" in the plan — it cannot ever have run. Notably ingest.ts declared allSessions inside the per-instance loop and read it after the loop (ReferenceError at runtime), and the QA scripts referenced config.limit/offset/cleanup which do not exist on BenchmarkConfig (questionLimit/questionOffset/ cleanupAfter do). Adds a request timeout: there was none, so one stalled call could hang a 500-question run. Verified: qwen3.5:cloud judge returns valid JSON via Ollama's /v1 shim and the reader answers from context; QA_API_BASE override resolves to OpenAI and flips the paper-protocol flag back on; benchmarks tsc 0 errors; root type-check and lint clean. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_011xCqQo49d3CEbn6oEvb3Ru
The published "93.2% R@5" was never R@5. evaluate.ts called recallAtK(ids, answers, ids.length) — passing the candidate list's own length as k, so the internal slice(0, k) never truncated and R@1, R@3, R@5 and R@10 were all computed over the entire retrieved set. Issue #47 diagnosed this in April; it was never fixed. The inflation is not marginal. Consolidation packs many sessions into each warm-tier row, so "the top 5 rows" can hold hundreds of sessions. The published figure actually meant "a gold session appeared anywhere among all sessions inside the top 5 rows". The k argument was disabled deliberately — the surrounding comment shows the author treating k as a count of rows rather than sessions. That is a definitional divergence from LongMemEval, not a typo, so the fix is definitional too. metrics.ts now computes two clearly named numbers: recallAtKSessions — gold session among the first k distinct sessions by rank. LongMemEval's definition; the headline. recallAtKRows — gold session anywhere inside the top-k rows. MemForge's native behaviour, strictly >= the session figure. Both are reported alongside sessionsPerRow, so the packing advantage is visible instead of silently inflating a single number. Running with CONSOLIDATION_INNER_BATCH_SIZE=1 makes rows and sessions 1:1 and the two converge — asserted in the tests. Adds tests/benchmark-metrics.test.ts (17 tests). The scorer had none, which is precisely how a wrong headline number shipped and survived. The central guard — gold session at rank 6 must miss at k=3 — returns 1 under the old implementation and 0 under the new one, verified directly. Also replaces exec-with-lastIndex-reset in extractSessionIds with matchAll: the shared /g regex carried mutable state between calls. Retracts the figure everywhere it was published — README badge, status line and table, ROADMAP, benchmarks/README, funding.json (crawled by funding platforms), and RESULTS.md, which now carries a full explanation of what the number actually measured. Suspends the PHASE_5_PLAN CI regression gate that enforced 93.2%, since a gate defending a retracted number is worse than none. Figures return with the corrected re-run. Refs #47 Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_011xCqQo49d3CEbn6oEvb3Ru
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Two related pieces of benchmark work. Refs #47.
1. QA harness runs on local Ollama (
qwen3.5:cloud)BENCHMARK_PLAN.mdalready documented an Ollama path, but the code hardcodedapi.openai.comand threw withoutOPENAI_API_KEY. Now real, and the default:BENCHMARK_LIMIT=10 npm run benchmark:longmemeval-qacosts $0 and needs no key. Newbenchmarks/lib/llm.tsis the single transport for both LLM roles; switching to OpenAI is a base-URL change.Comparability is enforced, not trusted. Only a
gpt-4o*judge follows the paper protocol. With any other judge the runner warns, the report replaces "directly comparable to leaderboard entries" with an explicit not-comparable notice, and every manifest recordsjudgeModel+paperProtocolJudge. Previously that claim was emitted unconditionally — switching the default would have auto-generated false credibility.Also fixes 21 pre-existing type errors that made the QA harness uncompilable despite being marked "✅ Implemented" — it cannot ever have run. Notably
ingest.tsdeclaredallSessionsinside the per-instance loop and read it after (a runtimeReferenceError), and the scripts referencedconfig.limit/offset/cleanup, which don't exist. Adds a request timeout; there was none, so one stalled call could hang a 500-question run.2. Recall@k was miscomputed — figures retracted
evaluate.tscalledrecallAtK(ids, answers, ids.length). Passing the array's own length askmeantslice(0, k)never truncated: R@1, R@3, R@5 and R@10 were all the same number, computed over the entire retrieved set.Not marginal — consolidation packs many sessions per row, so "top 5 rows" can hold hundreds of sessions. The published
93.2% R@5actually meant "a gold session appeared anywhere among all sessions inside the top 5 rows."The surrounding comment shows
kwas disabled deliberately — the author treatedkas rows, not sessions. That's a definitional divergence from LongMemEval, so the fix is definitional:recallAtKSessionsrecallAtKRowsBoth are reported with
sessionsPerRow, so the packing advantage is visible rather than baked into one number. WithCONSOLIDATION_INNER_BATCH_SIZE=1they converge — asserted in tests.Tests where there were none
tests/benchmark-metrics.test.ts, 17 tests. The scorer had zero coverage, which is how a wrong headline number shipped and survived. The central guard — gold at rank 6 must miss at k=3 — I verified returns1under the old implementation and0under the new.Retraction
Removed from the README badge/status/table, ROADMAP, benchmarks/README,
funding.json(crawled by funding platforms), and RESULTS.md, which now explains exactly what the number measured. The PHASE_5_PLAN CI gate enforcing 93.2% is suspended — a gate defending a retracted number is worse than no gate. Figures return with the corrected re-run.test:benchmark-metricstest:http/integration/epistemic-confidence🤖 Generated with Claude Code
https://claude.ai/code/session_011xCqQo49d3CEbn6oEvb3Ru