Skip to content

[None][fix] DeepSeek renderers: tools join the leading system message; V4.1 renders developer as system - #21051

Merged
JunyiXu-nv merged 1 commit into
NVIDIA:mainfrom
JunyiXu-nv:dev-junyix-fix-v41-tools-on-system
Oct 27, 2026
Merged

JunyiXu-nv merged 1 commit into
NVIDIA:mainfrom
JunyiXu-nv:dev-junyix-fix-v41-tools-on-system

Conversation

@JunyiXu-nv

@JunyiXu-nv JunyiXu-nv commented Oct 27, 2026 •

Copy link
Copy Markdown
Collaborator

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:

  • V3.2 and V4: a system or developer message;
  • V4.1: a system message. Its README says "Tools are defined on the system message via the tools field", and its test harness attaches a case's tools to messages[0].
<|System|>{system prompt}\n\n## Tools ...          (V4.1)

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. V4.1 also rendered a second <|System|> block:

<|System|>\n\n## Tools ...<|System|>{system prompt}

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 developer role, and its renderer raised NotImplementedError on one. So a client that sends developer messages got HTTP 400 on every request. V4.1's apply_chat_template now renders a developer message as a system message. encode_messages stays strict, like the checkpoint's encoder.

Tests in CI. tests/unittest/llmapi/test_deepseek_v41_tokenizer.py was 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.py gains:
    • 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 when LLM_MODELS_ROOT holds the checkpoint's encoding/, like the file's existing checkpoint differential.
  • test_deepseek_v32_tokenizer.py (new, CPU) covers:
    • tools joining a leading system or developer message, against the module's own encode_messages;
    • a conversation without a system message;
    • the system prompt coming before the tools.
  • test_deepseek_v4_tokenizer.py gains test_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:

  • Suites. The DeepSeek-V3.2, V4 and V4.1 tokenizer suites pass, with the V4.1 checkpoint differential on, and so do the tool-parser and Anthropic suites.
  • Base comparison. Without the change, the new tests that cover the layout and the developer role fail. That is 5 for V4.1 and 4 for V3.2 and V4; the fifth V3.2 test covers behavior that main already had.
  • Planted bugs. Each of 10 planted bugs makes at least one of the new tests fail:
    • V4.1: no developer mapping; always inserting a tools-only message; attaching the tools to a first message that is not a system message; mutating the caller's message; tools ahead of the content.
    • V3.2: always inserting; attaching to any first message; mutating the caller's message.
    • V4: always inserting; mutating the caller's message.
  • Against the checkpoints' encoders. Checked outside the test suite, with the encoders in each checkpoint's encoding/ directory:
    • V4.1 and V4 renders now equal their encoders byte for byte, for a leading system message, a leading developer message (V4) and no system message, in chat and thinking mode.
    • The V4 checkpoint's own golden case with tools is reproduced.
    • For V3.2, the layout now matches the encoder's. The checkpoint at hand (DeepSeek-V3.2) renders a few more newlines in its tools preamble than the encoder vendored in 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.

  • Each request's served render equals the checkpoint encoder's rendering of the same conversation.
  • Each billed prompt token count matches that render.
  • Before the change, the served layout differed for every such request.

PR Checklist

  • PR description clearly explains what and why.

GitHub Bot Help

To see a list of available CI bot commands, please comment /bot help.

🤖 Generated with Claude Code

Dev Engineer Review

  • DeepSeek-V3.2 and V4 now attach nonempty tools to a leading system or developer message when it has no tools field. Otherwise, they prepend a tools-only system message. This keeps tools after the leading message content and avoids changing the input message.
  • DeepSeek-V4.1 maps developer messages to system messages in apply_chat_template and applies the same tools-placement behavior. Its lower-level encode_messages remains strict and does not accept developer roles.
  • The CPU test list now includes the V3.2 and V4.1 tokenizer suites. The V4 tokenizer suite is listed in the B200 test list. No manual-QA list entries were found.
  • Reported validation includes the three tokenizer suites, tool-parser and Anthropic suites, plus checkpoint comparisons. These results were not independently run here. The reported V3.2 checkpoint comparison retains a tools-preamble newline difference that this change does not address.

