Repository navigation
[Bugfix] Honor code_revision in config-based tokenizer loading - #61611
Conversation
Propagate model code revisions through both config helpers only when model and tokenizer share a repository. Preserve explicit caller overrides and avoid applying model commits to separate tokenizer repositories. Co-authored-by: Hermes Agent Signed-off-by: Yuan Li <dskwelmcy@163.com>
|
@claude review |
Drop HF-only code_revision after Mistral auto detection. Avoid injecting unset revisions into custom tokenizers, and tolerate minimal configs without code_revision in both helpers. Add real Mistral loading and dispatch regressions. Co-authored-by: Hermes Agent Signed-off-by: Yuan Li <dskwelmcy@163.com>
|
@claude review |
There was a problem hiding this comment.
Nothing blocking. The comments below are optional suggestions. There is no need to push a fix for them before merging.
Additional findings (outside the current diff — GitHub can't attach inline comments there):
-
🟣
vllm/tokenizers/registry.py— Pre-existing, security-relevant: operators pinning--code-revisionstill run the tokenizer config's remote code from--revision, even though this PR now forwardscode_revision.get_tokenizercallsget_configwithtrust_remote_codeandrevisionbut nocode_revision(registry.py:247-253). It then reuses that config throughkwargs.setdefault("config", config). Fix: passcode_revision=kwargs.get("code_revision")to thisget_configcall, so config code and tokenizer code come from the same pinned commit. The Mistral path already pops the key before this point.Why this was flagged
Trigger: a trust_remote_code model whose config.json has an
auto_mapAutoConfig entry, served with--revision R --code-revision Cwhere R != C. Model and tokenizer share a repo.cached_tokenizer_from_confignow addscode_revision=Cto kwargs (registry.py:298-301). Because C differs from R,resolve_tokenizer_argsskips the snapshot path (registry.py:168) and keeps the repo id.get_tokenizerthen callsget_configat registry.py:247-253 withoutcode_revision.get_config(transformers_utils/config.py:851-855) acceptscode_revision, but here it defaults to None, so Transformers imports and runs configuration_*.py from R. That config object is handed to AutoTokenizer at registry.py:271. The base branch also ran R's config code here, so this is pre-existing. The PR's stated goal of honoringcode_revisionin tokenizer loading is therefore only partly met.Verification: Pre-existing.
get_tokenizercallsget_configwithoutcode_revision(registry.py:247-253), thoughget_configaccepts it (transformers_utils/config.py:855). When C != R, Transformers falls back torevision, so the remote configuration module runs from R and is reused viakwargs.setdefault("config", config)(registry.py:271).
Forward the resolved code_revision to get_config so remote AutoConfig code uses the same pin as tokenizer code. Cover default, matching and distinct revisions with actual HF dynamic configuration loading. Co-authored-by: Hermes Agent Signed-off-by: Yuan Li <dskwelmcy@163.com>
|
/ci run |
|
✅ Triggered Buildkite CI #95320 for commit |
…project#61611) Signed-off-by: Yuan Li <dskwelmcy@163.com>
Overview
Honor
ModelConfig.code_revisionin both config-based tokenizer helpers when the model and tokenizer share a repository. Complements #61272, which fixed snapshot resolution for direct tokenizer loading.Claims
code_revision=None, remain authoritative.Validation
Config-code pin follow-up (
4a34bfa32d)get_tokenizernow forwards the resolvedcode_revisiontoget_config, so the remote AutoConfig module follows the same pin as tokenizer code. This is an appended commit; earlier reviewed commits are unchanged.hf-internal-testing/test_dynamic_model, with only tokenizer instantiation replaced. Default and matching revisions pass before the fix; a distinct pin fails because the loaded config class comes from the data revision. Negative control: 1 failed, 2 passed. With the fix, the full code-revision selection is 26 passed, 5 deselected.c8ae8a2e5845fc3ad4e5237e4346e5a9ba1e87d2; the fix loaded the requested2efddce40dddaccd37bae208c3c7ca66dbedf68a. Both encodedhello worldto[1, 2]. This is tokenizer validation, not model inference or a new HTTP serving test.Review follow-up (
554c1c1842)Both review findings are addressed: resolved Mistral mode drops the HF-only
code_revisionkey; both config helpers tolerate a missing attribute and no longer inject unset revisions. HF-backed aliases and explicit caller overrides remain supported.Native pytest was rerun in the existing Linux CPU container (Python 3.12.13, Torch 2.13.0+cpu, Transformers 5.19.0, pytest 9.1.1), with this worktree's Python package and tests copied into an isolated
/validation/pr61611directory. No AST extraction was used for these results.python -m pytest tests/tokenizers_/test_registry.py -q --tb=shortpython -m pytest tests/tokenizers_/test_basic.py -k code_revision -q --tb=shortpython -m pytest tests/tokenizers_/test_basic.py -k config_mistral_code_revision -q --tb=noagainst the original PR registrycode_revisionpython -m pytest tests/entrypoints/scale_out/token_in_token_out/test_generate_stream.py tests/entrypoints/openai/completion/test_lora_resolvers.py -q --tb=shortpre-commit run --files vllm/tokenizers/registry.py tests/tokenizers_/test_registry.py tests/tokenizers_/test_basic.pyThe Mistral tests load
mistralai/Mistral-7B-Instruct-v0.3through both helpers in explicit and auto modes, withcode_revision=Noneand"never". Original registry behavior was tested in the same environment with automatic restoration of the fixed file. Warnings were Torch JIT deprecations and the existingTestTokenizercollection warning. These are real tokenizer/backend tests, not model-weight loading, inference/GPU evaluation, or a new HTTP serving test.Initial validation
Linux x86_64, Python 3.12.13, Torch 2.13.0+cpu, Transformers 5.19.0. Validation reused a native environment based on e0317b2 with the relevant current modules and tests copied in; it was not a complete native rebuild of b9b401b.
tests/tokenizers_/test_registry.pygit diff --checkA real
ModelConfig→VllmConfig→ renderer integration also verified separate-repository loading and explicitNoneoverrides.A real no-weights
vllm launch renderHTTP reproducer confirmed:/tokenize.OSError.[1, 0, 0, 2].How to Test
For manual reproduction, use
hf-internal-testing/test_dynamic_tokenizeras both model and tokenizer, pinned to08a35780b7a78794436d8654aafa26ee0165c149, with--tokenizer-mode hf --trust-remote-code --max-model-len 32. The tokenizer-only fixture needs--hf-config-pathpointing to a real cached OPT config (tested withfacebook/opt-125mat27dcfa74d334bc871f3234de431e71c6eeba5dd6). Keep Hub access enabled. Set--code-revision neveror73293ac504e43f4a895688b57c65cebe24f01a38: baseline starts, but this fix rejects the requested code. A matching code revision starts successfully and/tokenizereturns the control token IDs above.Details
Both
tokenizer_args_from_configandcached_tokenizer_from_configomitted the model's code revision. They now usesetdefaultand the same model/tokenizer repository equality guard already used by the processor chat-template path. This avoids applying a model commit to an independently configured tokenizer repository. The skip-tokenizer-init early return is unchanged.vllm/tokenizers/registry.py: guarded code-revision propagation in both wrappers.tests/tokenizers_/test_basic.py: real remote-code loading regressions for both helpers, separate repositories and explicit overrides; real Mistral config-loading regressions; accept equivalent offline errors in the existing negative test.tests/tokenizers_/test_registry.py: complete the existing config fixture and regress Mistral argument filtering, minimal configs, HF-backed aliases and explicit overrides.Duplicate searches were refreshed October 14, 2026 for
code_revision tokenizer, both helper names, and #61272 references. No matching open wrapper fix was found. #61210 concerns speculative MTP config; #51446 and #42616 fix other artifact boundaries; #61272 fixes the downstream resolver, not these wrappers.AI assistance was used for investigation, implementation, validation and review. No inference accuracy or performance improvement is claimed. Full tokenizer-suite, model-inference/GPU evaluations, repository-wide lint and a full current-HEAD native rebuild were not run; the HTTP evidence is render-only tokenization, not generation.
Pull Request Checklist
I used vLLM's
/pr-checklistskill. (Mandatory for agents, optional for humans).AI assistance was used during the creation of this PR.
Design Fit: Minimizes impact on core components, reuses existing functionality, and justifies added complexity.
Testing and Validation: Validates the change and ensures any added tests are meaningful and reliable, with CI coverage or documented CI resource constraints and validation performed outside CI.
Code Quality and Style: Keeps code and comments clear and concise, and updates relevant documentation and examples.
Pull Request Contents: Includes a brief summary and relevant links, supports claims with evidence, explains root causes and implementation trade-offs, and follows the contributing guide.