Repository navigation
[None][fix] DeepSeek renderers: tools join the leading system message; V4.1 renders developer as system - #21051
Conversation
…; V4.1 renders developer as system The checkpoints' encoders for DeepSeek-V3.2, V4 and V4.1 render tools from the system (or, V3.2 and V4, developer) message that leads the conversation, after its content: one system block. Each tokenizer's apply_chat_template instead inserted a tools-only system message ahead of the caller's own, so every request with a system prompt and tools rendered the tools before the system prompt, and V4.1 rendered a second <|System|> block. V4.1 has no developer role, and its renderer raised on one, so every request from a client that sends developer messages failed with HTTP 400. V4.1's apply_chat_template now renders a developer message as a system message. The V4.1 tokenizer tests were in no CI list; they and the new V3.2 tests run in the CPU stage. Signed-off-by: JunyiXu-nv <219237550+JunyiXu-nv@users.noreply.github.com>
|
No actionable comments were generated in the recent review.
|
| Layer / File(s) | Summary |
|---|---|
Tokenizer tool placement tensorrt_llm/tokenizer/deepseek_v32/tokenizer.py, tensorrt_llm/tokenizer/deepseek_v4/tokenizer.py, tensorrt_llm/tokenizer/deepseek_v41/tokenizer.py |
The tokenizers attach supplied tools to an eligible leading system or developer message. Otherwise, they prepend a system message containing the tools. The v4.1 tokenizer also maps developer messages to system messages. |
Rendering tests and CPU test registration tests/unittest/llmapi/test_deepseek_v32_tokenizer.py, tests/unittest/llmapi/test_deepseek_v4_tokenizer.py, tests/unittest/llmapi/test_deepseek_v41_tokenizer.py, tests/integration/test_lists/test-db/l0_cpu.yml |
Tests cover tool placement, rendering, input immutability, and v4.1 output against the vendor encoder. The CPU integration test list adds the v3.2 and v4.1 tokenizer tests. |
Priority:
Estimated code review effort: 2 (Simple) | ~12 minutes
Change: Bug fix
Possibly related PRs
- NVIDIA/TensorRT-LLM#21049: Adds serving paths for native tokenizer renderers, flattened tools, and developer-role rendering that use related DeepSeek renderer behavior.
- NVIDIA/TensorRT-LLM#19883: Changes Responses API preprocessing for developer messages and tool-bearing chat-template input, overlapping with the v4.1 developer-message mapping.
Suggested reviewers: schetlur-nv, brnguyen2
Merge Risk: 
Minimal · up to b2b56
-
The reviewed changes have no identified issue requiring correction before merge.
Pre-merge checks: 
4 · 
1
-
Failed checks: 1 warning Passed checks: 4 passed-
Full details: Description check
-
Explanation
The description includes the required Description, Test Coverage, and PR Checklist sections. It explains the problem, solution, affected tokenizers, CI changes, and validation results. The checklist is only partially checked, but the required technical information is present.
-
Full details: Docstring Coverage
-
Explanation
Docstring coverage is 35.71% which is insufficient. The required threshold is 80.00%. Docstring coverage is scoped to functions touched by this diff. Analyzed 14 functions across 6 files. (1 skipped: 1 unsupported.)
Resolution
Write docstrings for the functions missing them to satisfy the coverage threshold.
-
Finishing Touches
-
🧪 Generate unit tests (beta)
-
- Create a new PR
-
- Autofix · Keep fixing CodeRabbit findings and required CI, and resolving merge conflicts
Comment @coderabbitai help to get the list of available commands.
|
/bot run --disable-fail-fast |
|
PR_Github #80736 [ run ] triggered by Bot. Commit: |
|
Automatically added "ci: full pre-merge approved" because this PR has satisfied the required GitHub review approvals. Unresolved review conversations and other required checks remain independent merge requirements. |
|
PR_Github #80736 [ run ] completed with state |
Description
The checkpoints' own encoders for DeepSeek-V3.2, V4 and V4.1 render tools from the message that leads the conversation, after its content:
systemmessage via thetoolsfield", and its test harness attaches a case's tools tomessages[0].Each tokenizer's
apply_chat_templateinstead inserted a tools-only system message ahead of the caller's own. So every request with a system prompt and tools rendered the tools before the system prompt. V4.1 also rendered a second<|System|>block:This is not the layout the checkpoints were trained on, and agent clients always send both a system prompt and tools.
The tools now join the leading system message, and for V3.2 and V4 a leading developer message too. A conversation without one still gets a tools-only system message, as before.
V4.1 developer role. V4.1 has no
developerrole, and its renderer raisedNotImplementedErroron one. So a client that sends developer messages got HTTP 400 on every request. V4.1'sapply_chat_templatenow renders a developer message as a system message.encode_messagesstays strict, like the checkpoint's encoder.Tests in CI.
tests/unittest/llmapi/test_deepseek_v41_tokenizer.pywas in no CI list, so its tests never ran in CI. It now runs in the CPU stage (l0_cpu.yml), together with the new V3.2 tests.Test Coverage
test_deepseek_v41_tokenizer.pygains:test_tools_argument_joins_the_leading_system_message: one system block, the content before the tools, and the caller's message left unchanged;test_developer_message_renders_as_a_system_message, with and without tools;test_tools_argument_matches_the_checkpoint_encoder:apply_chat_template(..., tools=...)against the checkpoint's encoder, for a conversation with a system prompt and one without, in chat and thinking mode. It runs whenLLM_MODELS_ROOTholds the checkpoint'sencoding/, like the file's existing checkpoint differential.test_deepseek_v32_tokenizer.py(new, CPU) covers:encode_messages;test_deepseek_v4_tokenizer.pygainstest_deepseek_v4_tools_argument_joins_the_leading_system_message, for system and developer.Results, on a build of main 9bb7931 with this change and the other open serving fixes applied:
encoding/directory:tensorrt_llm/tokenizer/deepseek_v32/encoding.py. That is a separate difference this change does not touch.End to end, Claude Code sessions were served by DeepSeek-V4.1-Flash with this change: 631 Messages requests, each with a system prompt and tools.
PR Checklist
GitHub Bot Help
To see a list of available CI bot commands, please comment
/bot help.🤖 Generated with Claude Code
Dev Engineer Review
toolsto a leading system or developer message when it has notoolsfield. Otherwise, they prepend a tools-only system message. This keeps tools after the leading message content and avoids changing the input message.apply_chat_templateand applies the same tools-placement behavior. Its lower-levelencode_messagesremains strict and does not accept developer roles.QA Engineer Review
l0_cpu.ymllists V3.2 and V4.1;l0_b200.ymllists V4. No manual-QA entries were found.Per-File QA Perspective
tensorrt_llm/tokenizer/deepseek_v32/tokenizer.py— Verify that tools join a leading system or developer message only when it has no tools, and otherwise use a tools-only system message.tensorrt_llm/tokenizer/deepseek_v4/tokenizer.py— Verify tools render after leading system or developer content, and fallback insertion works when no eligible leading message exists.tensorrt_llm/tokenizer/deepseek_v41/tokenizer.py— Verify the wrapper maps developer to system and preserves strict developer-role rejection inencode_messages; also verify tools placement.tests/unittest/llmapi/test_deepseek_v32_tokenizer.py— Covers tools placement, fallback insertion, render order, and input immutability. Listed intest-db/l0_cpu.yml.tests/unittest/llmapi/test_deepseek_v4_tokenizer.py— Covers tools placement after a leading system or developer message. Listed intest-db/l0_b200.yml.tests/unittest/llmapi/test_deepseek_v41_tokenizer.py— Covers developer conversion, tools placement, and checkpoint comparisons. Listed intest-db/l0_cpu.yml.tests/integration/test_lists/test-db/l0_cpu.yml— Adds the V3.2 and V4.1 tokenizer test files to the CPU CI stage.