feat(retrieve_uplift): per-flag NDCG@k bench harness for v1.7 default-on flip (#154) - #425
Merged
Merged
Conversation
tests/retrieve_uplift_runner.py is the per-flag uplift harness for the v1.7 default-on flip. For each row in the lab-side retrieve_uplift corpus, it: 1. Builds a transient MemoryStore from the row's beliefs + edges. 2. Calls retrieve(store, query, k=row["k"], ...) once with all flags off (baseline) and once with each v1.7 flag toggled on (others off). 3. Scores each result list with graded NDCG@k against row["expected_top_k"]. 4. Reports mean NDCG_off, NDCG_on, and uplift per flag. Five flags exercised: - use_bm25f_anchors (#148) — wired - use_signed_laplacian (#149) — placeholder; warning-only flag, will report uplift=0 until the lane lands in retrieve() - use_heat_kernel (#150) — wired via heat_kernel_enabled - use_posterior_ranking (#151) — wired via non-zero posterior_weight - use_hrr_structural (#152) — placeholder; same as #149 The runner is also a CLI: AELFRICE_CORPUS_ROOT=... python -m tests.retrieve_uplift_runner prints the per-flag NDCG table and exits 1 if any flag regresses. Seven unit tests in tests/test_retrieve_uplift_runner.py cover the NDCG@k arithmetic (perfect, empty, no-overlap, partial, position matters), the FlagUplift dataclass, and the end-to-end harness over a one-row synthetic corpus. All run on public CI without AELFRICE_CORPUS_ROOT.
…154) - tests/bench_gate/test_retrieve_uplift.py asserts no v1.7 flag regresses NDCG@k against the all-flags-off baseline. Skip-on-no- corpus per the directory-of-origin rule. The test deliberately does NOT enforce a positive-uplift threshold — that's an operator decision per flag, made on the resulting evidence table. - tests/test_corpus_schema.py registers retrieve_uplift as a graded module with required fields {query, beliefs, edges, expected_top_k (ordered), k}. - tests/corpus/v2_0/README.md documents the new module under both the layout tree and the per-line-shape table. Notes expected_top_k is ORDERED (top-relevant first) — distinct from the BFS modules' set-shaped expected_hit_ids — because NDCG cares about position. Once lab corpus rows land under tests/corpus/v2_0/retrieve_uplift/, the bench-gate runs from the lab side via: AELFRICE_CORPUS_ROOT=~/projects/aelfrice-lab/tests/corpus/v2_0 \\ uv run pytest tests/bench_gate/test_retrieve_uplift.py The per-flag uplift table is the evidence the operator uses to decide which flags flip default-on and which stay default-off in the v1.7 release.
Reviewer's GuideAdds a per-flag NDCG@k benchmark harness for retrieve() to support the v1.7 default-on flip decision, wires it into the corpus schema and lab corpus layout, and introduces both unit tests and a bench-gated test that enforce no per-flag NDCG regression against an all-flags-off baseline. File-Level Changes
Possibly linked issues
Tips and commandsInteracting with Sourcery
Customizing Your ExperienceAccess your dashboard to:
Getting Help
|
📝 WalkthroughWalkthroughThis PR adds a regression testing suite for the ChangesRetrieve Uplift Regression Testing
Sequence DiagramsequenceDiagram
participant Corpus as Corpus (JSONL)
participant Harness as Per-Flag Harness
participant Store as MemoryStore<br/>(Temp SQLite)
participant Retrieve as retrieve()
participant Scorer as NDCG@k Scorer
participant Aggregator as Metrics<br/>Aggregator
loop For each corpus row
Corpus->>Harness: Load row (beliefs, edges, query, expected_top_k, k)
Harness->>Store: Create fresh MemoryStore
Harness->>Store: Seed beliefs & edges
loop For baseline + each flag
Note over Harness: Prepare retrieve() kwargs<br/>(baseline or flag-enabled)
Harness->>Retrieve: retrieve(store, query, k, **kwargs)
Retrieve->>Store: Query & rank
Retrieve-->>Harness: result_ids (top-k list)
Harness->>Scorer: Compute ndcg_at_k(result_ids, expected_top_k, k)
Scorer-->>Harness: NDCG value ∈ [0, 1]
Harness->>Aggregator: Accumulate score
end
Harness->>Store: Close MemoryStore
end
Aggregator->>Aggregator: mean_ndcg_off per flag
Aggregator->>Aggregator: mean_ndcg_on per flag
Aggregator->>Aggregator: uplift = mean_ndcg_on − mean_ndcg_off
Aggregator-->>Harness: FlagUplift list (one per flag)
Harness->>Harness: Fail if any uplift < 0
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Possibly related PRs
🚥 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 |
![sourcery-ai[bot]](https://avatars.githubusercontent.com/in/48477?s=60&v=4){kind=small}
There was a problem hiding this comment.
Hey - I've left some high level feedback:
- The
--corpus-rootdefault currently evaluates toPath('')(i.e..) whenAELFRICE_CORPUS_ROOTis unset, soargs.corpus_rootis neverNoneand the"not set"branch is effectively dead; if you want missing env to be an error, consider computing the default explicitly (e.g.env = os.environ.get(...); default=None if not env else Path(env)). - The module docstring and
FLAG_KWARGScomments sayuse_signed_laplaciananduse_hrr_structuralare "warning-only" placeholders, but there’s no actual warning or differentiation inrun_per_flag_uplift; either wire in a warning/log when these flags are exercised or adjust the comments to match the current behavior.
Prompt for AI Agents
Please address the comments from this code review:
## Overall Comments
- The `--corpus-root` default currently evaluates to `Path('')` (i.e. `.`) when `AELFRICE_CORPUS_ROOT` is unset, so `args.corpus_root` is never `None` and the `"not set"` branch is effectively dead; if you want missing env to be an error, consider computing the default explicitly (e.g. `env = os.environ.get(...); default=None if not env else Path(env)`).
- The module docstring and `FLAG_KWARGS` comments say `use_signed_laplacian` and `use_hrr_structural` are "warning-only" placeholders, but there’s no actual warning or differentiation in `run_per_flag_uplift`; either wire in a warning/log when these flags are exercised or adjust the comments to match the current behavior.Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (1)
tests/retrieve_uplift_runner.py (1)
214-232: ⚡ Quick winBaseline NDCG is recomputed once per flag per row — O(2 × flags × rows) SQLite calls instead of O(flags + 1) × rows.
For each flag iteration,
_row_ndcg(row, k, {}, tmp_root)creates a fresh SQLite DB, seeds it, and runsretrieve()— identically to every other flag's baseline call for the same row. With 5 flags and the ≥50-row v0.1 corpus target that's 500 calls instead of 300. The two placeholder flags (use_signed_laplacian,use_hrr_structural) make this especially wasteful since their "on" calls are also identical to baseline.♻️ Proposed refactor — compute baseline once per row
- for flag, kwargs_fn in FLAG_KWARGS.items(): - kwargs_on = kwargs_fn() - off_total = 0.0 - on_total = 0.0 - for row in rows: - k = _default_k(row) - off_total += _row_ndcg(row, k, {}, tmp_root) - on_total += _row_ndcg(row, k, kwargs_on, tmp_root) - n = len(rows) - out.append(FlagUplift( - flag=flag, - n_rows=n, - mean_ndcg_off=off_total / n if n else 0.0, - mean_ndcg_on=on_total / n if n else 0.0, - )) + # Compute baseline once per row, then reuse across all flag arms. + baseline_scores = [_row_ndcg(row, _default_k(row), {}, tmp_root) for row in rows] + n = len(rows) + for flag, kwargs_fn in FLAG_KWARGS.items(): + kwargs_on = kwargs_fn() + on_scores = [_row_ndcg(row, _default_k(row), kwargs_on, tmp_root) for row in rows] + off_total = sum(baseline_scores) + on_total = sum(on_scores) + out.append(FlagUplift( + flag=flag, + n_rows=n, + mean_ndcg_off=off_total / n if n else 0.0, + mean_ndcg_on=on_total / n if n else 0.0, + ))🤖 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/retrieve_uplift_runner.py` around lines 214 - 232, The loop recomputes the baseline NDCG for every flag and row via _row_ndcg(row, k, {}, tmp_root); change the logic to compute the baseline once per row and reuse it for all flags: for each row (and k = _default_k(row)) call baseline_ndcg = _row_ndcg(row, k, {}, tmp_root) once, then iterate over FLAG_KWARGS items and compute on_ndcg = _row_ndcg(row, k, kwargs_on, tmp_root) only for the flagged variant (and if kwargs_on is {} reuse baseline_ndcg), accumulating off_total using baseline_ndcg instead of calling _row_ndcg repeatedly; keep producing FlagUplift objects with mean_ndcg_off based on the cached baseline values.
🤖 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/retrieve_uplift_runner.py`:
- Around line 85-104: The function _belief_from_row incorrectly reads
b["content"] (causing KeyError on real corpus rows where the field is "text");
update _belief_from_row to read b["text"] and map it into the Belief.content
field (i.e., set content=b["text"]) and keep content_hash and other fields
unchanged; also update the synthetic test data in
tests/test_retrieve_uplift_runner.py to use "text" (not "content") so tests
reflect the schema-validated corpus shape and will catch regressions.
In `@tests/test_retrieve_uplift_runner.py`:
- Around line 61-82: The test uses synthetic belief dicts with key "content"
which will break once _belief_from_row is fixed to read b["text"]; update the
synthetic row in test_run_per_flag_uplift_covers_all_flags so each belief uses
"text" instead of "content" (the row passed into run_per_flag_uplift), and scan
the test for any other synthetic beliefs to make the same change; keep
references to run_per_flag_uplift and FLAG_KWARGS unchanged.
---
Nitpick comments:
In `@tests/retrieve_uplift_runner.py`:
- Around line 214-232: The loop recomputes the baseline NDCG for every flag and
row via _row_ndcg(row, k, {}, tmp_root); change the logic to compute the
baseline once per row and reuse it for all flags: for each row (and k =
_default_k(row)) call baseline_ndcg = _row_ndcg(row, k, {}, tmp_root) once, then
iterate over FLAG_KWARGS items and compute on_ndcg = _row_ndcg(row, k,
kwargs_on, tmp_root) only for the flagged variant (and if kwargs_on is {} reuse
baseline_ndcg), accumulating off_total using baseline_ndcg instead of calling
_row_ndcg repeatedly; keep producing FlagUplift objects with mean_ndcg_off based
on the cached baseline values.
🪄 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: 5a00d32c-105d-46d3-b896-bd6071973478
📒 Files selected for processing (5)
tests/bench_gate/test_retrieve_uplift.pytests/corpus/v2_0/README.mdtests/retrieve_uplift_runner.pytests/test_corpus_schema.pytests/test_retrieve_uplift_runner.py
Comment on lines
+85
to
+104
| def _belief_from_row(b: dict) -> Belief: # type: ignore[type-arg] | ||
| """Build a Belief from a corpus row's belief dict. | ||
|
|
||
| Required: `id`, `content`. Optional: `type`, `alpha`, `beta`. | ||
| Defaults match the factual/agent-inferred shape. | ||
| """ | ||
| return Belief( | ||
| id=b["id"], | ||
| content=b["content"], | ||
| content_hash=f"corpus:{b['id']}", | ||
| alpha=float(b.get("alpha", 1.0)), | ||
| beta=float(b.get("beta", 1.0)), | ||
| type=b.get("type", BELIEF_FACTUAL), | ||
| lock_level=LOCK_NONE, | ||
| locked_at=None, | ||
| demotion_pressure=0, | ||
| created_at=_TS, | ||
| last_retrieved_at=None, | ||
| origin=ORIGIN_AGENT_INFERRED, | ||
| ) |
There was a problem hiding this comment.
_belief_from_row reads b["content"] but corpus beliefs have a "text" field — KeyError on real corpus data.
The schema validator in test_corpus_schema.py (the "list[belief]" spec) enforces b["text"] on every corpus row. Other BFS modules also document belief objects as {"id": str, "text": str} (README line 120). The harness maps this corpus field to Belief.content (the internal model attribute name), so the key to read is "text", not "content".
The unit test in test_retrieve_uplift_runner.py (lines 68–71) uses "content" in its synthetic beliefs, which means the tests pass locally while silently masking the mismatch — the integration against real corpus rows will raise KeyError: 'content'.
🐛 Proposed fix
def _belief_from_row(b: dict) -> Belief:
return Belief(
id=b["id"],
- content=b["content"],
+ content=b["text"],
content_hash=f"corpus:{b['id']}",And in tests/test_retrieve_uplift_runner.py lines 68–71, update synthetic beliefs to match the schema-validated shape:
"beliefs": [
- {"id": "b1", "content": "the memory store persists beliefs"},
- {"id": "b2", "content": "the configuration file lives at /etc"},
- {"id": "b3", "content": "the memory store uses sqlite"},
+ {"id": "b1", "text": "the memory store persists beliefs"},
+ {"id": "b2", "text": "the configuration file lives at /etc"},
+ {"id": "b3", "text": "the memory store uses sqlite"},
],🤖 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/retrieve_uplift_runner.py` around lines 85 - 104, The function
_belief_from_row incorrectly reads b["content"] (causing KeyError on real corpus
rows where the field is "text"); update _belief_from_row to read b["text"] and
map it into the Belief.content field (i.e., set content=b["text"]) and keep
content_hash and other fields unchanged; also update the synthetic test data in
tests/test_retrieve_uplift_runner.py to use "text" (not "content") so tests
reflect the schema-validated corpus shape and will catch regressions.
Comment on lines
+61
to
+82
| def test_run_per_flag_uplift_covers_all_flags() -> None: | ||
| """Hypothesis: the harness reports one row per registered flag. | ||
| Falsifiable if a flag is silently dropped.""" | ||
| row = { | ||
| "id": "ru-test-001", | ||
| "query": "memory store", | ||
| "k": 3, | ||
| "beliefs": [ | ||
| {"id": "b1", "content": "the memory store persists beliefs"}, | ||
| {"id": "b2", "content": "the configuration file lives at /etc"}, | ||
| {"id": "b3", "content": "the memory store uses sqlite"}, | ||
| ], | ||
| "edges": [], | ||
| "expected_top_k": ["b1", "b3"], | ||
| } | ||
| results = run_per_flag_uplift([row]) | ||
| flags = {r.flag for r in results} | ||
| assert flags == set(FLAG_KWARGS.keys()) | ||
| for r in results: | ||
| assert r.n_rows == 1 | ||
| assert 0.0 <= r.mean_ndcg_off <= 1.0 | ||
| assert 0.0 <= r.mean_ndcg_on <= 1.0 |
There was a problem hiding this comment.
Synthetic beliefs use "content" instead of "text" — masks the corpus field name bug.
Once _belief_from_row is corrected to read b["text"] (see the critical issue on retrieve_uplift_runner.py line 93), these synthetic beliefs must also be updated to "text" or the unit test will start failing with a KeyError. See the proposed diff in that comment for the coordinated fix.
🤖 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/test_retrieve_uplift_runner.py` around lines 61 - 82, The test uses
synthetic belief dicts with key "content" which will break once _belief_from_row
is fixed to read b["text"]; update the synthetic row in
test_run_per_flag_uplift_covers_all_flags so each belief uses "text" instead of
"content" (the row passed into run_per_flag_uplift), and scan the test for any
other synthetic beliefs to make the same change; keep references to
run_per_flag_uplift and FLAG_KWARGS unchanged.
This was referenced May 5, 2026
This was referenced May 5, 2026
robotrocketscience
added a commit
that referenced
this pull request
May 5, 2026
The v1.7 row was stale: components #149/#150/#152/#153/#216 are all merged on main and reachable via opt-in feature flags (use_signed_laplacian, use_heat_kernel, use_hrr_structural in [retrieval] of .aelfrice.toml). Updates the row to reflect ship state. The default-on flip (#154) is deferred. The retrieve-uplift bench harness (#403/#425) measured +0.6010 NDCG@k uplift for use_bm25f_anchors on the v0.1 fixture, but a follow-up smoke test exposed a stemming gap: BM25F's lowercase-tokenize-only path misses matches that FTS5's Porter stemming catches (banana vs bananas). The +0.6010 number was correct for exact-token queries; the production cost on natural-language queries that stem-differ from content is not yet quantified. Until that's measured, leaving v1.7 components opt-in keeps the v1.6 retrieval characteristic intact. v2.0 row tightened to call out v1.7 default-on flip as the prereq for the reproducibility-cut tag.
robotrocketscience
added a commit
that referenced
this pull request
May 5, 2026
The v1.7 row was stale: components #149/#150/#152/#153/#216 are all merged on main and reachable via opt-in feature flags (use_signed_laplacian, use_heat_kernel, use_hrr_structural in [retrieval] of .aelfrice.toml). Updates the row to reflect ship state. The default-on flip (#154) is deferred. The retrieve-uplift bench harness (#403/#425) measured +0.6010 NDCG@k uplift for use_bm25f_anchors on the v0.1 fixture, but a follow-up smoke test exposed a stemming gap: BM25F's lowercase-tokenize-only path misses matches that FTS5's Porter stemming catches (banana vs bananas). The +0.6010 number was correct for exact-token queries; the production cost on natural-language queries that stem-differ from content is not yet quantified. Until that's measured, leaving v1.7 components opt-in keeps the v1.6 retrieval characteristic intact. v2.0 row tightened to call out v1.7 default-on flip as the prereq for the reproducibility-cut tag.
robotrocketscience
added a commit
that referenced
this pull request
May 5, 2026
The v1.7 row was stale: components #149/#150/#152/#153/#216 are all merged on main and reachable via opt-in feature flags (use_signed_laplacian, use_heat_kernel, use_hrr_structural in [retrieval] of .aelfrice.toml). Updates the row to reflect ship state. The default-on flip (#154) is deferred. The retrieve-uplift bench harness (#403/#425) measured +0.6010 NDCG@k uplift for use_bm25f_anchors on the v0.1 fixture, but a follow-up smoke test exposed a stemming gap: BM25F's lowercase-tokenize-only path misses matches that FTS5's Porter stemming catches (banana vs bananas). The +0.6010 number was correct for exact-token queries; the production cost on natural-language queries that stem-differ from content is not yet quantified. Until that's measured, leaving v1.7 components opt-in keeps the v1.6 retrieval characteristic intact. v2.0 row tightened to call out v1.7 default-on flip as the prereq for the reproducibility-cut tag.
This was referenced May 5, 2026
Merged
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
2 participants
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.
Task 1 of #154's default-on flip workflow. Builds the per-flag retrieve() NDCG@k bench harness; the lab-side run + per-flag flip decisions + README update + v2.0 tag are tasks 2–5 (sequenced after this lands).
What ships
Per-flag NDCG@k harness —
tests/retrieve_uplift_runner.pyDrives a labeled query corpus through
retrieve()once with all v1.7 flags off (baseline) and once with each flag toggled on (others off). Computes mean NDCG@k per arm and reports per-flag uplift.Five flags exercised:
use_bm25f_anchors([retrieval] Augmented BM25F (incoming-edge anchor text) + vectorized BM25 sparse matvec #148) — wireduse_signed_laplacian([retrieval] Signed normalized Laplacian + offline eigenbasis (top-K=200) builder #149) — placeholder; reports uplift=0 until the lane landsuse_heat_kernel([retrieval] Heat kernel authority signal via precomputed eigenbasis (signed Laplacian) #150) — wired viaheat_kernel_enableduse_posterior_ranking([retrieval] Posterior-weighted ranking via Beta-Bernoulli prior (log-additive, weight 0.5) #151) — wired viaposterior_weight=0.5use_hrr_structural([retrieval] HRR structural-query lane (bind/probe over outgoing edges) #152) — placeholder; same shape as [retrieval] Signed normalized Laplacian + offline eigenbasis (top-K=200) builder #149The runner is also a CLI:
Prints a per-flag NDCG table; exits 1 if any flag regresses.
Bench-gate test —
tests/bench_gate/test_retrieve_uplift.py@pytest.mark.bench_gated. Asserts no flag regresses NDCG@k against the baseline. Skip-on-no-corpus.The test deliberately does NOT enforce a positive-uplift threshold — per-flag flip thresholds are operator decisions on the resulting evidence table.
Unit tests —
tests/test_retrieve_uplift_runner.pySeven tests cover the NDCG@k arithmetic (perfect ordering = 1.0, empty expected = 0, no overlap = 0, partial overlap in (0,1), position matters) plus the FlagUplift dataclass and an end-to-end one-row synthetic corpus. All run on public CI without
AELFRICE_CORPUS_ROOT.Schema registration
tests/test_corpus_schema.pyregistersretrieve_upliftas a graded module:{query, beliefs, edges, expected_top_k (ordered), k}.tests/corpus/v2_0/README.mdupdated layout + per-line-shape table. Notesexpected_top_kis ordered (top-relevant first) — distinct from BFS modules' set-shapedexpected_hit_ids— because NDCG cares about position.Out of scope (subsequent #154 tasks)
AELFRICE_CORPUS_ROOTmounted.shipped.Test plan
uv run pytest tests/test_retrieve_uplift_runner.py -v— 7 passed.uv run pytest --ignore=tests/bench_gate -q— 2456 passed (+10 new), 23 skipped.uv run pytest tests/test_corpus_schema.py -q— 15 skipped (no corpus mounted on public CI; schema entries registered).AELFRICE_CORPUS_ROOT=... uv run pytest tests/bench_gate/test_retrieve_uplift.py— runs once a populated corpus lands.Summary by Sourcery
Introduce a per-flag NDCG@k benchmark harness and gate for retrieve() v1.7 flags using a graded corpus module.
New Features:
Enhancements:
Documentation:
Tests:
Summary by CodeRabbit
New Features
Tests
Documentation