fix(benchmarks): honor --granularity across hybrid_v2/v3/v4; reject in palace/diary - #1490
fix(benchmarks): honor --granularity across hybrid_v2/v3/v4; reject in palace/diary#1490nakata-app wants to merge 4 commits into
Conversation
build_palace_and_retrieve_hybrid_v4 accepted a granularity parameter but
never branched on it: the corpus loop always emitted one document per
session, so `--granularity turn` was silently equivalent to `--granularity
session`. With the defect in place, hybrid_v4 turn and session runs
produced bitwise-identical metrics at every hybrid_weight.
Fix:
- Branch the corpus build on granularity. In turn mode, emit one doc per
user turn with corpus IDs shaped as {sess_id}_turn_{i} so the existing
session_id_from_corpus_id helper rolls turns up to sessions at eval.
- Dedup by session id in both the assistant-reference two-pass and the
main scoring path, so multiple high-scoring turns of the same session
collapse to a single ranked entry.
- Keep synthetic preference docs session-aggregated; map a pref-driven
hit to the first user-turn index of its session.
C-β v0.1 sweep results (50q dev split, MiniLM, no LLM rerank) under
benchmarks/c_beta/. Wash hypothesis rejected: keyword-boost lift at turn
granularity (NDCG@10 +0.010) is comparable to session-level lift (+0.007),
with the same concave shape (peak at hw=0.30).
Other hybrid modes (hybrid, hybrid_v2, hybrid_v3, palace, diary, aaak,
rooms) likely carry the same dead-parameter defect — not addressed here.
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request introduces a benchmark sweep to evaluate the impact of session versus turn granularity on retrieval performance, specifically investigating the hybrid_v4 keyword boost. It includes shell scripts for running sweeps and parsing results, along with documentation of findings that revealed a previous defect where the granularity parameter was ignored. The core logic in benchmarks/longmemeval_bench.py was updated to support turn-level indexing and session-based deduplication. Feedback identifies a critical bug in turn-level indexing that breaks assistant-reference logic, the use of non-portable absolute paths in scripts, and a parsing error in the sweep script that results in incorrect metric reporting.
| turn_idx += 1 | ||
| continue | ||
| corpus_user.append(t["content"]) | ||
| corpus_full.append(t["content"]) |
There was a problem hiding this comment.
In turn granularity mode, corpus_full is currently populated with only the individual user turn content. This breaks the hybrid_v4 assistant-reference logic (Pass 1), which relies on corpus_full containing the assistant's responses to match quoted phrases. To ensure the two-pass functionality works as intended in turn mode, corpus_full should contain the full session text for each indexed turn.
| corpus_full.append(t["content"]) | |
| corpus_full.append("\n".join(all_turns)) |
There was a problem hiding this comment.
Fixed in fbbbfa0. In turn mode corpus_full[i] now mirrors the full session text ("\n".join(all_turns)) while corpus_user[i] keeps the per-turn signal, so Pass 2 of the assistant-reference two-pass has assistant content to query against.
Same pattern applied to hybrid_v2 and hybrid_v3 (commit 0d2ffcb).
Re-ran the full 8-row matrix on the fixed code (389650d, benchmarks/c_beta/RESULTS.md and sweep_results.csv updated):
| gran | hw | R@1 | R@10 | NDCG@10 |
|---|---|---|---|---|
| session | 0.30 | 0.880 | 1.000 | 0.944 |
| turn | 0.30 | 0.920 | 1.000 | 0.954 |
Pre-fix turn,hw=0.30 was NDCG@10 0.934 — confirms the two-pass was degraded in turn mode and the fix recovers +0.020.
| DATA=/Users/macmini/Projects/metis-pair/benchmarks/data/longmemeval/longmemeval_s_cleaned.json | ||
| SPLIT=benchmarks/lme_split_50_450.json | ||
| OUTDIR=benchmarks/c_beta | ||
| LOG=$OUTDIR/sweep.log | ||
| CSV=$OUTDIR/sweep_results.csv | ||
|
|
||
| cd /Users/macmini/Projects/mempalace |
There was a problem hiding this comment.
The script contains absolute paths specific to a local machine (e.g., /Users/macmini/...). This prevents the benchmark suite from being portable across different environments. It is recommended to use relative paths or environment variables to define project and data locations.
There was a problem hiding this comment.
Fixed in fbbbfa0. All three scripts (run_sweep.sh, run_turn_sweep.sh, rebuild_csv.sh) now resolve the repo root from BASH_SOURCE instead of hard-coding /Users/macmini/Projects/mempalace. The external dataset path lives behind a DATA env var with a sensible default and a clear error when the file is missing.
| r1=$(awk '/SESSION-LEVEL METRICS/{flag=1;next} flag && /Recall@ 1:/ {print $2; exit}' "$stdout") | ||
| r5=$(awk '/SESSION-LEVEL METRICS/{flag=1;next} flag && /Recall@ 5:/ {print $2; exit}' "$stdout") |
There was a problem hiding this comment.
The awk field index for Recall@ 1: and Recall@ 5: is incorrect in this script. Due to the space between the @ symbol and the single-digit number in the output format, the numeric value resides in the 3rd field ($3), not the 2nd. This results in incorrect values being written to the CSV (e.g., 1: instead of the actual metric), as seen in the provided sweep.log.
| r1=$(awk '/SESSION-LEVEL METRICS/{flag=1;next} flag && /Recall@ 1:/ {print $2; exit}' "$stdout") | |
| r5=$(awk '/SESSION-LEVEL METRICS/{flag=1;next} flag && /Recall@ 5:/ {print $2; exit}' "$stdout") | |
| r1=$(awk '/SESSION-LEVEL METRICS/{flag=1;next} flag && /Recall@ 1:/ {print $3; exit}' "$stdout") | |
| r5=$(awk '/SESSION-LEVEL METRICS/{flag=1;next} flag && /Recall@ 5:/ {print $3; exit}' "$stdout") |
There was a problem hiding this comment.
Fixed in fbbbfa0. Recall@ 1: / Recall@ 5: (single-digit @k with the extra space) now read $3; Recall@10: / Recall@30: (no space) still read $2. Inline comment in the script documents why the two cases differ. rebuild_csv.sh was already using $3 for the single-digit case, which is why the committed CSV was correct despite the bug in run_sweep.sh.
…e/diary
Audit of the remaining retrieval functions for the same dead-parameter
defect fixed in the previous commit on hybrid_v4.
hybrid_v2, hybrid_v3
Same defect — the corpus loop joined all user turns into a single
session-level doc regardless of --granularity. Applied the same fix
pattern: branch the corpus build on granularity (turn mode emits one
doc per user turn with {sess_id}_turn_{i} corpus IDs), and dedup by
session id in both the assistant-reference two-pass and the main
scoring path. Synthetic preference docs stay session-aggregated and
resolve to the first user-turn index of their session.
palace, diary
Algorithm is intrinsically session-keyed:
- palace: hall classification, closets, drawers, and the preference
wing are all per-session structures.
- diary: the LLM topic layer is computed once per session and
cached by sess_id.
A turn-level rewrite would change the algorithm, not just the data
layout, so the honest behavior is to reject the parameter rather
than silently fall back to session. Both now raise a clear
ValueError when called with granularity != "session".
raw / aaak / rooms / hybrid / full
Already honored the flag at the corpus level. Not touched. Their
index-based final dedup (vs. session-id dedup used in the hybrid_v2
/v3/v4 fixes) is a separate concern outside this scope.
Smoke (5q dev split):
- hybrid_v2 turn hw=0.30 → 1.000 R@k / NDCG@k
- hybrid_v3 turn hw=0.30 → 1.000 R@k / NDCG@k
- palace turn → ValueError as expected
- palace session → unchanged from baseline (R@10 1.000, NDCG@10 0.852)
nakata-app
changed the title
1. corpus_full must hold session text in turn mode (HIGH, hybrid_v2/v3/v4) -------------------------------------------------------------- The turn branch wrote only the single user turn into corpus_full, so the assistant-reference two-pass (Pass 2 queries corpus_full for quoted/assistant content) had nothing to match against in turn granularity. Fix: corpus_full now duplicates the full session text per user turn while corpus_user keeps the granular per-turn signal. Removed the now-stale "session-level boosts compensate" comment — they do not, the two-pass quoted match reads corpus_full directly. 2. run_sweep.sh: awk field index for single-digit @k (MEDIUM) ---------------------------------------------------------- "Recall@ 1:" prints with a space, so the value lands in $3, not $2. The previous script wrote literal "1:" / "5:" into the CSV. rebuild_csv.sh already used $3 (which is why the committed CSV is correct), but run_sweep.sh would corrupt any fresh CSV. 3. Absolute paths broke portability (MEDIUM) ------------------------------------------ run_sweep.sh, run_turn_sweep.sh, rebuild_csv.sh all hard-coded /Users/macmini/Projects/mempalace. Switched to BASH_SOURCE-based repo root resolution. DATA path moved behind an env var with a sensible default and a clear error when the file is missing.
After fixing the assistant-reference two-pass for turn granularity (corpus_full now mirrors session text, not the single user turn), the previous turn-mode rows in the sweep were measuring a degraded code path. Re-ran the full 8-row matrix on the fixed code. Key changes vs pre-fix table: - hybrid_v4 turn R@10 saturates at 1.000 across all hw (was 0.980) - hybrid_v4 turn NDCG@10 lifts +0.020 at hw=0.30 (0.934 → 0.954) - Sign of session-vs-turn gap flipped: turn now ≥ session at every hw Hypothesis result is unchanged: keyword boost lift survives at turn granularity (+0.010 NDCG@10 lift hw=0.0→0.30 vs +0.007 at session), same concave shape with peak at hw=0.30. H0 (wash) still rejected. New secondary finding: turn ≥ session NDCG@10 across the whole sweep (Δ = +0.007, +0.010, +0.015 for hw = 0.0/0.30/0.60). Pre-fix table showed the inverse because turn mode was silently dropping assistant content from Pass 2. Direction is clear on 50q but the sample is small; full 500q bootstrap is the honest next step before promoting turn as default. RESULTS.md also corrects the stale caveat that claimed hybrid_v2/v3 were "not audited" — they got the same fix in commit 0d2ffcb.
|
Hi maintainers — quick status on this PR:
Happy to address any further feedback. Thanks! |
…ranularity cherry-pick: nakata-app upstream MemPalace#1490 — honor --granularity across hybrid_v2/v3/v4
…lete (#81) README header was stale: said \"JP's fork of milla-jovovich/mempalace\" (actual upstream is MemPalace/mempalace), pointed at jphein/* shields and links (we transferred to techempower-org), reported a 160K-drawer chromadb palace (now 273K-drawer postgres+pgvector+AGE), and described the substrate cutover as in-flight on feat/pgvector-age-impl (cutover shipped to main on 2026-05-13–15). Refreshes: * Title + author line + shield links → techempower-org * Production palace size + backend → 273K-drawer postgres * Substrate section status block → shipped, with refs to the operator runbook and bench artifact * Storage layer bullet → "main as of 2026-05-15; chromadb path still works behind MEMPALACE_BACKEND=chroma" CHANGELOG: new [Unreleased] section above [3.3.5] capturing the fork-side work that landed 2026-05-14 / 2026-05-15 — postgres cutover, hybrid retrieval (vector ∪ BM25 ∪ graph), symbol_header_prefix kwarg, n=200 git-derived probe set, RRF verifier (2-way + 3-way), bench captures, the pgvector lazy-index race fix, the EmbeddingFunction .embed_query doc note, and the @nakata-app MemPalace#1490 cherry-pick. Co-authored-by: Claude Opus 4.7 <noreply@anthropic.com>
|
@igorls @milla-jovovich — CI hasn't triggered because this is a first-time contributor fork PR and GitHub is holding the workflow run pending maintainer approval. Could one of you click "Approve and run workflows" so ruff + pytest run against the PR? Thanks. |
2 participants
Summary
build_palace_and_retrieve_hybrid_v4(and three of its siblings) accepted agranularityparameter but never branched on it. With the defect in place,--granularity turnand--granularity sessionproduced bitwise-identical metrics for those modes.This PR fixes the dead-parameter defect across the affected modes, and includes a small benchmark sweep that uses the fix to test a hypothesis on
hybrid_v4.What's defective and what each commit does
raw/aaak/rooms/hybrid/fullhybrid_v2hybrid_v3hybrid_v4palaceValueErrorongranularity != "session", hall classification, drawers, and the preference wing are intrinsically session-keyed (commit 2)diaryValueErrorongranularity != "session", LLM topic layer is computed and cached persess_id(commit 2)Fix shape (hybrid_v2 / v3 / v4)
granularity. Inturnmode, emit one doc per user turn with corpus IDs shaped as{sess_id}_turn_{i}so the existingsession_id_from_corpus_idhelper rolls turns up to sessions at eval time.corpus_full[i]mirrors the full session text (not the single user turn) so Pass 2 of the assistant-reference two-pass has assistant content to query against.corpus_user[i]stays the granular per-turn signal.session_id_from_corpus_idis a no-op there).Sweep + hypothesis test (hybrid_v4)
benchmarks/c_beta/(PLAN.md + RESULTS.md + scripts + logs). 50q dev split, default MiniLM, no LLM rerank. All numbers below are post-fix, the full 8-row matrix was re-run after the gemini-code-assist review caught a remaining defect in turn-modecorpus_full(see commitfbbbfa0/389650d).H0 (wash) rejected. Keyword-boost NDCG@10 lift hw=0.0→0.30: turn +0.010 vs session +0.007. Same concave shape (peak at hw=0.30, drop at hw=0.60) across both granularities, so the keyword signal is not a session-length artifact.
Secondary finding: post-fix,
hybrid_v4 turnmatches or beatshybrid_v4 sessionon NDCG@10 at every hw tested (Δ = +0.007, +0.010, +0.015 for hw = 0.0/0.30/0.60). The pre-fix table showed the inverse because turn mode was silently dropping assistant content from Pass 2. Sample is 50q, directional, not yet a default-flip recommendation. Full 500q bootstrap is the honest next step.Test plan
hybrid_v4 turn hw=0.30→ all hitshybrid_v4, 3 hw values, both granularities → see RESULTS.mdhybrid_v2 turn hw=0.30→ all hitshybrid_v3 turn hw=0.30→ all hitspalace turnraisesValueErrorcleanlypalace sessionunchanged from baseline (R@10 1.000, NDCG@10 0.852)hybrid_v4 sessionmetrics unchanged vs pre-fix run