bench(eventqa): route the EventQA runner through retrieve() so graph_expansion is measurable (unblocks E-025) - #196
Conversation
The EventQA runner built its generator context by hand: ContextAssembler, archive.search_fts per keyword, vmem.search, then an optional reranker. It never called taosmd.retrieval.retrieve(), so retrieval controls applied inside retrieve() had no effect on this benchmark. E-025 (graph-expansion fact readback) was pre-registered against EventQA and was blocked by that: both arms would have produced byte-identical context. The vector stage now goes through retrieve() with strategy="custom" and memory_layers=["vector"], carrying the KG in sources so the fact-readback stage can fire without the KG competing for the limit slots. candidate_top_k pins the vector fetch to RETRIEVE_LIMIT and limit narrows to RERANK_TOP_K when reranking is on, which reproduces the stage it replaces. New flags: --graph-expansion N (0 = off, the E-025 lever), --retrieval-path retrieve|legacy (legacy restores the pre-wiring stage for re-anchoring), and --report-retrieval-delta (measures wired-vs-legacy context divergence per question rather than assuming the E-016 anchor carries over). graph_expansion is only forwarded when non-zero, so the runner still imports and runs on a checkout whose retrieve() predates the control; asking for a non-zero value there is rejected up front instead of silently producing two identical arms. No shipped retrieval code or defaults changed.
Qodo reviews are paused for this user.Troubleshooting steps vary by plan Learn more → On a Teams plan? Using GitHub Enterprise Server, GitLab Self-Managed, or Bitbucket Data Center? |
|
Warning Review limit reached
Next review available in: 48 minutes Enable usage-based reviews in Billing to review now. Otherwise, wait until the next included review is available. How can I continue?After more reviews become available, a review can be triggered using the To avoid repeated limits, reduce automatic review volume by pausing incremental auto-reviews earlier, using label-based review opt-in, excluding WIP or generated PR titles, or requesting reviews manually when the PR is ready. If your team needs uninterrupted high-volume reviews, an organization admin can enable usage-based reviews. How do review limits work?CodeRabbit enforces per-developer PR review limits for each organization. Most developers receive the normal plan review availability. For paid Pro and Pro+ PR reviews, CodeRabbit uses adaptive limits for sustained high-volume activity. When a developer's recent PR review activity reaches the 95th percentile or higher among CodeRabbit users, additional reviews become available more gradually as earlier reviews age out of the rolling window. Please refer docs for additional details. Review details⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (2)
✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
1 participant
HELD FOR REVIEW. Do not merge without sign-off. This is benchmark-harness plumbing only; no shipped retrieval code or defaults change.
The blocker
E-025 (graph-expansion fact readback, the
graph_expansioncontrol added by #191) is pre-registered against MemoryAgentBench EventQA. It could not run.benchmarks/eventqa_runner.pybuilt its generator context by hand:It never called
taosmd.retrieval.retrieve().graph_expansionis applied insideretrieve()(_append_graph_expansion), so setting the control had exactly zero effect on this benchmark and the two E-025 arms would have been byte-identical. Any measured difference would have been pure noise.What changed
benchmarks/eventqa_runner.py:retrieve_vector_hits()(new) — the vector stage now goes throughtaosmd.retrieval.retrieve(). Parameterised to reproduce the hand-rolled stage it replaces:strategy="custom",memory_layers=["vector"]— only the vector layer is searched, as before.sources={"vector": vmem, "kg": kg}— the KG is passed but not searched._append_graph_expansionreadssources["kg"]regardless of which layers were queried, so the fact-readback block can fire without KG hits competing for thelimitslots.candidate_top_k=RETRIEVE_LIMIT— pins the vector fetch to the same pool size the old code used (retrieve()would otherwise fetchlimit * 3).limit=RERANK_TOP_Kwhen a reranker is available, elseRETRIEVE_LIMIT— matches the old narrow-after-rerank behaviour.retrieve_context()— gainsgraph_expansionandretrieval_path. The ContextAssembler block and the per-keyword FTS block are untouched; only the vector stage was rewired.retrieve_supports_graph_expansion()(new) — signature probe.graph_expansionis forwarded toretrieve()only when non-zero, so the runner still imports and runs on a checkout predating feat(retrieval): graph-expansion fact-readback (bi-temporal Phase 1, default-off) #191. Asking for a non-zero value on such a checkout is rejected up front with a clear message rather than silently yielding two identical arms — the exact failure mode this PR exists to remove.--graph-expansion N(default 0),--retrieval-path retrieve|legacy(defaultretrieve;legacyrestores the pre-wiring stage byte for byte),--report-retrieval-delta(builds both contexts per question and reports divergence).retrieval_path,graph_expansion,num_ctx, and theretrieval_deltasummary.tests/test_eventqa_retrieve_wiring.py(new, 9 tests): the vector stage callsretrieve(); the KG is insources;graph_expansionis forwarded only when set; the derived block reaches the assembled context and the two arms differ; the legacy path raises rather than ignoring the control; the wired default matches the legacy context and fetches the same pool; CLI surface.Does the default path still match the E-016 anchor?
Yes, byte-identical in the verified case. Evidence, running the real
retrieve()(not a mock) against a stub vector store of 20 distinct passages:graph_expansionsupportmasterfeat/bitemporal-readback(#191)And on #191 with
--graph-expansion 256the control demonstrably fires:So at
graph_expansion=0the wiring is a no-op on the assembled context, and the E-016 ~75% anchor (num_ctx=8192) carries over unchanged.The one residual difference, stated plainly
retrieve()runs a near-duplicate filter (_deduplicate, Jaccard >= 0.8 over hit texts); the old code deduplicated on exact string equality only. I reproduced this deliberately: with a stub store of 20 chunks differing only by an index number, the wired context collapsed to 65 chars against the legacy 806. That is not a bug, it isretrieve()doing its job, but it means the anchor is only guaranteed where retrieved chunks are not >=80% word-overlapping.EventQA chunks are ~4096-token distinct novel passages, so this should not fire. It should not be assumed.
--report-retrieval-deltameasures it directly on the real data, and the recommended first run below uses it. If that reports anything below 100% byte-identical, E-016 needs re-anchoring before E-025 is interpreted.The EventQA dataset is gitignored and is not present on this machine, so the wiring was verified statically and against stub stores only. The delta check on real data has to happen on the Fedora bench host.
Commands for the bench host
Run the anchor check first, on one context. If
byte-identical contextsis not 100%, stop and re-anchor E-016 before running the arms.# 0) anchor check on real EventQA data (no LLM needed) python3 benchmarks/eventqa_runner.py \ --tier eventqa_65536 --contexts 1 --limit 25 --report-retrieval-deltaThen the two E-025 arms.
num_ctx=8192is mandatory per E-016: the Ollama default of 4096 truncates EventQA prompts and the model emits a stub instead of an answer.Both arms must run on a checkout that has #191 (arm B errors out otherwise). Same host, same model, one at a time — concurrent Ollama runs on the shared 3060 starve each other into timeouts.
Suite
1119 passed(python3 -m pytest -q), including the 9 new tests.