Replace min-max BM25 fusion with RRF on hybrid recall path - #112
Conversation
Min-max BM25 + weighted cosine mixed an absolute geometry score with a per-query relative stretch, amplifying BM25 noise and compressing mid-tier hits. Fuse BM25 and cosine by Reciprocal Rank Fusion (k=60) instead, scale by (k+1)/2, then keep the existing temporal blend. Exact/lexical lanes and _normalize_bm25 callers are unchanged. Co-authored-by: Cursor <cursoragent@cursor.com>
📝 WalkthroughWalkthroughThe hybrid recall path now uses Reciprocal Rank Fusion over FTS candidate and cosine similarity rankings, then applies temporal blending. A reusable RRF helper, updated configuration parsing, documentation changes, and tests for fusion ordering and score calculations were added. ChangesHybrid recall fusion
Estimated code review effort: 3 (Moderate) | ~20 minutes Possibly related issues
Possibly related PRs
🚥 Pre-merge checks | ✅ 2✅ Passed checks (2 passed)
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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
🧹 Nitpick comments (1)
marm-mcp-server/marm_mcp_server/core/memory_recall.py (1)
9-12: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winDead
HYBRID_SEARCH_TEXT_WEIGHTimport removed here, but setting itself left dangling.The import is dropped but the underlying setting still exists elsewhere and is now inert for hybrid fusion — a knob operators could still tune with no effect. Consider removing/deprecating the setting itself (or documenting that it no longer affects hybrid ranking) so it doesn't silently mislead future tuning.
As per coding guidelines, "prefer the smallest change and avoid speculative abstractions or unused configuration flags."
🤖 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 `@marm-mcp-server/marm_mcp_server/core/memory_recall.py` around lines 9 - 12, Remove the now-unused HYBRID_SEARCH_TEXT_WEIGHT setting from its defining configuration and any related exports or references, leaving hybrid ranking unchanged and avoiding an inert tuning flag.Source: Coding guidelines
🤖 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.
Nitpick comments:
In `@marm-mcp-server/marm_mcp_server/core/memory_recall.py`:
- Around line 9-12: Remove the now-unused HYBRID_SEARCH_TEXT_WEIGHT setting from
its defining configuration and any related exports or references, leaving hybrid
ranking unchanged and avoiding an inert tuning flag.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro Plus
Run ID: eb757444-a7a7-4875-9cbc-7f3b8bd13817
📒 Files selected for processing (3)
marm-mcp-server/marm_mcp_server/core/memory_recall.pymarm-mcp-server/marm_mcp_server/core/memory_scoring.pymarm-mcp-server/tests/test_hybrid_search.py
📜 Review details
🧰 Additional context used
📓 Path-based instructions (5)
**/*.{py,json}
📄 CodeRabbit inference engine (AGENTS.md)
Keep HTTP and STDIO MCP transports in exact parity; when adding or removing a tool, update the endpoint implementation, HTTP route and whitelist, STDIO registration, server manifest, canonical tool list, documentation, and tests for both transports.
Files:
marm-mcp-server/marm_mcp_server/core/memory_scoring.pymarm-mcp-server/tests/test_hybrid_search.pymarm-mcp-server/marm_mcp_server/core/memory_recall.py
marm-mcp-server/marm_mcp_server/**/*.py
📄 CodeRabbit inference engine (AGENTS.md)
marm-mcp-server/marm_mcp_server/**/*.py: All memory writes must use the serialized asynchronous write queue; do not bypass it.
A semantic-store failure inmarm_log_entrymust never fail the corresponding log write.
Never share SQLite connections between the memory database and the isolated concept-graph database.
Graph and concept failures must never break the seven core memory tools; graph processes start lazily and run degraded on failure.
marm_smart_recallmust keep primary memory ranking authoritative; graph enrichment is bounded, read-only, fail-open, and trimmed before primary results when enforcing limits.
Use one lazy-loaded, lock-serializedjinaai/jina-embeddings-v2-small-enencoder with 512 dimensions; writes must succeed when it is unavailable.
Keep orchestration in its current owner file and extract modules only at real boundaries; prefer the smallest change and avoid speculative abstractions or unused configuration flags.
Use minimal comments that explain only non-obvious reasons; never narrate what the next line does.
When upgrading embeddings from MiniLM, migrate existing data withmarm-mcp-server --migrate-embeddingsbefore restarting.
Files:
marm-mcp-server/marm_mcp_server/core/memory_scoring.pymarm-mcp-server/marm_mcp_server/core/memory_recall.py
**/*.py
⚙️ CodeRabbit configuration file
**/*.py: Prioritize runtime correctness, async/concurrency safety, SQLite transaction safety, auth/rate-limit behavior, release-breaking packaging issues, and MCP protocol compatibility.
Files:
marm-mcp-server/marm_mcp_server/core/memory_scoring.pymarm-mcp-server/tests/test_hybrid_search.pymarm-mcp-server/marm_mcp_server/core/memory_recall.py
marm-mcp-server/tests/**/*.py
📄 CodeRabbit inference engine (AGENTS.md)
marm-mcp-server/tests/**/*.py: Run tests withpytestfrommarm-mcp-server/; use real FastAPI endpoints and real SQLite, mocking only when it meaningfully speeds tests and matches real behavior with at least 95% fidelity.
Every new MARM Console API route must have at least one happy-path FastAPI response-contract test with the MCP adapter stubbed.
Do not write existence-check or coded-to-pass tests; prefer deep tests exercising real paths over broad shallow coverage.
Usepytest.mark.skiponly for genuinely unavailable dependencies, never merely for effort.
Files:
marm-mcp-server/tests/test_hybrid_search.py
**/tests/**
⚙️ CodeRabbit configuration file
**/tests/**: Focus on tests that are flaky, non-isolated, incorrectly asserting behavior, or missing coverage for a changed high-risk path. Skip minor naming, comments, and layout preferences.
Files:
marm-mcp-server/tests/test_hybrid_search.py
🔇 Additional comments (2)
marm-mcp-server/marm_mcp_server/core/memory_scoring.py (1)
22-46: LGTM!marm-mcp-server/marm_mcp_server/core/memory_recall.py (1)
267-284: 🎯 Functional CorrectnessRRF's reliance on score-sorted input lists is unverified in both production code and tests.
The new fusion introduces a dependency that didn't exist before:
_rrf_scorestreats list position as rank, sobm25_ranked/cosine_rankedmust already be sorted best-first by their respective retrieval functions. Pre-PR, fusion used raw score values directly, so ordering never mattered.
marm-mcp-server/marm_mcp_server/core/memory_recall.py#L267-L284: confirm_fetch_and_score_by_ids/_score_chunk_awareand_fetch_fts_candidate_idsreturn their tuples sorted by descending score before relying on positional rank for RRF; if not guaranteed, sort explicitly before buildingbm25_ranked/cosine_ranked.marm-mcp-server/tests/test_hybrid_search.py#L644-L705: both tests stub the ranked-id lists directly, so they can't catch a regression where the real scoring functions stop returning sorted output; add (or request) a test exercising the real (non-mocked)_fetch_and_score_by_ids/_fetch_fts_candidate_idspath with distinguishable scores to assert the returned order matches score order.
|
I went through this carefully, and it's strong work. The RRF implementation is clean and correct, the tests are genuinely deep (the reversed-order cancellation case especially), and you held to every constraint. The consolidation.py cross-file catch was the standout for me; that's real systems thinking, not just patching the one file. Three things before this can merge.
One thing though: exact_mode="semantic" (your suggested fix #1) still runs the FTS-candidate + RRF fusion path (memory_recall.py:208-266); it only skips the exact lane. No mode returns pure cosine, so that remedy wouldn't actually restore the threshold. Your remedy #2 (report cosine separately from the ranking score) is the sound one. Can you open a tracked issue for this and sketch how you'd surface a cosine value alongside the RRF score? I don't want a known regression riding in on just a description note that disappears once merged.
LoCoMo10 retrieval accuracy, MARM-main (v2.29.0), limit=5:
Run config (please match it exactly, or the delta is meaningless):
Run the same eval on your branch and report the delta. Compare the semantic lane (sem-any) that's the lane RRF actually changes. Heads up: the log lane came back empty in my run, so compare semantic like-for-like rather than the union. Being straight about what this benchmark is: pure evidence-ID retrieval accuracy "did the right memory come back." It is NOT an LLM or answer-quality benchmark; it says nothing about answer generation, and one number can't fully capture "robustness across query types," which is the whole reason you picked RRF over normalizing both scores. So treat it as one strong signal, not the final verdict. And if it comes back a wash, that's useful too; report it honestly either way. A wash doesn't sink the PR; it just changes the conversation. |
The weighted BM25/cosine blend setting is unused now that hybrid recall fuses rank lists via RRF. Drop the env var from settings and README tables. Co-authored-by: Cursor <cursoragent@cursor.com>
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 `@CHANGELOG.md`:
- Line 557: Update the changelog entry describing marm_smart_recall to state
that hybrid recall uses scaled reciprocal rank fusion (RRF) of FTS5 BM25 and
cosine similarity rank orders before temporal blending, replacing the current
semantic/BM25 score-merging description.
🪄 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 Plus
Run ID: a38f78a6-5e1f-4809-9b77-94a7126de495
📒 Files selected for processing (5)
CHANGELOG.mdREADME.mdmarm-mcp-server/README.mdmarm-mcp-server/marm-docs/README.mdmarm-mcp-server/marm_mcp_server/config/settings.py
💤 Files with no reviewable changes (4)
- README.md
- marm-mcp-server/marm-docs/README.md
- marm-mcp-server/marm_mcp_server/config/settings.py
- marm-mcp-server/README.md
📜 Review details
🧰 Additional context used
📓 Path-based instructions (1)
**/*.md
⚙️ CodeRabbit configuration file
**/*.md: Only flag documentation issues that are materially wrong, misleading for installation/release behavior, or inconsistent with live MCP behavior. Skip style, phrasing, formatting, and wording preferences.
Files:
CHANGELOG.md
| ### Recall & Search | ||
|
|
||
| - Added SQLite FTS5 indexing for memory content with automatic insert/update/delete triggers; `marm_smart_recall` merges semantic similarity with FTS BM25 keyword scoring via `HYBRID_SEARCH_TEXT_WEIGHT`, improving recall for commands, config keys, filenames, and error strings. | ||
| - Added SQLite FTS5 indexing for memory content with automatic insert/update/delete triggers; `marm_smart_recall` merges semantic similarity with FTS BM25 keyword scoring, improving recall for commands, config keys, filenames, and error strings. |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
Update the changelog to describe RRF rank fusion.
Line 557 still describes semantic/BM25 score merging, but hybrid recall now fuses the FTS BM25 rank order with the cosine rank order using scaled RRF before temporal blending. This is materially misleading for users recalibrating similarity thresholds.
Proposed wording
-- Added SQLite FTS5 indexing for memory content with automatic insert/update/delete triggers; `marm_smart_recall` merges semantic similarity with FTS BM25 keyword scoring, improving recall for commands, config keys, filenames, and error strings.
+- Added SQLite FTS5 indexing for memory content with automatic insert/update/delete triggers; `marm_smart_recall` combines FTS BM25 and semantic rank orders using Reciprocal Rank Fusion, improving recall for commands, config keys, filenames, and error strings.📝 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.
| - Added SQLite FTS5 indexing for memory content with automatic insert/update/delete triggers; `marm_smart_recall` merges semantic similarity with FTS BM25 keyword scoring, improving recall for commands, config keys, filenames, and error strings. | |
| - Added SQLite FTS5 indexing for memory content with automatic insert/update/delete triggers; `marm_smart_recall` combines FTS BM25 and semantic rank orders using Reciprocal Rank Fusion, improving recall for commands, config keys, filenames, and error strings. |
🤖 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 `@CHANGELOG.md` at line 557, Update the changelog entry describing
marm_smart_recall to state that hybrid recall uses scaled reciprocal rank fusion
(RRF) of FTS5 BM25 and cosine similarity rank orders before temporal blending,
replacing the current semantic/BM25 score-merging description.
Source: Path instructions
|
Pushed all three fixes (commit 0abae39): PR description corrected: 0.92 (not ~0.88+), and updated to note exact_mode="semantic" doesn't bypass RRF Also sent the LoCoMo10 benchmark delta separately, result was a wash on sem-any, with my read on why that's plausible given this is a ranking-only change on a membership-insensitive metric. Ready for another pass whenever you have time. |
|
@Mxneeb I reviewed the new commits. All three items are cleanly resolved:
Nothing to send back on the code. As you already called out yourself, this doesn't prove a retrieval win until the |
|
Thanks for this, and for the care in the writeup. We took it seriously enough to build a full benchmark around it rather than accept or reject it on reasoning. Closing it unmerged, with the data below. Why it could not be measured as submittedThe PR removes That is a 10x change in lexical influence landing in the same diff as the fusion change, so any accuracy difference could not be attributed to rank-vs-score fusion. Two variables, one measurement. Weighted RRF, so the comparison was fairRather than reject on that, we derived a weighted RRF that preserves the shipped lexical weight: The Both arms then got identical candidates, an identical keyword weight, and an identical output range. The only variable was magnitude vs rank position. ResultsLoCoMo, 1,977 scored questions, 5,882 memories, top-5, no LLM judge. One corpus, three server processes,
Two min-max control runs of the identical configuration differ by 0.56 pp, so that is the noise floor. RRF lost by roughly 12x the noise. It gained 48 questions and lost 192. By category:
Worse in all five categories, and worst in the two where precision matters most. Latency was measured separately and was the same either way, so there was no speed benefit to trade against the accuracy loss. Result held against both min-max references and in both cross-fold directions. What we are not claiming
Full protocol, per-arm outputs, and the noise-floor diagnostic are in Chunk-pooling bias: real, deferredFrom your "what I'd do next" list, this one is correct on the mechanism. We measured it on a live database, holding the document constant and sampling k of its 16 chunks against 400 unrelated queries:
Same content, same queries, none relevant. +0.09 cosine purely from more shots at the max. Confirmed, not theoretical. Deferring it for now, for three reasons:
Revisiting when there is a workload that actually shows it. Two adjacent defects we did find while investigating it are being fixed instead, since they have correct answers that do not depend on ranking quality: chunk writes are dropped on shutdown by an untracked background task, and stale chunk boundaries survive every migration. FTS-as-hard-gate, your third candidate, is the one still on the table. You were right that it is higher impact and riskier, and it is the more interesting question. What landed from this
Declining the fusion change, keeping the review. Genuinely useful work, and the separation of #113 into its own issue is what let that fix land independently. Thanks @Mxneeb. |
|
Closing unmerged. Full reasoning and benchmark data in the comment above. |
Fusion is unchanged across six versions and that was a decision, not an omission. Documents the weighted-RRF bake-off against #112: why the submitted change could not be measured as-is, the weighted variant built to isolate fusion from keyword weight, the 6.8 to 7.3 point loss across all five categories, and the 0.56 point noise floor the experiment established in place of the 0.1 previously assumed. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
2 participants
Summary
Replace min-max BM25 + weighted cosine fusion on the hybrid filter→rerank path with Reciprocal Rank Fusion (Cormack et al., k=60). Exact/lexical lanes and semantic-fallback-only scoring are unchanged.
_normalize_bm25and its FTS callers are untouched.Math
RRF(d) = sum of 1/(k + rank_r(d)) across the BM25-ranked and cosine-ranked lists, scaled by (k+1)/2, then blended with temporal decay exactly as before: final = (1-w)s + wt
Rank-based fusion avoids mixing an absolute cosine score with a per-query relative BM25 stretch, which previously let tight BM25 clusters amplify noise or one outlier crush mid-tier lexical signal.
Implemented in
_rrf_scores(memory_scoring.py) and the hybrid fusion block in_recall_similar(memory_recall.py). The obsoleteHYBRID_SEARCH_TEXT_WEIGHTsetting has been removed.Client note
Hybrid similarity magnitudes are now RRF-scaled, not a cosine/BM25 convex mix. Don't compare them to old thresholds without recalibration.
Known follow-up (tracked separately)
#113 —
find_semantic_duplicate()checksresults[0]["similarity"] >= CONSOLIDATION_THRESHOLD(default 0.92), but hybrid-pathsimilarityis RRF-scaled, not cosine.exact_mode="semantic"does not fix this: it still runs FTS-candidate + RRF fusion whenever FTS returns scoreable candidates (memory_recall.py~208–286). Proposed fix: preserve raw cosine (e.g.cosine_similarity) alongside the RRF ranking score so consolidation can gate on the correct quantity. Scope: consolidation only, gated behindCONSOLIDATION_ENABLED(default off).What I'd do next (other candidates considered)
_recall_similaronly scores the semantic corpus when FTS returns zero candidates. When FTS returns any candidates, semantically close paraphrases that share no/few tokens never enter the pool. Higher impact than the fusion fix but riskier, since it changes candidate generation itself, not just ranking._score_chunk_awarecollapses multi-chunk memories via max-over-chunks, which is statistically biased toward memories with more chunks and discards the parent/summary embedding entirely. A length-corrected max or logsumexp pooling, plus including the parent embedding as a competitor, would fix this cheaply.I picked the RRF fusion fix because it had the best effort-to-impact ratio: most surgical of the three, has a clean established mathematical justification rather than an ad-hoc formula, and carries the lowest risk since it doesn't touch candidate generation or lane semantics.
Summary by CodeRabbit
HYBRID_SEARCH_TEXT_WEIGHTfrom configuration/environment variable references and related changelog notes.