Skip to content

fix(benchmark): Ollama-backed QA harness + correct Recall@k scoring, retract prior figures - #178

Merged
salishforge merged 2 commits into
mainfrom
feat/benchmark-ollama-qa
Jul 28, 2026
Merged

fix(benchmark): Ollama-backed QA harness + correct Recall@k scoring, retract prior figures#178
salishforge merged 2 commits into
mainfrom
feat/benchmark-ollama-qa

Conversation

@salishforge

Copy link
Copy Markdown
Owner

Two related pieces of benchmark work. Refs #47.

1. QA harness runs on local Ollama (qwen3.5:cloud)

BENCHMARK_PLAN.md already documented an Ollama path, but the code hardcoded api.openai.com and threw without OPENAI_API_KEY. Now real, and the default: BENCHMARK_LIMIT=10 npm run benchmark:longmemeval-qa costs $0 and needs no key. New benchmarks/lib/llm.ts is 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 records judgeModel + 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.ts declared allSessions inside the per-instance loop and read it after (a runtime ReferenceError), and the scripts referenced config.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.ts called recallAtK(ids, answers, ids.length). Passing the array's own length as k meant slice(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@5 actually meant "a gold session appeared anywhere among all sessions inside the top 5 rows."

The surrounding comment shows k was disabled deliberately — the author treated k as rows, not sessions. That's a definitional divergence from LongMemEval, so the fix is definitional:

Metric Meaning Use
recallAtKSessions gold session among first k distinct sessions by rank LongMemEval's definition — headline
recallAtKRows gold session anywhere in top-k rows MemForge-native, strictly ≥ sessions

Both are reported with sessionsPerRow, so the packing advantage is visible rather than baked into one number. With CONSOLIDATION_INNER_BATCH_SIZE=1 they 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 returns 1 under the old implementation and 0 under 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.

Gate Result
benchmarks tsc 0 errors (was 21)
root type-check / lint clean
test:benchmark-metrics 17/17
test:http / integration / epistemic-confidence 43 / 24 / 33
Ollama judge + reader smoke verified against live daemon

🤖 Generated with Claude Code

https://claude.ai/code/session_011xCqQo49d3CEbn6oEvb3Ru

salishforge and others added 2 commits July 28, 2026 00:05
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
@salishforge
salishforge merged commit 63d3e58 into main Jul 28, 2026
14 checks passed
@salishforge
salishforge deleted the feat/benchmark-ollama-qa branch July 28, 2026 00:20
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant