Skip to content

fix(embedder): safe 3072-token default + token-aware truncation - #2190

Open
RerankerGuo wants to merge 1 commit into
MemTensor:mainfrom
RerankerGuo:fix/issue-2121-embedding-3072-token-truncate
Open

fix(embedder): safe 3072-token default + token-aware truncation#2190
RerankerGuo wants to merge 1 commit into
MemTensor:mainfrom
RerankerGuo:fix/issue-2121-embedding-3072-token-truncate

Conversation

@RerankerGuo

Copy link
Copy Markdown
Contributor

Background

Issue #2121 reports that memos-local-plugin ingestion passes source text directly to the embedding API, and text-embedding-3 enforces a hard 3072-token input cap. Previously, the embedder's default max_tokens was 8192 (which exceeds the API limit) and the truncation was a plain character-slice that did not account for token counts at all. This caused out-of-budget embedding errors for longer documents and knowledge chunks.

Changes

  • memos/embedders/base.py:
    • Added _SAFE_EMBEDDING_MAX_TOKENS = 3072 matching text-embedding-3's input limit.
    • Added BaseEmbedder._effective_max_tokens(): returns an explicitly configured positive max_tokens, otherwise the 3072 safe default (also used when the user configures max_tokens=None or max_tokens=0).
    • Rewrote _truncate_texts() to delegate to the existing token-aware _truncate_text_to_tokens() binary-search helper. A fast-path length check keeps the cheap case cheap (text length <= max tokens means no token count required).
  • memos/configs/embedder.py:
    • Changed BaseEmbedderConfig.max_tokens default from 8192 to None so that the new 3072 safe-limit takes effect for any user not overriding the field explicitly; the docstring was updated.
  • tests/embedders/test_base.py:
    • Added coverage for _effective_max_tokens() behaviour across None, 0, and explicit positive values.
    • Added coverage for _truncate_text_to_tokens(): short text unchanged, empty/None/0 boundaries, long CJK text stays within budget.
    • Added coverage for _truncate_texts(): short texts untouched, very long CJK text truncated within the 3072-token default, explicit 10-token override honoured.

Verification

  • Ruff: ruff format + ruff check --fix applied (2 pre-existing BLE001 broad-excepts in _count_tokens_for_embedding left intact).
  • python3 -m py_compile succeeds for modified files.

Impact

  • Backwards compatible for callers that set an explicit max_tokens; behaviour changes only when the field was left at the previous 8192 default.
  • Eliminates the silent "send 8192 chars and hope for the best" behaviour for text-embedding-3 endpoints; the default now stays inside the API budget.

…3072-token default

- embedders/base.py: add _SAFE_EMBEDDING_MAX_TOKENS=3072, new
  BaseEmbedder._effective_max_tokens() fallback, and make
  _truncate_texts() token-aware by calling _truncate_text_to_tokens
  (binary search over prefix length) instead of a naive character-slice.
- configs/embedder.py: BaseEmbedderConfig.max_tokens default changed
  from 8192 to None so the new 3072-token safe-limit kicks in for
  providers such as text-embedding-3 that enforce a hard input cap.
- tests/embedders/test_base.py: new tests covering effective_max_tokens
  for None/0/explicit overrides, _truncate_text_to_tokens bounds on
  long CJK text, and _truncate_texts with the new 3072 default.
Closes MemTensor#2121.
@Memtensor-AI Memtensor-AI added area:core MOS 编排层 / 框架底座 / 跨模块问题 area:model llm + embedder + reranker status:in-progress Someone or AI is working on it | 人工或 AI 正在处理 labels Jul 31, 2026
@Memtensor-AI

Copy link
Copy Markdown
Collaborator

🤖 Open Code Review

Target: PR #2190
Task: bc3c30a4c517a246
Base: main
Head: fix/issue-2121-embedding-3072-token-truncate

🔍 OpenCodeReview found 4 issue(s) in this PR.


1. src/memos/configs/embedder.py (L17)

The description says Set to 0 ... to explicitly disable ... truncation, but _effective_max_tokens() in base.py treats max_tokens=0 (or any non-positive value) as "not configured" and falls back to the safe default of 3072 tokens — truncation is NOT disabled. Setting max_tokens=0 therefore silently behaves the same as None. The description misleads users who may set 0 expecting no truncation.

💡 Suggested Change

Before:

        description="Maximum number of tokens per text. When None (default) the embedder falls back to a provider-aware safe limit (currently 3072 tokens, matching text-embedding-3). Set to 0 or a concrete positive integer to explicitly disable or override truncation.",

After:

        description="Maximum number of tokens per text. When None (default) the embedder falls back to a provider-aware safe limit (currently 3072 tokens, matching text-embedding-3). Set to a positive integer to override truncation. Values of 0 or None both use the safe default.",

2. src/memos/embedders/base.py (L171-L173)

The fast-path condition len(t) <= max_tokens is semantically unsound. It compares character length against a token budget, but character count and token count have no reliable 1:1 relationship. For ASCII text, one token is roughly 4 characters, so a 3072-character text would comfortably fit within 3072 tokens — yet the fast-path skips it correctly only by accident. More critically, the comment says "~1 char/token for CJK text is the worst case", which is true, but the fast-path is supposed to guarantee no truncation is needed: a text of exactly max_tokens characters could exceed max_tokens tokens only if every character is a CJK character, in which case len(t) == max_tokens is exactly at the limit and is safe. However, in the boundary case where len(t) == max_tokens and all characters are CJK, the token count equals max_tokens exactly, so appending without checking is still correct.

The real issue is the opposite direction: a text with len(t) > max_tokens but few CJK characters (e.g., ASCII prose) is unconditionally sent to _truncate_text_to_tokens, even though it almost certainly fits within the token budget. This is a performance issue (unnecessary binary search + tiktoken calls) but not a correctness issue since _truncate_text_to_tokens returns the original text unchanged when current_tokens <= max_tokens.

A more accurate fast-path would be len(t) <= max_tokens // 4 (guaranteeing safety for pure ASCII at ~4 chars/token) combined with falling through to the token counter for longer texts, which avoids both false negatives and unnecessary truncation calls. Consider:

if len(t) <= max_tokens // 4:
    truncated.append(t)
    continue

3. src/memos/embedders/base.py (L191)

Changing max_tokens default from 8192 to None is a breaking behavioral change for all existing embedder subclasses and deployed configurations. Previously, max_tokens=None meant "disable truncation" (the old _truncate_texts returned early when config.max_tokens is None). Now, max_tokens=None triggers the fallback to _SAFE_EMBEDDING_MAX_TOKENS = 3072. Any existing caller relying on max_tokens=None to disable truncation will now silently get 3072-token truncation instead, which could silently shorten text that previously passed through unmodified. The config description says "Set to 0 ... to explicitly disable", but zero is also treated as "use the safe default" in _effective_max_tokens (condition: configured > 0 falls through to return _SAFE_EMBEDDING_MAX_TOKENS). There is no way to disable truncation entirely via the config anymore.

💡 Suggested Change

Before:

        return _SAFE_EMBEDDING_MAX_TOKENS

After:

        # Allow callers to opt out of truncation by setting max_tokens to a
        # sentinel negative value, e.g. -1.
        if configured is not None and configured < 0:
            return None  # truncation disabled
        if configured is not None and configured > 0:
            return configured
        return _SAFE_EMBEDDING_MAX_TOKENS

4. tests/embedders/test_base.py (L63-L68)

The assertion len(truncated) >= limit (500) is fragile when tiktoken is installed. In cl100k_base, CJK characters are not always 1 token per character — many encode as 2–3 tokens. With a 500-token budget and tiktoken active, the binary-search truncation could produce a string with fewer than 500 characters, causing this assertion to fail in CI environments where tiktoken is available.

Either drop the lower-bound length assertion (the token-count assertion already proves correct truncation), or loosen it to account for multi-token CJK encoding (e.g., len(truncated) >= limit // 3).

💡 Suggested Change

Before:

    def test_long_cjk_text_truncates_within_budget(self):
        text = "记忆内容测试" * 1000  # ~6000 chars -> ~6000 tokens with simple heuristic
        limit = 500
        truncated = _truncate_text_to_tokens(text, limit)
        assert _count_tokens_for_embedding(truncated) <= limit
        assert len(truncated) >= limit

After:

    def test_long_cjk_text_truncates_within_budget(self):
        text = "记忆内容测试" * 1000  # ~6000 chars -> ~6000 tokens with simple heuristic
        limit = 500
        truncated = _truncate_text_to_tokens(text, limit)
        assert _count_tokens_for_embedding(truncated) <= limit
        # Length lower-bound depends on tokenizer: tiktoken may encode CJK as
        # multiple tokens, so use a conservative bound rather than 1 char/token.
        assert len(truncated) >= limit // 3

Generated by cloud-assistant via Open Code Review.

@Memtensor-AI

Copy link
Copy Markdown
Collaborator

⚠️ Automated Test Results: INCONCLUSIVE

Automated tests inconclusive (auto-generated test defect); treated as non-blocking. Manual review recommended. Details: The AI-generated test file defines a _ConcreteEmbedder subclass of BaseEmbedder but fails to implement the abstract __init__ method, causing all instantiation-based tests to fail with TypeError. Additionally, one CJK truncation test has an off-by-one assertion that expects the truncated length to be >= the token limit, but token-aware truncation correctly produces a length <= limit. [advisory, non-gating] AI-generated tests on branch test/auto-gen-bc3c30a4c517a246-20260731105109: 24/65 passed, 41 failed — these do NOT affect the PR verdict; review the branch manually.

Branch: fix/issue-2121-embedding-3072-token-truncate

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

area:core MOS 编排层 / 框架底座 / 跨模块问题 area:model llm + embedder + reranker status:in-progress Someone or AI is working on it | 人工或 AI 正在处理

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants