fix(memory): keep the best duplicate score in recall dedup, not the first - #6748
fix(memory): keep the best duplicate score in recall dedup, not the first#6748LHMQ878 wants to merge 2 commits into
Conversation
…irst `synthesize_results` deduplicated by `record.id` keeping the first occurrence, but `_do_search` collects findings in `as_completed` order, so thread timing decided which of a duplicate's scores survived. Duplicates are structural, not incidental: `lancedb_storage.search` matches `scope LIKE 'prefix%'`, so a record at `/a/b` is returned for scope `/a` and for `/a/b`, and each of the up-to-3 sub-query embeddings scores it differently. `compute_composite_score` is monotonic in the semantic score, so keeping the first-arriving one can keep a strictly worse score, under-rank the record, or drop it out of `matches[: limit]`. Keep the best score per record id instead, and break sort ties on `record.id` so equal-scoring records do not order by arrival either. `state.confidence` already used `max()` over all findings and is unchanged. Closes crewAIInc#6747
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
📝 WalkthroughWalkthrough
ChangesMemory Recall Deduplication
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches🧪 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 |
![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
🤖 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 `@lib/crewai/src/crewai/memory/recall_flow.py`:
- Around line 367-380: Freeze a single scoring timestamp in the synthesis used
by test_equal_scores_rank_by_record_id_rather_than_arrival so
compute_composite_score applies identical recency decay to both records,
allowing the record.id tiebreaker assertion to remain deterministic. Update
lib/crewai/src/crewai/memory/recall_flow.py lines 367-380 only as needed to
support reusing that timestamp, and apply the corresponding test setup in
lib/crewai/tests/memory/test_recall_flow_dedup.py lines 88-104; preserve the
existing ordering assertion.
- Around line 367-369: Update the score-processing path around
compute_composite_score in lib/crewai/src/crewai/memory/recall_flow.py:367-369
to catch TypeError and ValueError during score conversion, skip non-finite
converted scores, and continue recall without aborting. Add invalid-score
entries before the valid record in
lib/crewai/tests/memory/test_recall_flow_dedup.py:138-149 and assert the valid
match remains available.
🪄 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: Organization UI
Review profile: CHILL
Plan: Pro Plus
Run ID: dfbe0a4e-4b18-46cb-ba9d-b02f74883e2e
📒 Files selected for processing (2)
lib/crewai/src/crewai/memory/recall_flow.pylib/crewai/tests/memory/test_recall_flow_dedup.py
| composite, reasons = compute_composite_score( | ||
| record, float(score), self._config | ||
| ) |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟡 Minor | ⚡ Quick win
Skip invalid numeric scores without aborting recall.
A valid MemoryRecord paired with None or "invalid" reaches float(score) and raises, so one malformed entry prevents synthesis instead of being skipped.
lib/crewai/src/crewai/memory/recall_flow.py#L367-L369: catchTypeError/ValueErroraround score conversion and skip non-finite values.lib/crewai/tests/memory/test_recall_flow_dedup.py#L138-L149: include invalid-score entries before the valid record and assert the valid match remains available.
Proposed fix
+from math import isfinite
+
- composite, reasons = compute_composite_score(
- record, float(score), self._config
- )
+ try:
+ semantic_score = float(score)
+ except (TypeError, ValueError):
+ continue
+ if not isfinite(semantic_score):
+ continue
+ composite, reasons = compute_composite_score(
+ record, semantic_score, self._config
+ )📝 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.
| composite, reasons = compute_composite_score( | |
| record, float(score), self._config | |
| ) | |
| try: | |
| semantic_score = float(score) | |
| except (TypeError, ValueError): | |
| continue | |
| if not isfinite(semantic_score): | |
| continue | |
| composite, reasons = compute_composite_score( | |
| record, semantic_score, self._config | |
| ) |
📍 Affects 2 files
lib/crewai/src/crewai/memory/recall_flow.py#L367-L369(this comment)lib/crewai/tests/memory/test_recall_flow_dedup.py#L138-L149
🤖 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 `@lib/crewai/src/crewai/memory/recall_flow.py` around lines 367 - 369, Update
the score-processing path around compute_composite_score in
lib/crewai/src/crewai/memory/recall_flow.py:367-369 to catch TypeError and
ValueError during score conversion, skip non-finite converted scores, and
continue recall without aborting. Add invalid-score entries before the valid
record in lib/crewai/tests/memory/test_recall_flow_dedup.py:138-149 and assert
the valid match remains available.
| composite, reasons = compute_composite_score( | ||
| record, float(score), self._config | ||
| ) | ||
| previous = best.get(record.id) | ||
| if previous is None or composite > previous.score: | ||
| best[record.id] = MemoryMatch( | ||
| record=record, | ||
| score=composite, | ||
| match_reasons=reasons, | ||
| ) | ||
| matches.sort(key=lambda m: m.score, reverse=True) | ||
| matches: list[MemoryMatch] = list(best.values()) | ||
| # ``record.id`` breaks ties, so equal-scoring records do not order by | ||
| # which task happened to finish first either. | ||
| matches.sort(key=lambda m: (-m.score, m.record.id)) |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
rg -n -C 12 'def compute_composite_score\b|datetime\.(now|utcnow)\(|time\.time\(' \
lib/crewai/src/crewai
rg -n -C 6 'test_equal_scores_rank_by_record_id|monkeypatch|freeze_time|freezegun' \
lib/crewai/tests/memory/test_recall_flow_dedup.pyRepository: crewAIInc/crewAI
Length of output: 50373
🏁 Script executed:
#!/bin/bash
set -euo pipefail
printf '--- recall_flow key regions ---\n'
sed -n '320,390p' lib/crewai/src/crewai/memory/recall_flow.py
printf '\n--- dedup test region ---\n'
sed -n '1,130p' lib/crewai/tests/memory/test_recall_flow_dedup.py
printf '\n--- locate MemoryConfig / compute_composite_score usages ---\n'
rg -n 'MemoryConfig|compute_composite_score|recency_half_life_days|recency_weight|relevance_weight' lib/crewai/src/crewai lib/crewai/tests | head -200Repository: crewAIInc/crewAI
Length of output: 245
🏁 Script executed:
#!/bin/bash
set -euo pipefail
printf '%s\n' '--- recall_flow key regions ---'
sed -n '320,390p' lib/crewai/src/crewai/memory/recall_flow.py
printf '%s\n' '
--- dedup test region ---'
sed -n '1,130p' lib/crewai/tests/memory/test_recall_flow_dedup.py
printf '%s\n' '
--- locate MemoryConfig / compute_composite_score usages ---'
rg -n 'MemoryConfig|compute_composite_score|recency_half_life_days|recency_weight|relevance_weight' lib/crewai/src/crewai lib/crewai/tests | head -200Repository: crewAIInc/crewAI
Length of output: 12823
🏁 Script executed:
#!/bin/bash
set -euo pipefail
printf '%s\n' '--- recall_flow key regions ---'
sed -n '320,390p' lib/crewai/src/crewai/memory/recall_flow.py
printf '%s\n' '--- dedup test region ---'
sed -n '1,130p' lib/crewai/tests/memory/test_recall_flow_dedup.py
printf '%s\n' '--- locate MemoryConfig/composite usages ---'
rg -n 'MemoryConfig|compute_composite_score|recency_half_life_days|recency_weight' lib/crewai/src/crewai lib/crewai/tests | head -200Repository: crewAIInc/crewAI
Length of output: 12805
Freeze the recency timestamp in test_equal_scores_rank_by_record_id_rather_than_arrival.
compute_composite_score calls datetime.utcnow() for the recency decay, so otherwise identical semantic scores can drift apart depending on processing order before the record.id tiebreaker is reached. Freeze one scoring timestamp for the synthesis, or replace this assertion with a monotonicity check that tolerates small timing drift.
📍 Affects 2 files
lib/crewai/src/crewai/memory/recall_flow.py#L367-L380(this comment)lib/crewai/tests/memory/test_recall_flow_dedup.py#L88-L104
🤖 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 `@lib/crewai/src/crewai/memory/recall_flow.py` around lines 367 - 380, Freeze a
single scoring timestamp in the synthesis used by
test_equal_scores_rank_by_record_id_rather_than_arrival so
compute_composite_score applies identical recency decay to both records,
allowing the record.id tiebreaker assertion to remain deterministic. Update
lib/crewai/src/crewai/memory/recall_flow.py lines 367-380 only as needed to
support reusing that timestamp, and apply the corresponding test setup in
lib/crewai/tests/memory/test_recall_flow_dedup.py lines 88-104; preserve the
existing ordering assertion.
…flaky The tie assertion added by this PR raced the very thing it asserts against. `compute_composite_score` reads `datetime.utcnow()` per call for the recency decay, so two records created at the same instant do not score identically: whichever is scored first is read a few hundred nanoseconds younger, gets a larger decay, and wins a comparison the `record.id` tiebreaker is supposed to decide. Measured on the two-record shape the test uses, 6 of 2000 pairs differed, every one of them favouring the record scored first. That direction is what makes it flaky rather than simply wrong. The test asserts `["aaa", "bbb"]` for both the forward and the reversed finding order, so the forward case passes for the wrong reason -- `aaa` is scored first and wins on drift, not on id -- while the reversed case is the one that breaks. Running the synthesize loop 20000 times: forward 0 failures, reversed 200 (1.0%). Fixed at the test layer, since the product behaviour is correct: sub-nanosecond recency drift is meaningless at a 30-day half-life and the sort already has a deterministic tiebreaker. A `_frozen_clock` fixture substitutes a `datetime` subclass whose `utcnow` returns a fixed instant one day after `_FIXED_CREATED_AT`, so the decay term is still exercised rather than zeroed. Under the frozen clock the difference between the two composites is exactly `0.0` over 50000 pairs, and the frozen composite matches the value computed by hand for a one-day age (`0.643147990530`), which is what shows the patch is in effect rather than silently a no-op. 40 consecutive runs of the file pass. `_record`'s docstring also claimed that pinning `created_at` and `importance` made the composite a function of the semantic score alone. It does not -- the wall clock is the third input -- so that is corrected too.
|
Two findings, and they land differently. The flakiness one is right and was mine — fixed in 1. Recency-drift flakiness in the tie test — confirmed, fixedRight, and worse than "can drift apart": the drift has a direction, which is what makes it a flake rather than simply noise. Measured on the exact two-record shape the test builds: So the test passes for the wrong reason in the forward case — Fixed at the test layer rather than in the product: sub-nanosecond drift is meaningless at a 30-day half-life, and the sort already carries a deterministic tiebreaker — the test was just failing to hold the clock still long enough to reach it. A Two checks that the fixture is actually in effect, not silently a no-op: Exactly I also corrected 2. Invalid numeric scores — declining, with the reasonThe hazard is real but the suggested guard does not prevent the abort, because the abort happens earlier than the patched line. top_composite, _ = compute_composite_score(results[0][0], results[0][1], self._config)Measured against that call: Neither is caught — the surrounding And nothing in tree can produce one. The
So Happy to open it separately as a
|
1 participant
Closes #6747
Problem
RecallFlow.synthesize_resultsdeduplicated byrecord.idkeeping the first occurrence, but_do_searchappends findings insidefor future in as_completed(futures)— i.e. in thread-completion order. So when the same record arrived from more than one(sub-query, scope)task, thread timing decided which score survived.Duplicates are structural rather than incidental:
lancedb_storage.searchfilters withquery.where(f"scope LIKE '{prefix}%'"), so a record stored at/a/bis returned for scope/aand/a/b— andfilter_and_chunkroutinely selects both.analyze_query_stepyields up to 3 sub-query embeddings and_do_searchruns the fullembeddings × scopescross product, so the same record arrives carrying different semantic scores.compute_composite_scoreis monotonic insemantic_score, so keeping the first-arriving duplicate can keep a strictly worse score — under-ranking the record, or dropping it out ofmatches[: self.state.limit]. This reaches users throughUnifiedMemory(flow.state.final_results→touch_records→MemoryQueryCompletedEvent).A second, smaller dependence:
sort(key=..., reverse=True)is stable, so equal-scoring records kept their arrival order too.Fix
Keep the best score per
record.idinstead of the first, and break sort ties onrecord.id:Best-wins is both order-independent and the score ranking should have been using. Behaviour that already worked is untouched: dedup still collapses duplicates to one entry, ranking is still by descending composite,
limitis still applied last, and the defensive shape checks (non-dict finding, non-listresults, short tuple, non-MemoryRecord) are all preserved.state.confidenceis deliberately unchanged —max((f["top_score"] for f in findings), default=0.0)is already order-independent.Tests
New
lib/crewai/tests/memory/test_recall_flow_dedup.py, 6 tests. Each drivessynthesize_resultson a hand-builtchunk_findingslist rather than through a thread pool, so the assertions pin the dedup rule itself instead of racing the scheduler.test_duplicate_record_keeps_the_best_score_not_the_first_seen[high, low]and[low, high]both resolve to the higher scoretest_equal_scores_rank_by_record_id_rather_than_arrivalrecord.idin both a forward and a reversed findings listtest_record_reachable_from_several_scopes_appears_oncetest_distinct_records_are_all_kept_and_ranked_by_scoretest_limit_is_applied_after_deduplicationlimitslotstest_malformed_findings_are_skipped_without_raisingThe composite carries a recency term read from the wall clock at scoring time, so the score assertions compare with
pytest.approx(..., abs=1e-6)against separately-scored baselines — a tolerance three orders of magnitude below the 0.9/0.1 gap being asserted — rather than against hard-coded floats.Control run — with
git checkout origin/main -- lib/crewai/src/crewai/memory/recall_flow.pyand the new test file in place:So both new assertions fail on unfixed source, and the four guarding existing behaviour pass on it — they are not tautologies.
Suite comparison —
lib/crewai/tests/memory/before and after, comparing failing test names:Checks
ruff checkon both files:All checks passed!ruff format --checkon the source file: already formatted (lib/crewai/tests/is ruff-excluded)