[https://nvbugs/6160085][fix] At tensorrt_llm/tokenizer/tokenizer.py import time, re-export `bytes_to_unicod - #14116
Conversation
📝 WalkthroughWalkthroughThis PR adds a compatibility shim to handle Transformers 5.x moving the ChangesTransformers 5.x Compatibility and Test Cleanup
Estimated code review effort🎯 1 (Trivial) | ⏱️ ~3 minutes Suggested reviewers
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@tensorrt_llm/tokenizer/tokenizer.py`:
- Around line 18-24: The compatibility shim is importing bytes_to_unicode from
the wrong module; update the fallback import so we pull bytes_to_unicode from a
model-specific tokenizer module (e.g., from
transformers.models.clvp.tokenization_clvp or
transformers.models.gpt2.tokenization_gpt2) instead of
transformers.convert_slow_tokenizer — i.e., in tokenizer.py, inside the try
block where _gpt2_mod is loaded and you check hasattr(_gpt2_mod,
"bytes_to_unicode"), attempt to import bytes_to_unicode from known
model-specific modules (try clvp.tokenization_clvp then gpt2.tokenization_gpt2)
and assign it to _gpt2_mod.bytes_to_unicode so the monkey‑patch succeeds.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Enterprise
Run ID: 4dd320ce-dd84-4ab0-aff3-90c60e840b0b
📒 Files selected for processing (2)
tensorrt_llm/tokenizer/tokenizer.pytests/integration/test_lists/waives.txt
💤 Files with no reviewable changes (1)
- tests/integration/test_lists/waives.txt
96ec4d6 to
de4652d
Compare
90d201d to
132447e
Compare
…e tokenizers Transformers 5.x removed bytes_to_unicode from transformers.models.gpt2.tokenization_gpt2 (it now lives in transformers.convert_slow_tokenizer). Some checkpoints loaded via trust_remote_code=True (e.g. Kimi-K2's tokenization_kimi.py) still import the helper from the historical location, which made AutoTokenizer.from_pretrained fail with "cannot import name 'bytes_to_unicode'", leaving llm.tokenizer = None and breaking TestKimiK2::test_nvfp4_longseq_trtllm_moe_stress at tokenizer.encode(...). Inject the symbol back onto the legacy module at tokenizer-module load time, mirroring the existing maybe_fix_byte_level_tokenizer compat pattern. Also remove the matching nvbugs/6160085 waivers for both test_nvfp4_longseq_trtllm_moe_stress and test_nvfp4_longseq_trtllm_moe_async_cancel. Signed-off-by: tensorrt-cicd <90828364+tensorrt-cicd@users.noreply.github.com>
132447e to
a2bc16f
Compare
|
/bot run |
|
PR_Github #50482 [ run ] triggered by Bot. Commit: |
|
PR_Github #50482 [ run ] completed with state
|
|
/bot run |
|
PR_Github #50683 [ run ] triggered by Bot. Commit: |
|
PR_Github #50683 [ run ] completed with state |
Summary
bytes_to_unicodefromtransformers.models.gpt2.tokenization_gpt2, but transformers 5.x moved that symbol totransformers.convert_slow_tokenizer, so the import fails and TRT-LLM falls back totokenizer=None.tensorrt_llm/tokenizer/tokenizer.pyimport time, re-exportbytes_to_unicodeontotransformers.models.gpt2.tokenization_gpt2fromtransformers.convert_slow_tokenizerif missing; remove the two matching nvbugs/6160085 waivers. Verified with the full test (passed in 1421s).Test plan
Links
Summary by CodeRabbit
Chores
Tests