Skip to content

fix: isolate tokenizer truncation for concurrent encode calls - #385

Closed
tuanzirwar wants to merge 1 commit into
MinishLab:mainfrom
tuanzirwar:codex/isolate-concurrent-truncation
Closed

tuanzirwar wants to merge 1 commit into
MinishLab:mainfrom
tuanzirwar:codex/isolate-concurrent-truncation

Conversation

@tuanzirwar

Copy link
Copy Markdown

Concurrent calls on one StaticModel can overwrite each other's tokenizer truncation settings. A controlled interleaving of max_length=1 and max_length=3 returns the longer embedding for the short request, even though sequential encoding returns the expected result.

Use a request-local tokenizer when max_length overrides the model default, pass it through serial and joblib batches, and keep the default path reusing the original tokenizer. This covers encode and encode_as_sequence and avoids restoring shared mutable state after a request. Concurrent changes to model configuration are outside this change.

Validation: the reproduction failed before the fix and now matches sequential results; 411 non-integration tests passed on Windows/Python 3.14, including 18 new tests covering mixed encoding modes, truncation removal, threaded batches, tokenizer reuse, and exceptions. Ruff and diff checks passed. Mypy reports the same pre-existing numpy dtype assignment error on both upstream and modified source; no new type errors were reported. No hosted/pretrained model integration suite was run.

Prepared with Codex; the reproduction and tests above were executed locally.

@stephantul

Copy link
Copy Markdown
Contributor

Hello, we are aware of this and don't consider this an issue.

@stephantul stephantul closed this Oct 1, 2026
@greptile-apps

greptile-apps Bot commented Oct 1, 2026 •

Copy link
Copy Markdown

RetriggerConfidence Score: 4/5

[Medium risk] Refactors tokenizer state management for concurrent encoding.

The PR should not merge until encoding continues to honor public tokenize() overrides.

Reviews (1) · Last reviewed commit: "fix: isolate tokenizer truncation for co..."

Comment thread model2vec/model.py
) -> list[np.ndarray]:
"""Encode a batch of sentences as a sequence."""
ids = self.tokenize(sentences=sentences)
ids = self._tokenize(sentences, self.tokenizer if tokenizer is None else tokenizer)

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

P1 Tokenize overrides are bypassed If a StaticModel subclass overrides the public tokenize() method to customize token IDs, encode_as_sequence() now calls _tokenize() instead. encode() makes the same change, so both methods silently ignore the override and can return different embeddings than before.

Comment thread model2vec/model.py
Comment on lines +283 to +285
if max_length != self.max_length:
# 默认长度复用只读 tokenizer,仅显式覆盖时复制一次供所有批次使用。
tokenizer = copy.deepcopy(tokenizer)

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

P2 Default sequence calls copy tokenizer encode_as_sequence() defaults to max_length=None, which differs from a model's usual non-None default. As a result, every such call deep-copies the tokenizer, even for a small request, adding repeated copying cost to sequence encoding.

Note: If this suggestion doesn't match your team's coding style, reply to this and let me know. I'll remember it for next time!

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

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants