fix(tokenizer): register deepseek_v32 config alias for DeepSeek-V3.2-Exp - #1069
Conversation
Try out this PRQuick install: pip install --upgrade --force-reinstall git+https://github.com/ai-dynamo/aiperf.git@725ed3c3b2e721e362edc6e62e69ed364a3150f1Recommended with virtual environment (using uv): uv venv --python 3.12 && source .venv/bin/activate
uv pip install --upgrade --force-reinstall git+https://github.com/ai-dynamo/aiperf.git@725ed3c3b2e721e362edc6e62e69ed364a3150f1Last updated for commit: |
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Enterprise Run ID: 📒 Files selected for processing (3)
✅ Files skipped from review due to trivial changes (1)
🚧 Files skipped from review as they are similar to previous changes (2)
WalkthroughAdds a best-effort ChangesDeepSeek V3.2 tokenizer shim
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
🧹 Nitpick comments (1)
src/aiperf/common/tokenizer.py (1)
415-416: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueBroad
except Exceptionis appropriate here.The blanket catch (Ruff BLE001) is justified for this best-effort shim: registration must never block tokenizer loading, and the docstring documents the silent-fallthrough contract.
KeyboardInterrupt/SystemExitstill propagate, and this is synchronous code, so cancellation semantics don't apply. Consider a# noqa: BLE001with a short rationale to keep the linter quiet.🤖 Prompt for 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. In `@src/aiperf/common/tokenizer.py` around lines 415 - 416, Keep the broad exception handling in the tokenizer alias registration shim, but explicitly mark the `except Exception as e` in `tokenizer.py` with a `# noqa: BLE001` and a short rationale so Ruff does not flag the intentional best-effort fallback. Refer to the alias registration block around the `deepseek_v32` shim and preserve the existing debug logging and silent-fallthrough behavior.Source: Linters/SAST tools
🤖 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.
Nitpick comments:
In `@src/aiperf/common/tokenizer.py`:
- Around line 415-416: Keep the broad exception handling in the tokenizer alias
registration shim, but explicitly mark the `except Exception as e` in
`tokenizer.py` with a `# noqa: BLE001` and a short rationale so Ruff does not
flag the intentional best-effort fallback. Refer to the alias registration block
around the `deepseek_v32` shim and preserve the existing debug logging and
silent-fallthrough behavior.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Enterprise
Run ID: c1924124-019a-4ab0-a994-ccf3e9ecec8c
📒 Files selected for processing (3)
docs/reference/tokenizer-auto-detection.mdsrc/aiperf/common/tokenizer.pytests/unit/common/test_tokenizer_deepseek_v32.py
Codecov Report✅ All modified and coverable lines are covered by tests. 📢 Thoughts on this report? Let us know! |
DeepSeek-V3.2-Exp ships config.json with model_type "deepseek_v32" and no auto_map. On transformers releases without native deepseek_v32 support (our >=4.56 floor still admits these), the AutoConfig lookup that AutoTokenizer performs internally fails and tokenizer loading aborts before any benchmark traffic. --tokenizer-trust-remote-code cannot help because, with no auto_map, there is no remote config class to import. Register a narrow DeepseekV3Config alias under model_type "deepseek_v32" before loading the tokenizer (V3.2 reuses the V3 config schema; vLLM and SGLang do the same). The shim is idempotent and best-effort: a no-op when transformers already knows the model type natively, and silent when the base config class is unavailable so loading reaches its normal error path. Native support landed upstream in huggingface/transformers#41251; this covers the older releases in our supported range that predate it. Fixes ai-dynamo#1047 Signed-off-by: Siwei Li <siweili@amazon.com>
0424f8d to
06e415e
Compare
![dynamo-review-agent[bot]](https://avatars.githubusercontent.com/u/201626793?s=60&v=4){kind=small}
ajcasagrande
left a comment
ajcasagrande
left a comment
There was a problem hiding this comment.
I reviewed this against origin/main and did not find any blocking correctness issues.
What I validated:
- The shim registers only
deepseek_v32, returns without overriding native transformer support, and runs before allAutoTokenizer.from_pretrained(...)load branches. - The new unit tests pass (
tests/unit/common/test_tokenizer_deepseek_v32.py: 5 passed). - A runtime probe against the real
deepseek-ai/DeepSeek-V3.2-Exptokenizer shows the pre-shimAutoConfig.for_model("deepseek_v32")failure, then successful alias registration andLlamaTokenizerload through AIPerf'sTokenizer.from_pretrained. - A real
aiperf profileinvocation against the in-repo mock server with--tokenizer deepseek-ai/DeepSeek-V3.2-Exp --tokenizer-trust-remote-codecompleted successfully.
Overall assessment: the fix is narrow, well-covered, and matches the stated compatibility goal. Nice job keeping it best-effort and self-disabling for future native support.
|
Thanks for the review and approval, @ajcasagrande! Flagging the 4 red Windows checks — they look unrelated to this change:
Could you re-run the Windows jobs when you get a chance? I don't have permission to trigger it from my fork. Happy to rebase on |
…Exp (#1069) Signed-off-by: Siwei Li <siweili@amazon.com> Co-authored-by: Siwei Li <siweili@amazon.com>
4 participants
What
DeepSeek-V3.2-Exp ships
config.jsonwithmodel_type: "deepseek_v32"andno
auto_map. Ontransformersreleases without nativedeepseek_v32support (which our
>=4.56floor still admits), theAutoConfiglookup thatAutoTokenizerperforms internally fails, and tokenizer loading aborts at"Configure Profiling" before any benchmark traffic.
--tokenizer-trust-remote-codecannot help: with no
auto_map, there is no remote config class to import.The serving engine (vLLM/SGLang) loads the model fine — only the AIPerf client crashes.
Fixes #1047
How
Register a narrow
DeepseekV3Configalias undermodel_type "deepseek_v32"before loading the tokenizer (V3.2 reuses the V3 config schema; vLLM and SGLang
do the same via
AutoConfig.register). The shim is:support on newer
transformers, or a prior call), and silent if the baseconfig class is unavailable so loading reaches its normal error path.
deepseek_v32only (kimi_k2ships anauto_map;deepseek_v3/deepseek_v2are already intransformers).so this covers only the older releases in our supported range that predate it.
Testing
New unit tests in
tests/unit/common/test_tokenizer_deepseek_v32.py(5 tests):registration / no-op / error-swallow cases, a real-
transformersround-trip,and a
_load_from_hubhook check.Verified end-to-end against the real
deepseek-ai/DeepSeek-V3.2-Exptokenizer files on real
transformers==5.5.4, driving AIPerf's ownTokenizer.from_pretrained:TokenizerError: ... AttributeError: 'PreTrainedConfig' object has no attribute 'max_position_embeddings'— reproduces #1047LlamaTokenizer; encodes/decodes correctlyNo regressions: 250 tokenizer + 2048
tests/unit/common+ 59 property tests pass.Doc note added to
docs/reference/tokenizer-auto-detection.md.Summary by CodeRabbit