Repository navigation
Fix #2461: [Bug] Content between embedder limit and chat_window_max_tokens is silently stor - #2462
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
Items whose token count fell in (embedder limit, chat_window_max_tokens] were neither split nor embeddable, so /product/add returned 200 while the item was stored with metadata.embedding=None and vector_sync != "success", permanently invisible to semantic search (issue MemTensor#2461). - expose chat_window_max_tokens as an explicit, env-configurable field on BaseMemReaderConfig (MEM_READER_CHAT_WINDOW_MAX_TOKENS), default 1024 - derive the split budget as min(window, embedder per-item limit) - re-check chunker output and hard-split any over-budget chunk, preferring a punctuation boundary and falling back to a binary-search character window; a chunker that fails or yields nothing now hard-splits instead of passing the over-budget item through - truncate to budget before the per-item embedding retry so one over-limit item cannot leave a whole batch without vectors
Memtensor-AI
added
ai:generated
4 tasks done
Collaborator
Author
🤖 Open Code ReviewTarget: PR #2462 ✅ OpenCodeReview: Review complete: 0 finding(s) across 6 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. |
Addresses the review findings on the MemTensor#2461 fix: - multi_modal_struct: the window loop and the overlap trim still called `_count_tokens` directly, so a reader built without a live tokenizer made its budget decision with `_count_tokens_safe` and then raised `AttributeError` a few lines later, dropping the whole scene. Both call sites now use the safe counter. - multi_modal_struct: `_chunk_within_budget` wrapped the per-chunk token check and hard split in the same `try` as the chunker, so a tokenizer error was reported as "Chunker failed" and the already-validated chunks were thrown away. The guard now covers only `chunker.chunk(text)`. - simple_struct: `_find_hard_split_index` could return `best + 1`, letting a terminator just past the budget be kept on the left side. That made the reported prefix `budget + 1` tokens; `_truncate_to_budget` used it directly, so the last guard before `embedder.embed([payload])` could still hand the provider an over-limit payload — the exact rejection this change set out to remove. The scan now stays inside the fitting prefix. - simple_struct: drop the unreachable range guard in the scan loop.
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
3 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
Fixed the silent no-vector write for memory items whose token count falls in (embedder per-item limit, chat_window_max_tokens] (issue #2461). Such an item was previously neither split (the splitter only fired above chat_window_max_tokens, hard-coded to 1024 and never passed by either api/config.py mem_reader assembly site) nor embeddable (it exceeded the embedder's limit). /product/add still returned 200, but the item was persisted with metadata.embedding=None and vector_sync != "success", and search_by_embedding filters on vector_sync == "success" — so the memory was permanently invisible to semantic search. A secondary cause: _split_large_memory_item trusted chunker output and never re-checked the token budget per chunk, and the sentence chunker (chonkie) does not split CJK punctuation (it can also raise on an incompatible installed version), in which case the except branch returned the over-budget item unchanged.
The fix adds an explicit, env-configurable chat_window_max_tokens field on BaseMemReaderConfig (MEM_READER_CHAT_WINDOW_MAX_TOKENS, default 1024 so existing deployments are unchanged), wired into all three mem_reader assembly sites via APIConfig.get_chat_window_max_tokens() with fault-tolerant parsing. The effective split budget is now min(chat_window_max_tokens, embedder per-item limit). A new _chunk_within_budget helper re-checks every chunker-produced chunk and hard-splits any that still exceeds the budget — preferring a punctuation boundary (CJK and Latin terminators) and falling back to a binary-search character window — and a chunker that raises or yields nothing now hard-splits instead of passing the over-budget item through. _concat_multi_modal_memories uses the effective budget for both the split trigger and the window accumulator, plus a guard that emits over-budget items directly rather than aggregating them into a larger window. Finally, the per-item embedding retry (and the base reader's _make_memory_item path) truncates to budget before calling the embedder, so one over-limit item can no longer leave a whole batch without vectors.
Verification: a new 18-case regression suite (tests/mem_reader/test_embed_budget_split.py) passes, covering budget derivation, punctuation-free CJK splitting within budget, boundary preference, empty/failed chunker fallback, contiguous re-indexing, the 700-token in-window case, and the mixed-batch embedding fallback. Affected suites (mem_reader, configs, api, memories, mem_cube) report 341 passed with 4 pre-existing failures confirmed on the untouched baseline (markitdown extra not installed; unrelated kv-cache API). ruff check and ruff format both clean. An end-to-end reproduction using the real SentenceChunker and tiktoken counter with a 512-token embedder limit turns the issue's 780-token Chinese paragraph into chunks of 512 + 268 tokens, all embedded, while the same script's pre-fix branch reproduces the reported embedded=[False].
Out of scope: backfilling vectors for memories already stored without one.
Related Issue (Required): Fixes #2461
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