QA Engineer Review

  • Tests and CI lists: The change adds or updates tokenizer tests for V3.2, V4, and V4.1. l0_cpu.yml lists V3.2 and V4.1; l0_b200.yml lists V4. No manual-QA entries were found.
  • Coverage: Tests cover tools placement, fallback system-message insertion, input immutability, developer-message handling, and V4.1 checkpoint comparisons. Coverage verdict: sufficient for the changed behavior. Test execution results are not independently verified here.

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 in encode_messages; also verify tools placement.
  • tests/unittest/llmapi/test_deepseek_v32_tokenizer.py — Covers tools placement, fallback insertion, render order, and input immutability. Listed in test-db/l0_cpu.yml.
  • tests/unittest/llmapi/test_deepseek_v4_tokenizer.py — Covers tools placement after a leading system or developer message. Listed in test-db/l0_b200.yml.
  • tests/unittest/llmapi/test_deepseek_v41_tokenizer.py — Covers developer conversion, tools placement, and checkpoint comparisons. Listed in test-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.

…; 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>
@coderabbitai

coderabbitai Bot commented Oct 27, 2026

Copy link
Copy Markdown
Contributor

Review in Change Stack →

No actionable comments were generated in the recent review.

Recent review info
Run configuration
Commits

Reviewing files that changed from the base of the PR and between cfae451 and b2b56d4.

Files selected for processing: 7
  • tensorrt_llm/tokenizer/deepseek_v32/tokenizer.py
  • tensorrt_llm/tokenizer/deepseek_v4/tokenizer.py
  • tensorrt_llm/tokenizer/deepseek_v41/tokenizer.py
  • tests/integration/test_lists/test-db/l0_cpu.yml
  • tests/unittest/llmapi/test_deepseek_v32_tokenizer.py
  • tests/unittest/llmapi/test_deepseek_v41_tokenizer.py
  • tests/unittest/llmapi/test_deepseek_v4_tokenizer.py

Included review availability: This review used your included allowance. Your plan provides up to 12 included reviews per hour; 10 remain after this review.

Walkthrough

DeepSeek v3.2, v4, and v4.1 chat templates now place supplied tools in an existing leading system or developer message when possible. The v4.1 tokenizer also converts developer messages to system messages. New tests cover tool rendering, input immutability, and encoder output.

Changes

DeepSeek tool rendering

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: Normal

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: Passed 4 · Failed 1

Failed Failed checks: 1 warning

Check name Status Explanation Resolution
Docstring Coverage Warning 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: … Write docstrings for the functions missing them to satisfy the coverage threshold.
Passed Passed checks: 4 passed
Check name Status Explanation
Title check Passed The title clearly summarizes the primary renderer changes, including tool placement and V4.1 developer-message handling. It uses the required ticket and type format.
Description check Passed 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 i…
Linked Issues check Passed Check skipped because no linked issues were found for this pull request.
Out of Scope Changes check Passed Check skipped because no linked issues were found for this pull request.
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.

@JunyiXu-nv

Copy link
Copy Markdown
Collaborator Author

/bot run --disable-fail-fast

@tensorrt-cicd

Copy link
Copy Markdown
Collaborator

PR_Github #80736 [ run ] triggered by Bot. Commit: b2b56d4 Link to invocation

@github-actions

Copy link
Copy Markdown

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.

@tensorrt-cicd

Copy link
Copy Markdown
Collaborator

PR_Github #80736 [ run ] completed with state SUCCESS. Commit: b2b56d4
/LLM/main/L0_MergeRequest_PR pipeline #66479 completed with status: 'SUCCESS'
Pipeline passed with automatic retried tests. Check the rerun report for details.

CI Report

Link to invocation

@JunyiXu-nv
JunyiXu-nv merged commit 32d8ada into NVIDIA:main Oct 27, 2026
18 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants