Repository navigation
fix(mem_reader): stop duplicating parent sources on every split chunk (#2453) - #2488
RerankerGuo wants to merge 2 commits into
Conversation
🤖 Open Code ReviewTarget: PR #2488 🔍 OpenCodeReview found 2 issue(s) in this PR. 1.
|
✅ Automated Test Results: PASSEDAll tests passed (3/3 executed). memos_python_core/changed-repo-python: 3/3. Duration: 8s Branch: |
|
Two changes pushed in 7bfc73b. Build failure. OpenCodeReview finding 2. OpenCodeReview finding 1 — keeping role/type provenance on the chunks — is not addressed by this push: the chunks still carry no sources, so that part is still open for your call. |
…MemTensor#2453) MultiModalStructMemReader._split_large_memory_item passed `sources=item.metadata.sources or []` to _make_memory_item for every chunk, so each chunk item carried the full list of source messages from the parent. With N chunks and a parent carrying N original SourceMessages, every chunk ended up with N byte-identical full-text SourceMessage objects — a multiplicative duplication of memory and graph size that scaled with the square of how chunked a message was. Empirically from the issue: one 1,456-character message split into 4 chunks produced 15 memory items, all with sources length 4 and all 4 elements byte-identical (length [1456, 1456, 1456, 1456], identical MD5). 27.8% of memories in a sampled 320-memory production instance showed the same bloat. Chunks are derived fragments, not original messages, so they should not inherit the parent's sources list. Provenance is already preserved through `internal_info.chunk_index / chunk_total / ingest_batch_id` (set immediately after _make_memory_item), which gives downstream consumers the same reconstruction ability without the duplication. The fix drops the sources= argument from the _make_memory_item call in _split_large_memory_item. _make_memory_item's default is sources=None, which is normalised to [] inside TextualMemoryMetadata, so chunks now carry no sources (the same as if they had been created fresh from the raw chunk text). - New tests/mem_reader/test_split_sources_no_duplication.py with three red/green cases: chunks must not carry parent sources (red against the unpatched code); chunks must still carry internal_info provenance; empty parent sources still produce empty chunk sources. - The parent's sources are preserved on the parent (no change there), so downstream aggregation paths that read the parent's sources continue to work. Fixes MemTensor#2453.
… items Dropping the sources kwarg entirely broke the _make_memory_item call-site contract: callers intercepting the keyword with a strict signature (e.g. the existing project-id propagation test whose fake requires sources=) fail, and the broad except in _split_large_memory_item turns the TypeError into a silent fall-back to the unsplit item, so large items stopped being chunked at all. Passing an explicit empty list keeps the MemTensor#2453 no-duplication behaviour identical while restoring the stable kwarg contract. Also drop the vacuous no-parent-sources test case: with an empty parent sources list the old and new code both produced sources == [], so the assertion could never distinguish the bug from the fix. The non-vacuous test_split_chunks_do_not_carry_parent_sources already pins the invariant with a parent that does carry sources.
7bfc73b to
b4087c7
Compare
✅ Automated Test Results: PASSEDAll tests passed (2/2 executed). memos_python_core/changed-repo-python: 2/2. Duration: 23s Branch: |
Summary
MultiModalStructMemReader._split_large_memory_itempassedsources=item.metadata.sources or []to_make_memory_itemfor every chunk. With N chunks and a parent carrying N originalSourceMessages, every chunk ended up with N byte-identical full-textSourceMessageobjects — a multiplicative duplication of memory and graph size that scaled with the square of how chunked a message was.Root cause
src/memos/mem_reader/multi_modal_struct.pyline 169 (pre-fix):Empirical reproduction from the issue: one 1,456-character message split into 4 chunks produced 15 memory items, all with
sourceslength 4 and all 4 elements byte-identical (length[1456, 1456, 1456, 1456], identical MD5). In a sampled 320-memory production instance, 27.8% of memories showed the same bloat.Why chunks must not carry the parents sources
Chunks are derived fragments, not original messages. The parent already carries the full source list, so propagating it onto every chunk multiplies the list without adding provenance. Chunk-level provenance is already preserved separately on
metadata.internal_infoviachunk_index/chunk_total/ingest_batch_id(set immediately after the_make_memory_itemcall), which gives downstream consumers the same reconstruction ability without the duplication.Fix
Drop the
sources=argument from the_make_memory_itemcall in_split_large_memory_item._make_memory_items default issources=None, whichTextualMemoryMetadatanormalises to[], so chunks now carry no sources — the same as if they had been created fresh from the raw chunk text.The parents sources remain unchanged, so downstream aggregation paths that read the parents sources (e.g.
_build_aggregated_nodewhich iteratesitem.metadata.sourcesto extract roles) continue to work.Tests
New
tests/mem_reader/test_split_sources_no_duplication.py, three red/green cases:test_split_chunks_do_not_carry_parent_sources— red against unpatchedmain, green with the fixtest_split_chunks_preserve_provenance_via_internal_info— pins the chunk provenance contract viainternal_info.chunk_index / chunk_total / ingest_batch_idtest_split_with_no_parent_sources_yields_no_sources— sanity check for the empty-sources pathVerification
Ruff:
ruff format --check: cleanruff check: 0 new findings (originalmainhad 15 BLE001 errors in this file; my change introduces none)Out of scope
Closes #2453