feat(bench-gate): #433 vocab_bridge NDCG@k uplift driver — evidence: bridge regresses on labelled corpus - #535
Conversation
📝 WalkthroughWalkthroughThis PR introduces a new bench-gate uplift runner for Vocabulary Bridge ( ChangesVocab Bridge Uplift Runner
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Possibly related PRs
Suggested labels
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 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 |
Reviewer's GuideImplements a strict NDCG@k uplift driver for the vocab bridge bench gate (#433), including a new runner that evaluates retrieval with and without the vocab bridge, supporting seeding helpers and result typing, plus unit tests and a bench-gated ship‑gate test that asserts strictly positive uplift when labelled corpus rows are present. File-Level Changes
Tips and commandsInteracting with Sourcery
Customizing Your ExperienceAccess your dashboard to:
Getting Help
|
There was a problem hiding this comment.
Hey - I've left some high level feedback:
- _seed_store_for_vocab_bridge duplicates much of the belief/edge seeding logic from the existing vocab-bridge precondition seeder; consider factoring this into a shared helper so the store shape stays consistent if the schema or defaults change.
- _BENCH_TS is hard-coded to a specific future date; if this is only meant to be a stable, non-semantic timestamp you might prefer a more neutral constant (e.g. an epoch-like value or clearly fake date) and a short comment so it’s not confused with real chronology.
Prompt for AI Agents
Please address the comments from this code review:
## Overall Comments
- _seed_store_for_vocab_bridge duplicates much of the belief/edge seeding logic from the existing vocab-bridge precondition seeder; consider factoring this into a shared helper so the store shape stays consistent if the schema or defaults change.
- _BENCH_TS is hard-coded to a specific future date; if this is only meant to be a stable, non-semantic timestamp you might prefer a more neutral constant (e.g. an epoch-like value or clearly fake date) and a short comment so it’s not confused with real chronology.Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
|
This PR is now behind Auto-rebase was removed because the bot has no signing key; rebasing as the bot strips author signatures and the |
|
[claim:review:Noether:2026-05-10T03:29:06Z] |
Mirrors run_doc_linker_uplift / run_query_strategy_uplift / run_clustering_uplift:
per row, runs retrieve_v2 twice on fresh stores seeded from the row's
store_beliefs (with anchors), once with use_vocab_bridge=False (baseline)
and once with =True. NDCG@k against expected_top_k, averaged across rows.
Result type is VocabBridgeUplift{n_rows, mean_ndcg_off, mean_ndcg_on};
uplift property mirrors the other module result types so the bench-gate
failure formatter doesn't need per-runner branching.
Adds _seed_store_for_vocab_bridge to translate the existing precondition
row shape (store_beliefs[i].anchors as inbound CITES edges) into store
state — same logic as the precondition gate's local seeder, hoisted into
the runner so the OFF/ON arms see byte-identical store contents and any
NDCG delta is attributable to the bridge alone.
Rows missing expected_top_k are skipped so precondition-only rows
(those carrying just expected_canonicals) don't dilute the mean.
Three tests mirroring the doc_linker / query_strategy harness coverage: - empty_input: zero rows -> zero metrics; the contract is total silence on a no-op call, not a divide-by-zero. - skips_rows_without_expected_top_k: a precondition-only row (expected_canonicals but no expected_top_k) must not be counted in n_rows. Falsifiable if the runner silently treats missing ground truth as ndcg=0 and dilutes the mean. - runs_on_synthetic_row: shape + bounded-metric smoke. The contract under test is that OFF/ON arms execute end-to-end and produce in-range NDCG values; whether the bridge actually wins on a hand-crafted two-belief store is the lab-side gate.
Adds test_vocab_bridge_ship_gate_runner_present, mirroring the #436 intentional-clustering / #435 doc-linker pattern: skip-if-runner-absent plus skip-if-no-row-has-expected_top_k, so the gate fires only when both the harness and labelled rows are present. Operator-side gate for flipping use_vocab_bridge to default-on once lab evidence clears. Module docstring updated to document the extended row schema: expected_top_k joins expected_canonicals as an optional field; the precondition gate reads expected_canonicals, the strict gate reads expected_top_k. Rows can carry either or both. No code change to the precondition gate — its 50% appends-at-least-one threshold remains the regression tripwire that fires before the heavier NDCG run.
0d57ff3 to
37e0764
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@tests/bench_gate/test_vocab_bridge_uplift.py`:
- Around line 155-157: The test currently hard-fails when
load_corpus_module(aelfrice_corpus_root, "vocab_bridge") returns no rows; change
that behavior to skip the test instead. Replace the assert rows check with a
conditional that calls pytest.skip(...) with a clear reason when rows is empty
or falsy (and add an import for pytest if not already present). Keep the rest of
the test using the loaded rows when present.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 0418c668-e1e8-449c-bba2-8d0edb842a8f
📒 Files selected for processing (3)
tests/bench_gate/test_vocab_bridge_uplift.pytests/retrieve_uplift_runner.pytests/test_retrieve_uplift_runner.py
| rows = load_corpus_module(aelfrice_corpus_root, "vocab_bridge") | ||
| assert rows, "vocab_bridge corpus produced zero rows" | ||
|
|
There was a problem hiding this comment.
Skip on empty corpus instead of failing the bench gate
Line 156 hard-fails when the corpus is absent/empty, which makes this gate brittle in environments without lab resources. This should skip like the other absent-resource paths.
Proposed fix
rows = load_corpus_module(aelfrice_corpus_root, "vocab_bridge")
- assert rows, "vocab_bridge corpus produced zero rows"
+ if not rows:
+ pytest.skip(
+ "vocab_bridge corpus produced zero rows; "
+ "add rows under tests/corpus/v2_0/vocab_bridge/*.jsonl "
+ "before enabling this ship gate"
+ )📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| rows = load_corpus_module(aelfrice_corpus_root, "vocab_bridge") | |
| assert rows, "vocab_bridge corpus produced zero rows" | |
| rows = load_corpus_module(aelfrice_corpus_root, "vocab_bridge") | |
| if not rows: | |
| pytest.skip( | |
| "vocab_bridge corpus produced zero rows; " | |
| "add rows under tests/corpus/v2_0/vocab_bridge/*.jsonl " | |
| "before enabling this ship gate" | |
| ) | |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@tests/bench_gate/test_vocab_bridge_uplift.py` around lines 155 - 157, The
test currently hard-fails when load_corpus_module(aelfrice_corpus_root,
"vocab_bridge") returns no rows; change that behavior to skip the test instead.
Replace the assert rows check with a conditional that calls pytest.skip(...)
with a clear reason when rows is empty or falsy (and add an import for pytest if
not already present). Keep the rest of the test using the loaded rows when
present.
robotrocketscience
left a comment
There was a problem hiding this comment.
LGTM — rebased and FF-merging.
Verified
- Rebased onto current
github/main(3 atomic commits re-signed, all G). - Local pytest on rebased tree:
tests/test_retrieve_uplift_runner.py tests/bench_gate/test_vocab_bridge_uplift.py→ 20 passed, 2 skipped (the bench-gate skips on public CI withoutAELFRICE_CORPUS_ROOT, as designed — same skip-policy contract as#291query-strategy and#436clustering gates). - Discretion grep on diff: clean.
- Runner shape mirrors
run_doc_linker_uplift/run_query_strategy_uplift—mean_ndcg_off/mean_ndcg_on/upliftfield names preserve the branch-free formatter. expected_top_krow schema is a clean superset of the precondition row schema (expected_canonicalsstays); rows can carry one or both, and the runner skips ones without ranking ground truth so they don't dilute the mean.- Bench-gate test correctly assertion-pairs:
pytest.importorskipon the runner module + skip when no row carriesexpected_top_k+assert results.uplift > 0for the strict gate.
Substrate diagnosis spot-checked
Walked src/aelfrice/vocab_bridge.py:_harvest() on github/main HEAD. The PR body's claim is exactly what's there: every observed surface form is added as its own canonical (vocab.add(low, low) at line 223 for entities, vocab.add(tok, tok) at line 243 for BM25 tokens). There is no surface→canonical mapping logic in the harvest. The HRR superposition built downstream is therefore bridge_vec ≈ Σ_t bind(t, t) — self-bound tokens plus noise — and the rewriter's deduplication step (vocab_bridge.py:310-311) drops any token that's already in the query, so the rewrite path emits noise rather than canonicals. The "vocabulary-gap-recovery" claim presupposes a structure the shipped harvest doesn't build.
Per-row "appends ≥1 expected canonical" failing 0/20 plus strict NDCG@k uplift = -0.0240 is consistent with that diagnosis: the bridge isn't merely a no-op, it actively perturbs the query with low-amplitude noise, dropping retrieval slightly.
Recommendation
Merge as evidence + harness. Keep #433 open; #536 (re-spec _harvest) is the right follow-up. Do not flip use_vocab_bridge to default-on at any point on the current substrate — the bench gate scaffolded here will fire correctly the moment a future substrate change re-enables the lab corpus runner, so the gate is its own forward-protection.
(Cross-session same-account auth blocks formal --approve; merging by local FF push per protocol.)
|
[release:review:Noether:2026-05-10T03:35:47Z] |
vocab_bridge.py is being removed: the lab campaign exp/hrr-vocabulary-bridge adopted typed-edge structural retrieval (shipped via hrr_index.py + use_hrr_structural default-on) and explicitly DROPPED the cascade form that vocab_bridge.py implemented (R2: recall@K bottleneck). PR #535's bench evidence (-0.024 NDCG, 0/20 precondition rows) is consistent with that lab finding. This commit removes the unit + integration tests; module + wiring + docs follow in subsequent commits. After this commit no test references vocab_bridge.py except the run_vocab_bridge_uplift driver in tests/retrieve_uplift_runner.py and its unit tests, which the next commit handles.
The lab campaign exp/hrr-vocabulary-bridge concluded that the vocabulary-gap-recovery claim is closed by the typed-edge structural-retrieval lane (R5 reframe), which ships separately as src/aelfrice/hrr_index.py + the use_hrr_structural lane in retrieve_v2. The R2 finding explicitly DROPPED the cascade form that this module's query-rewrite mechanism implemented (recall@K bottleneck for the bridge use case). PR #535's bench evidence (-0.024 NDCG, 0/20 precondition rows on a 20-row labelled corpus) confirmed the substrate doesn't work in production. Removing the module rather than leaving default-OFF dead code keeps one source of truth for the vocab-gap closure. Module wiring + tests + uplift driver were removed in earlier commits; this drops the file. Docs updates follow.
vocab_bridge.py is being removed: the lab campaign exp/hrr-vocabulary-bridge adopted typed-edge structural retrieval (shipped via hrr_index.py + use_hrr_structural default-on) and explicitly DROPPED the cascade form that vocab_bridge.py implemented (R2: recall@K bottleneck). PR #535's bench evidence (-0.024 NDCG, 0/20 precondition rows) is consistent with that lab finding. This commit removes the unit + integration tests; module + wiring + docs follow in subsequent commits. After this commit no test references vocab_bridge.py except the run_vocab_bridge_uplift driver in tests/retrieve_uplift_runner.py and its unit tests, which the next commit handles.
The lab campaign exp/hrr-vocabulary-bridge concluded that the vocabulary-gap-recovery claim is closed by the typed-edge structural-retrieval lane (R5 reframe), which ships separately as src/aelfrice/hrr_index.py + the use_hrr_structural lane in retrieve_v2. The R2 finding explicitly DROPPED the cascade form that this module's query-rewrite mechanism implemented (recall@K bottleneck for the bridge use case). PR #535's bench evidence (-0.024 NDCG, 0/20 precondition rows on a 20-row labelled corpus) confirmed the substrate doesn't work in production. Removing the module rather than leaving default-OFF dead code keeps one source of truth for the vocab-gap closure. Module wiring + tests + uplift driver were removed in earlier commits; this drops the file. Docs updates follow.
Removing src/aelfrice/vocab_bridge.py also dropped the use_hrr alias on retrieve_v2 (no **kwargs catch-all). The seven academic-suite bench adapters still passed use_hrr=True and would TypeError on first call; benchmarks/README.md still documented the old call shape. Fix is to drop the kwarg entirely — the structural HRR lane (use_hrr_structural) is default-on since v2.1, which is the behavior these adapters wanted. Pytest doesn't exercise benchmarks/, so this only surfaces when the bench suite is run; same gap PR #535's precondition-row check found.
vocab_bridge.py is being removed: the lab campaign exp/hrr-vocabulary-bridge adopted typed-edge structural retrieval (shipped via hrr_index.py + use_hrr_structural default-on) and explicitly DROPPED the cascade form that vocab_bridge.py implemented (R2: recall@K bottleneck). PR #535's bench evidence (-0.024 NDCG, 0/20 precondition rows) is consistent with that lab finding. This commit removes the unit + integration tests; module + wiring + docs follow in subsequent commits. After this commit no test references vocab_bridge.py except the run_vocab_bridge_uplift driver in tests/retrieve_uplift_runner.py and its unit tests, which the next commit handles.
The lab campaign exp/hrr-vocabulary-bridge concluded that the vocabulary-gap-recovery claim is closed by the typed-edge structural-retrieval lane (R5 reframe), which ships separately as src/aelfrice/hrr_index.py + the use_hrr_structural lane in retrieve_v2. The R2 finding explicitly DROPPED the cascade form that this module's query-rewrite mechanism implemented (recall@K bottleneck for the bridge use case). PR #535's bench evidence (-0.024 NDCG, 0/20 precondition rows on a 20-row labelled corpus) confirmed the substrate doesn't work in production. Removing the module rather than leaving default-OFF dead code keeps one source of truth for the vocab-gap closure. Module wiring + tests + uplift driver were removed in earlier commits; this drops the file. Docs updates follow.
Removing src/aelfrice/vocab_bridge.py also dropped the use_hrr alias on retrieve_v2 (no **kwargs catch-all). The seven academic-suite bench adapters still passed use_hrr=True and would TypeError on first call; benchmarks/README.md still documented the old call shape. Fix is to drop the kwarg entirely — the structural HRR lane (use_hrr_structural) is default-on since v2.1, which is the behavior these adapters wanted. Pytest doesn't exercise benchmarks/, so this only surfaces when the bench suite is run; same gap PR #535's precondition-row check found.
vocab_bridge.py is being removed: the lab campaign exp/hrr-vocabulary-bridge adopted typed-edge structural retrieval (shipped via hrr_index.py + use_hrr_structural default-on) and explicitly DROPPED the cascade form that vocab_bridge.py implemented (R2: recall@K bottleneck). PR #535's bench evidence (-0.024 NDCG, 0/20 precondition rows) is consistent with that lab finding. This commit removes the unit + integration tests; module + wiring + docs follow in subsequent commits. After this commit no test references vocab_bridge.py except the run_vocab_bridge_uplift driver in tests/retrieve_uplift_runner.py and its unit tests, which the next commit handles.
The lab campaign exp/hrr-vocabulary-bridge concluded that the vocabulary-gap-recovery claim is closed by the typed-edge structural-retrieval lane (R5 reframe), which ships separately as src/aelfrice/hrr_index.py + the use_hrr_structural lane in retrieve_v2. The R2 finding explicitly DROPPED the cascade form that this module's query-rewrite mechanism implemented (recall@K bottleneck for the bridge use case). PR #535's bench evidence (-0.024 NDCG, 0/20 precondition rows on a 20-row labelled corpus) confirmed the substrate doesn't work in production. Removing the module rather than leaving default-OFF dead code keeps one source of truth for the vocab-gap closure. Module wiring + tests + uplift driver were removed in earlier commits; this drops the file. Docs updates follow.
Removing src/aelfrice/vocab_bridge.py also dropped the use_hrr alias on retrieve_v2 (no **kwargs catch-all). The seven academic-suite bench adapters still passed use_hrr=True and would TypeError on first call; benchmarks/README.md still documented the old call shape. Fix is to drop the kwarg entirely — the structural HRR lane (use_hrr_structural) is default-on since v2.1, which is the behavior these adapters wanted. Pytest doesn't exercise benchmarks/, so this only surfaces when the bench suite is run; same gap PR #535's precondition-row check found.
vocab_bridge.py is being removed: the lab campaign exp/hrr-vocabulary-bridge adopted typed-edge structural retrieval (shipped via hrr_index.py + use_hrr_structural default-on) and explicitly DROPPED the cascade form that vocab_bridge.py implemented (R2: recall@K bottleneck). PR #535's bench evidence (-0.024 NDCG, 0/20 precondition rows) is consistent with that lab finding. This commit removes the unit + integration tests; module + wiring + docs follow in subsequent commits. After this commit no test references vocab_bridge.py except the run_vocab_bridge_uplift driver in tests/retrieve_uplift_runner.py and its unit tests, which the next commit handles.
The lab campaign exp/hrr-vocabulary-bridge concluded that the vocabulary-gap-recovery claim is closed by the typed-edge structural-retrieval lane (R5 reframe), which ships separately as src/aelfrice/hrr_index.py + the use_hrr_structural lane in retrieve_v2. The R2 finding explicitly DROPPED the cascade form that this module's query-rewrite mechanism implemented (recall@K bottleneck for the bridge use case). PR #535's bench evidence (-0.024 NDCG, 0/20 precondition rows on a 20-row labelled corpus) confirmed the substrate doesn't work in production. Removing the module rather than leaving default-OFF dead code keeps one source of truth for the vocab-gap closure. Module wiring + tests + uplift driver were removed in earlier commits; this drops the file. Docs updates follow.
Removing src/aelfrice/vocab_bridge.py also dropped the use_hrr alias on retrieve_v2 (no **kwargs catch-all). The seven academic-suite bench adapters still passed use_hrr=True and would TypeError on first call; benchmarks/README.md still documented the old call shape. Fix is to drop the kwarg entirely — the structural HRR lane (use_hrr_structural) is default-on since v2.1, which is the behavior these adapters wanted. Pytest doesn't exercise benchmarks/, so this only surfaces when the bench suite is run; same gap PR #535's precondition-row check found.
Adds the strict NDCG@k uplift driver for #433 Phase 2 (the operator-side ship gate). Three atomic commits:
feat(test):run_vocab_bridge_upliftdriver intests/retrieve_uplift_runner.py— mirrorsrun_doc_linker_uplift/run_query_strategy_uplift/run_clustering_upliftshape. Result typeVocabBridgeUplift{n_rows, mean_ndcg_off, mean_ndcg_on}with.upliftproperty.test(uplift):three unit tests (empty input, skip-without-expected_top_k, synthetic-row smoke).test(bench-gate):test_vocab_bridge_ship_gate_runner_presentintests/bench_gate/test_vocab_bridge_uplift.py— same skip-if-runner-absent + skip-if-no-expected_top_kpattern as [v2.0] Intentional clustering — co-locate related beliefs for multi-fact coherence #436. Module docstring updated for the extended row schema (expected_top_kjoinsexpected_canonicals).Bench evidence
I authored a 20-row labelled corpus on the lab side (
~/projects/aelfrice-lab/tests/corpus/v2_0/vocab_bridge/{seed,v0_1,v0_2_charitable}.jsonl, gitea origin only per directory-of-origin rule, lab commit0f0f2b3) and ran both gates against the substrate atgithub/mainHEAD24134b6:appends ≥1 expected canonical, threshold ≥50%)ON=0.6623 OFF=0.6863 uplift=-0.0240(n=20)Both gates fail. The bridge regresses retrieval slightly (~2-3 NDCG points) across all 20 rows.
Substrate root cause
Per-row debug shows the bridge appends HRR-noise tokens (
across,config,run) instead of the designed canonicals (vim,emacs,sqlite,compression,ruff, etc.) — even on rows designed maximally favorably (canonicals strictly NOT in the raw query, anchored on multiple beliefs).Inspection of
_harvest()(src/aelfrice/vocab_bridge.py:203-263):Every surface form gets added as its own canonical. There is no cross-form mapping anywhere in the harvest. So
bridge_vec ≈ sum_t bind(t, t)(every token bound to itself) plus HRR superposition noise. The cleanup-memory query for anyunbind(query_token, bridge_vec)recovers the same token (self-similarity ≈ 1) — but the rewriter then drops it because it's already in the query (vocab_bridge.py:310-311). What gets through is HRR noise: random near-neighbors in the cleanup matrix that exceed the1/sqrt(dim) ≈ 0.022noise floor.The "vocabulary-gap-recovery" claim from the spec presupposes the harvest step distinguishes surface forms from canonicals. The shipped harvest doesn't — it treats every observed token as both. There's no surface→canonical structure for HRR to recover.
What this PR ships
The PR is correct and lands the harness regardless of substrate verdict:
bench_gatedautouse marker).pytest -k vocab_bridge_ship_gate AELFRICE_CORPUS_ROOT=...that fires only when both runner and labelled rows are present.Recommended next step (operator decision)
Two options:
Close-as-not-pursued (mirror Deduplication module (dedup) — v2.0 evaluation #197 dedup R2 outcome). The bench cleared no threshold on plausible corpora; the substrate's "vocab gap recovery" claim isn't supported by
_harvest's self-mapping behavior.use_vocab_bridgestays default-OFF; the substrate stays in tree as a lane other modules can extend; the umbrella row for [v2.0] HRR vocabulary bridge — close vocabulary-gap-recovery claim #433 flips to "evaluated, declined."Re-spec the bridge. Rework
_harvestto extract surface↔canonical pairs (e.g. via lemmatization, abbreviation expansion, anchor-text aliasing rules, or a pre-trained subword tokeniser) so HRR cleanup has actual structure to recover. That's a fresh design loop and a fresh PR — out of scope for [v2.0] HRR vocabulary bridge — close vocabulary-gap-recovery claim #433 as filed.Memory belief lock policy on #197 says "we don't ship a v2.x retrieval feature without positive bench delta" — option 1 is the consistent pick.
Companion to issue #474 umbrella refresh (separate small PR).
Summary by Sourcery
Add an NDCG@k-based uplift runner and ship gate for the vocabulary bridge feature flag.
New Features:
Tests:
Summary by CodeRabbit