cherry-pick: nakata-app upstream #1490 — honor --granularity across hybrid_v2/v3/v4 - #79
Merged
Merged
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.
…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)
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.
jphein
force-pushed
the
cherry-pick/nakata-1490-granularity
branch
from
May 15, 2026 07:36
2cb602f to
7766343
Compare
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.
Summary
Cherry-picks three commits from MemPalace/mempalace#1490 (open upstream, authored by @nakata-app):
Skipped: `389650d` (`docs(benchmarks): rerun full sweep post-fix, update RESULTS.md`) — that's @nakata-app's own benchmark numbers; we'll generate our own when we re-run.
Why now (vs. waiting for upstream merge)
The upstream PR is open, gated on workflow approval, and the maintainer has been quiet. Per our fix-in-our-fork-now, accept-conflict-at-upstream-merge-time convention, taking the fix locally unblocks our own benchmarking without depending on the upstream timeline. The bug is real: `build_palace_and_retrieve_hybrid_v{2,3,4}` accepted a `granularity` parameter but never branched on it — so `--granularity turn` and `--granularity session` produced bitwise-identical metrics for those modes. We hit this any time we run `benchmarks/longmemeval_bench.py` and care about the granularity knob.
What the fix does
Headline numbers from @nakata-app's post-fix sweep
Pre-fix turn,hw=0.30 was NDCG@10 0.934 → +0.020 from honoring the parameter correctly. We'll cross-check on our corpus once this merges.
Conflict cost at upstream merge
If `MemPalace#1490` eventually merges, our fork will see these three commits twice (once as cherry-picks here, once via the upstream merge). Standard duplicate-pick resolution: git auto-detects identical-content cherries and the redundant commits squash cleanly. Expected zero-cost.
Test plan
🤖 Generated with Claude Code