fix(benchmarks): make question order deterministic and load() idempotent - #87
Open
Agnik47 wants to merge 1 commit into
Open
fix(benchmarks): make question order deterministic and load() idempotent#87Agnik47 wants to merge 1 commit into
Agnik47 wants to merge 1 commit into
Conversation
`LongMemEvalBenchmark.loadQuestions` took whatever order `readdirSync` returned. That order is unspecified — hash-ordered on ext4 with dir_index, roughly lexicographic on APFS/NTFS — and it decides which questions a run covers, because the orchestrator slices the front of the list for `--limit N`, for `sampling.mode === "limit"`, and for consecutive sampling. The same command therefore benchmarked a different subset on a maintainer's Mac than on CI, and the accuracy gap between the two was indistinguishable from a real provider difference. Sorting the file list makes a limited run cover the same questions everywhere. `load()` was also not idempotent: all three benchmarks pushed into `this.questions` (and LongMemEval into `this.data`) without clearing first, so calling it twice on one instance duplicated every question. `getQuestions()` would return each one twice, double-counting in the report and halving the effective coverage of `--limit`. The CLI creates a fresh instance per run, but the server caches benchmark instances in `routes/runs.ts`, so the footgun is reachable. Reset the accumulated state at the start of the load — for ConvoMem after the JSON parses, so a failed reload does not discard good data. Fixes supermemoryai#75 Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01Nwg8d2HEScsHTWpBgWewWX
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.
Fixes #75.
Problem
Question order is whatever the filesystem hands back
readdirSyncorder is unspecified: hash-ordered on ext4 withdir_index, roughly but not reliably lexicographic on APFS/NTFS, and liable to change when files are rewritten. That order then decides which questions a limited run covers — the orchestrator slices the front of the list for--limit N(src/orchestrator/index.ts:227), forsampling.mode === "limit", and for consecutive sampling.So
run -p supermemory -b locomo --limit 50on a maintainer's Mac and on CI's Linux box benchmark different sets of 50 questions, and the accuracy difference between them is indistinguishable from a real provider difference. For a benchmark whose published numbers are meant to be independently reproducible, that is the whole point undone.load()is not idempotentAll three benchmarks push into
this.questions(and LongMemEval intothis.data) without clearing first, andsessionsMap.setoverwrites. Callingload()twice on one instance duplicates every question, sogetQuestions()returns each one twice — double-counting in the report and halving the effective coverage of--limit.The CLI path builds a fresh instance per run, but the server caches benchmark instances (
src/server/routes/runs.ts:16-24), so this is reachable rather than theoretical.Fix
.sort()the question file list inLongMemEvalBenchmark.loadQuestions, so a limited or sampled run covers the same questions on every machine. LoCoMo and ConvoMem read a single JSON file, so their order was already deterministic.data/questions/sessionsMapat the start of the load in all three benchmarks. For ConvoMem the reset happens afterJSON.parsesucceeds, so a failed reload doesn't discard data that was already loaded.No public API change, and a fresh single
load()behaves exactly as before.Tests
New
src/benchmarks/question-order.test.ts(7 tests,bun test). It mocksfsto serve a deliberately unsorted directory listing —["q3.json", "q1.json", "q10.json", "q2.json"], i.e. what ext4 hands back — so the ordering assertion is deterministic on every platform rather than depending on the developer's filesystem. The realfsis captured up front and restored inafterAll, so the mock doesn't leak into other test files.--limit 2slice selects the same pair on any filesystem (it would have been a different pair pre-fix)5 of the 7 fail on
main; all 7 pass with this change.Left out
The issue also suggests recording the resolved
targetQuestionIdsin the report. That means touchingsrc/orchestrator/phases/report.ts, which #80 is currently rewriting, so I left it out to keep this reviewable and conflict-free. Happy to add it separately once #80 lands.bunx tsc --noEmitis clean and the new file is Prettier-clean. The three touched files already failbun run format:checkonmain(along with ~69 others), so I left their pre-existing formatting alone to keep the diff scoped.