Fix #2453: [Bug] sources array duplicates the full original message N times (N = chunk coun - #2454
Open
Memtensor-AI wants to merge 2 commits into
Open
Memtensor-AI wants to merge 2 commits into
Memtensor-AI wants to merge 2 commits into
Conversation
…2453) MultiModalStructMemReader._split_large_memory_item was attaching the parent item's full `sources` list to every chunk, and _build_window_from_items then concatenated them without dedup. The result: a message split into N chunks produced memory items whose `sources` list held N byte-identical copies of the original message. Production data showed 27% of memories affected, bloating storage, retrieval context (advanced_searcher re-injects sources into prompts verbatim), and provenance tooling. Fix is two-layer: * In _split_large_memory_item, attach `sources` only to chunk 0 (the owning chunk) and record source_chunk_index=0 in internal_info. Non-owning chunks keep ingest_batch_id + chunk_index + chunk_total for lineage discoverability. * In _build_window_from_items, dedup `all_sources` by a stable (type, role, message_id, chat_time, doc_path, content) signature so future regressions or multi-parser overlap also cannot inflate the list. Added 8 regression tests in tests/mem_reader/test_sources_dedup.py covering the propagation fix, the dedup fix, role-detection preservation, and an end-to-end _concat_multi_modal_memories invariant. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Memtensor-AI
added
ai:generated
3 tasks done
Collaborator
Author
🤖 Open Code ReviewTarget: PR #2454 ✅ OpenCodeReview: Review complete: 0 finding(s) across 2 selected item(s). Generated by cloud-assistant via Open Code Review. |
Collaborator
Author
🔧 Open Code Review requested Agent fixOpen Code Review found 4 issue(s). I have resumed the development Agent to fix them.
The Agent will push a new commit to this PR branch. OCR will recheck after the commit is pushed. |
Four follow-ups from the open code review on MemTensor#2454: 1. `_source_signature` now folds in all non-None keys returned by `model_dump` instead of only the six known fields. SourceMessage declares `extra="allow"`, so two paragraphs from the same document that differ only in an extra locator (page, offset, span, …) and share `message_id=None` would otherwise collapse incorrectly after dedup. Unhashable extras fall back to object identity so we never silently merge distinct sources. 2. `test_long_message_produces_windows_with_single_source` additionally asserts that at least one window actually carries the owning source. The pre-existing `≤ 1` check alone would pass a buggy implementation that dropped the source entirely. 3. `test_split_bails_before_marking_when_only_one_chunk` now matches the implementation: chunk 0 is the owning chunk even when chunk_total==1, so `source_chunk_index=0` is set and the test asserts it. Docstring updated. 4. `test_window_role_detection_still_works_after_dedup` additionally asserts `len(window.metadata.sources) == 1` so a dedup regression that strips every source cannot slip through on role defaulting. Also adds `test_window_keeps_sources_distinguished_by_extra_fields` to guard the first fix against future regressions. Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>
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.
Description
Fixes issue #2453: sources array duplicated per chunk in MultiModalStructMemReader.
Root cause:
_split_large_memory_itemattached the parentsourceslist to every chunk (multi_modal_struct.py:169), and_build_window_from_itemsthen concatenated them without dedup, so a message split into N chunks produced memory items whosesourceslist carried N byte-identical copies of the original message. The reporter measured 27% of production memories affected.Fix is two-layer: (1) attach
sourcesonly to chunk 0 (the "owning" chunk) with an additionalinternal_info["source_chunk_index"] = 0marker; (2) dedupall_sourcesin_build_window_from_itemsby a stable (type, role, message_id, chat_time, doc_path, content) signature for defense-in-depth. Lineage keys (ingest_batch_id/chunk_index/chunk_total) are preserved on every chunk.Testing: 8 new regression cases in tests/mem_reader/test_sources_dedup.py (4 reproduced the bug on pristine HEAD; all 8 pass after the fix). Full tests/mem_reader/ suite: 71 pass. Pre-existing failures from a missing
markitdowndependency and a torch/transformersDynamicCachecompat issue intests/memories/activation/test_kv.pyare unrelated (verified on pristine HEAD). Ruff check + format: clean.Related Issue (Required): Fixes #2453
Type of change
Please delete options that are not relevant.
How Has This Been Tested?
Not run; documentation-only change.
Checklist
@WeiminLee please review this PR.
Reviewer Checklist