[#17572][fix] Emit the withheld buffer in DeepSeek streaming tool parsers - #17573
[#17572][fix] Emit the withheld buffer in DeepSeek streaming tool parsers#17573Yigtwxx wants to merge 4 commits into
Conversation
|
@tongyuantongyu @asfiyab-nvidia when you get a chance, could one of you kick off a pipeline run? The change is confined to the four DeepSeek tool parsers and the new cases are |
|
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 as they are similar to previous changes (3)
WalkthroughDeepSeek V3, V3.1, and V3.2 streaming parsers now preserve accumulated text while checking partial tool-call markers. End markers are removed from emitted text. Parameterized tests cover all four DeepSeek parser variants. ChangesDeepSeek streaming parsing
Estimated code review effort: 2 (Simple) | ~10 minutes Mergeability Score: ⚪ Minimal · up to This change preserves ordinary assistant text that was previously lost during DeepSeek streaming while retaining tool-call parsing behavior; no actionable merge-blocking risk remains after normal checks and review. Suggested reviewers: 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
tests/unittest/llmapi/apps/test_tool_parsers.py (1)
1798-1799: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winAdd the required function annotations.
Annotate
sample_tools,parser_cls, anddeltaswith precise types. Add-> None. Do not useAny.As per coding guidelines, “Annotate every function” and “avoid unnecessary
Any.”🤖 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 `@tests/unittest/llmapi/apps/test_tool_parsers.py` around lines 1798 - 1799, Update test_deepseek_streaming_preserves_withheld_text by adding precise, non-Any type annotations for sample_tools, parser_cls, and deltas, and annotate its return type as None.Source: Coding guidelines
🤖 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/serve/tool_parser/deepseekv32_parser.py`:
- Around line 185-193: Update the diverged-prefix handling in the DeepSeek V3.2
parser so potentially_dsml remains true only when the buffered text ends with a
partial self.bot_token or <｜DSML｜invoke suffix, not merely when those markers
appear anywhere; preserve ordinary text such as split ["Use <｜DSML｜function",
"ality"] through the normal-text path. In
tests/unittest/llmapi/apps/test_tool_parsers.py lines 1791-1808, add this
split-input case and assert normal-text preservation for every parser class
listed there.
---
Nitpick comments:
In `@tests/unittest/llmapi/apps/test_tool_parsers.py`:
- Around line 1798-1799: Update test_deepseek_streaming_preserves_withheld_text
by adding precise, non-Any type annotations for sample_tools, parser_cls, and
deltas, and annotate its return type as None.
🪄 Autofix
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: 48a01eb1-be42-47bb-a453-4d4287207a85
📒 Files selected for processing (4)
tensorrt_llm/serve/tool_parser/deepseekv31_parser.pytensorrt_llm/serve/tool_parser/deepseekv32_parser.pytensorrt_llm/serve/tool_parser/deepseekv3_parser.pytests/unittest/llmapi/apps/test_tool_parsers.py
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Nitpick comments (1)
tensorrt_llm/serve/tool_parser/deepseekv32_parser.py (1)
178-181: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winHandle the intentional Unicode delimiters in Ruff configuration.
Ruff reports RUF003 for the comment at Line 178 and RUF001 for the string at Line 181. Do not replace
｜with ASCII|; the fullwidth character is required by the DeepSeek DSML token.If these rules are enabled in CI, add a narrow suppression or per-file ignore. Verify the configured Ruff rules before changing the code.
Ruff reported these warnings in the changed lines.
🤖 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 `@tensorrt_llm/serve/tool_parser/deepseekv32_parser.py` around lines 178 - 181, Preserve the intentional fullwidth `｜` delimiters in the comment and `partial_tokens` definition. Inspect the repository’s Ruff configuration and add the narrowest appropriate suppression or per-file ignore for RUF003 and RUF001 affecting `deepseekv32_parser.py`, without replacing the required Unicode token characters.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.
Inline comments:
In `@tensorrt_llm/serve/tool_parser/deepseekv32_parser.py`:
- Around line 190-193: Update the tool-call parsing flow in detect_and_parse()
to emit any ordinary text preceding self.bot_token before consuming the matched
tool segment, preserving that prefix when _buffer is reset after the invoke
match. Ensure the returned streaming normal_text includes the prefix alongside
the parsed tool call, and add a streaming test covering ordinary text and a
complete tool call in the same delta.
- Around line 175-190: Update the tool-call detection in the parser method
containing has_tool_call so it only recognizes a complete invoke header, not any
occurrence of "<｜DSML｜invoke" within current_text. Keep buffering exclusively
when the trailing text is a valid partial prefix of that header, allowing
ordinary text such as the split "invoke" plus "ality" case to be emitted; add
this scenario to the regression tests.
In `@tests/unittest/llmapi/apps/test_tool_parsers.py`:
- Around line 1796-1799: Update the DeepSeek DSML token in the parser test case
to silence RUF001 without changing its value: represent each fullwidth vertical
line as \uFF5C or add a targeted # noqa: RUF001 to the affected line.
---
Nitpick comments:
In `@tensorrt_llm/serve/tool_parser/deepseekv32_parser.py`:
- Around line 178-181: Preserve the intentional fullwidth `｜` delimiters in the
comment and `partial_tokens` definition. Inspect the repository’s Ruff
configuration and add the narrowest appropriate suppression or per-file ignore
for RUF003 and RUF001 affecting `deepseekv32_parser.py`, without replacing the
required Unicode token characters.
🪄 Autofix
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: 937a9700-c351-4d2a-b39a-9b0638f3f7bd
📒 Files selected for processing (2)
tensorrt_llm/serve/tool_parser/deepseekv32_parser.pytests/unittest/llmapi/apps/test_tool_parsers.py
| if not has_tool_call and not ends_with_partial_token: | ||
| # The guard above withholds the whole buffer, so the buffer is what has | ||
| # to be emitted once it clears; returning only the latest delta would | ||
| # drop everything withheld by an earlier increment. |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | 🏗️ Heavy lift
Preserve text before a complete tool call.
When a delta contains ordinary text before self.bot_token, this branch is skipped. The parse path later resets _buffer after the invoke match at Line 278, so the prefix is discarded.
For example, a delta beginning with "Normal text <｜DSML｜function_calls> ..." can return tool calls without returning "Normal text". detect_and_parse() returns that prefix at Lines 127-130.
Emit the pre-tool text before consuming the tool segment. Add a streaming test with ordinary text and a complete tool call in the same delta. Otherwise, concatenated streaming normal_text does not match non-streaming parsing.
🤖 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 `@tensorrt_llm/serve/tool_parser/deepseekv32_parser.py` around lines 190 - 193,
Update the tool-call parsing flow in detect_and_parse() to emit any ordinary
text preceding self.bot_token before consuming the matched tool segment,
preserving that prefix when _buffer is reset after the invoke match. Ensure the
returned streaming normal_text includes the prefix alongside the parsed tool
call, and add a streaming test covering ordinary text and a complete tool call
in the same delta.
There was a problem hiding this comment.
This one is real, and I reproduced it, but I would rather not fix it here.
delta: 'Normal text <｜DSML｜function_calls> ... </｜DSML｜function_calls>'
streaming -> normal_text='', calls=2
non-streaming -> normal_text='Normal text', calls=1
It is not specific to V3.2 either. DeepSeekV31Parser drops the prefix the same way on 'Normal text <｜tool▁calls▁begin｜>...', and the call-count difference above suggests the streaming emission path has its own problem separate from the dropped prefix.
That puts it in the tool-parsing branch rather than the withhold guard this PR is about, it touches all four parsers, and it needs its own tests for the emission shape. Fixing it here would make the change span two unrelated code paths. I will open a separate issue with the reproduction so it does not get lost, and link it from this thread.
There was a problem hiding this comment.
@Yigtwxx, agreed. The reproduction confirms the finding.
This pull request changes the partial-token buffer guard. The dropped prefix and the streaming call-emission shape are in a separate tool-parsing path. A fix needs cross-parser coverage and dedicated streaming tests.
Deferring this work to a follow-up issue is appropriate. Please link the issue here when available.
✏️ Learnings added
Learnt from: Yigtwxx
Repo: NVIDIA/TensorRT-LLM PR: 17573
File: tensorrt_llm/serve/tool_parser/deepseekv32_parser.py:195-198
Timestamp: 2026-08-12T19:38:23.109Z
Learning: In NVIDIA/TensorRT-LLM DeepSeek streaming tool parsers, ordinary text before a complete tool-call segment can be dropped by the tool-parsing path: `DeepSeekV32Parser` drops text before `<｜DSML｜function_calls>`, and `DeepSeekV31Parser` has the equivalent issue before `<｜tool▁calls▁begin｜>`. This is separate from suffix-based partial-token buffering and requires a cross-parser follow-up with tests for normal-text preservation and streaming tool-call emission shape.
Note: Learnings are effective only in the context of similar code segments. To apply general coding guidelines, please configure review instructions. You can manage existing learnings in the app.
🧠 Learnings used
Learnt from: Yigtwxx
Repo: NVIDIA/TensorRT-LLM PR: 17573
File: tensorrt_llm/serve/tool_parser/deepseekv32_parser.py:0-0
Timestamp: 2026-08-12T19:26:02.216Z
Learning: In `tensorrt_llm/serve/tool_parser/deepseekv32_parser.py`, `DeepSeekV32Parser.parse_streaming_increment()` must use suffix-based partial-token detection on the accumulated buffer for `bot_token`, the invoke start token, `eot_token`, `invoke_end_token`, and `_eos_token`. This prevents loss of ordinary text after a diverged DSML prefix and prevents split closing delimiters from leaking into `normal_text`. `DeepSeekV4Parser` inherits this behavior.
You are interacting with an AI system.
There was a problem hiding this comment.
Filed as #17580 with the reproduction and the table for all four parsers.
There was a problem hiding this comment.
@Yigtwxx, thank you for filing #17580. The issue captures the deferred cross-parser tool-parsing work and its reproduction coverage.
🧠 Learnings used
Learnt from: Yigtwxx
Repo: NVIDIA/TensorRT-LLM PR: 17573
File: tensorrt_llm/serve/tool_parser/deepseekv32_parser.py:0-0
Timestamp: 2026-08-12T19:26:02.216Z
Learning: In `tensorrt_llm/serve/tool_parser/deepseekv32_parser.py`, `DeepSeekV32Parser.parse_streaming_increment()` must use suffix-based partial-token detection on the accumulated buffer for `bot_token`, the invoke start token, `eot_token`, `invoke_end_token`, and `_eos_token`. This prevents loss of ordinary text after a diverged DSML prefix and prevents split closing delimiters from leaking into `normal_text`. `DeepSeekV4Parser` inherits this behavior.
Learnt from: Yigtwxx
Repo: NVIDIA/TensorRT-LLM PR: 17573
File: tensorrt_llm/serve/tool_parser/deepseekv32_parser.py:195-198
Timestamp: 2026-08-12T19:38:23.109Z
Learning: In NVIDIA TensorRT-LLM DeepSeek streaming tool parsers, preserve ordinary text that appears before a complete tool-call marker such as <｜DSML｜function_calls> or <｜tool▁calls▁begin｜>. Do not drop this prefix while handling tool calls; keep this behavior distinct from suffix-based partial-token buffering, and add tests covering normal-text preservation and the emitted streaming tool-call shape.
You are interacting with an AI system.
Would be better if you can attach run time of each case when you are talking about that. They seem lightweight so it's probably fine anyway. |
|
Numbers for the parametrizations, measured locally (Python 3.12, CPU only; best
All twelve together are 0.19 ms of actual work; under pytest the whole I have also pushed the review fixes: the redundant |
|
/bot run |
|
PR_Github #65820 [ run ] triggered by Bot. Commit: |
|
PR_Github #65820 [ run ] completed with state |
|
/bot run |
|
PR_Github #65862 [ run ] triggered by Bot. Commit: |
|
PR_Github #65862 [ run ] completed with state
|
…ol parsers parse_streaming_increment accumulates deltas into self._buffer and withholds the buffer while it could still grow into a tool-call start token. Once the ambiguity cleared, the buffer was cleared but only the current delta was returned, so everything withheld by an earlier increment was dropped from the streamed response. Both start tokens begin with "<", so ordinary assistant text containing "<" lost characters. Emit the buffer instead of the delta. In the V3 and V3.1 parsers also test the buffer with the _ends_with_partial_token helper rather than testing the delta with startswith, which matches BaseToolParser.parse_streaming_increment and the gemma4, minimax_m3, poolside_v1, qwen3 and kimi_k3 parsers. V3.2 already gates on the buffer, so only the emitted text changes there, and DeepSeekV4Parser inherits the fix from DeepSeekV32Parser. Signed-off-by: Yiğit ERDOĞAN <yigiterdogan023@gmail.com>
… DSML tag The V3.2 guard withheld the buffer whenever a DSML marker appeared anywhere in it, so once ordinary text diverged from a delimiter the buffer was never released. "Use <｜DSML｜function" followed by "ality" is the smallest case: the text is neither a tool call nor emittable, and with no end-of-stream flush it is lost. The same applies to DeepSeekV4Parser, which inherits this path. Replace the marker-presence and rstrip-endswith heuristics with the _ends_with_partial_token helper, matching the V3 and V3.1 parsers. The check covers the closing tokens as well, so a delimiter split across deltas is still withheld and stripped rather than leaking into content. Also annotate the new test and add the diverged-prefix case, which fails on the V3.2 and V4 parsers without this change. Signed-off-by: Yiğit ERDOĞAN <yigiterdogan023@gmail.com>
… as a tool call The V3.2 streaming path treated the bare "<｜DSML｜invoke" prefix appearing anywhere in the buffer as a tool call, so ordinary text that merely starts like the token, such as "<｜DSML｜invoke" followed by "ality", was routed into the tool-call branch. No invoke matched there, the buffer was kept, and the text was never emitted. DeepSeekV4Parser inherits this path. Key off the header up to its opening quote instead. Everything after that point is the arbitrary function name, so this is the longest fixed prefix ordinary text cannot reproduce by accident, and using the same string in the partial-token list keeps a header that arrives split across deltas buffered. The regression case now diverges from both tokens the parser looks for. Signed-off-by: Yiğit ERDOĞAN <yigiterdogan023@gmail.com>
… comments `str.replace` already scans the string, so guarding it with `in` only repeats the scan. The comments that motivated the change described the previous behaviour rather than the invariant the code now holds, which is noise for anyone reading only the current version. Signed-off-by: Yiğit ERDOĞAN <yigiterdogan023@gmail.com>
35b42bb to
8b382f6
Compare
|
Rebased onto Neither run published an L0 test tally, which usually means the pipeline ended Could you trigger one more |
|
@Yigtwxx please don't attempt rebase / merge main unless otherwise asked to. Our CI can skip tests already passed during retry, but you doing rebase will prevent that from working. |
|
/bot run |
|
PR_Github #66129 [ run ] triggered by Bot. Commit: |
|
Understood — apologies for the extra CI churn, and thanks for re-triggering. I won't rebase or merge main on open PRs unless asked. |
|
PR_Github #66129 [ run ] completed with state
|
|
@tongyuantongyu that run is red again on Could you paste the failing stage name, or the first few lines of its log? That For what it is worth, on the current tree the change is confined to three If it turns out to be infra rather than this PR, just say so and I will sit tight |
3 participants
Description
Fixes #17572.
parse_streaming_incrementin the DeepSeek tool parsers accumulates deltas intoself._bufferand withholds the buffer while it could still grow into a tool-call starttoken. When the ambiguity cleared, the buffer was cleared but only
new_text, the currentdelta, was returned, so everything withheld by an earlier increment was dropped from the
streamed response. Both start tokens begin with
<, so the trigger is ordinary assistanttext containing
<:detect_and_parseis unaffected because it sees the whole response and never buffers, sothe streamed and non-streamed content differ for the same generation.
The change is to emit the buffer instead of the delta. In
DeepSeekV3ParserandDeepSeekV31Parserthe partial-token check is also moved onto the buffer via the existingBaseToolParser._ends_with_partial_tokenhelper; testinge_token.startswith(new_text)is only correct when the buffer is empty, and it additionally means a start token split
across two deltas is emitted as normal text instead of being recognised.
DeepSeekV32Parserused substring heuristics instead: it withheld the buffer whenever aDSML marker appeared anywhere in it, and whenever the right-stripped buffer ended with one
of
<,<｜,</,</｜, and it treated a bare<｜DSML｜invokeanywhere in the bufferas a tool call. None of those release the buffer once ordinary text diverges from a
delimiter, so
"Use <｜DSML｜function"followed by"ality"is withheld forever and, withno end-of-stream flush, lost. The same holds for
"Use <｜DSML｜invoke"followed by"ality".They are replaced by the same
_ends_with_partial_tokencheck over the opening andclosing tokens, so a delimiter split across deltas is still withheld and stripped rather
than leaking into content, and the tool-call predicate now keys off
<｜DSML｜invoke name="rather than the bare prefix. That is the longest fixed prefix ofan invoke header, since everything after the opening quote is the arbitrary function name,
and it is also in the partial-token list so a header split anywhere inside it stays
buffered.
DeepSeekV4Parserinherits this path.This is the pattern the rest of the directory already uses.
BaseToolParser.parse_streaming_increment,gemma4_parser.py,minimax_m3_parser.py,poolside_v1_parser.py,qwen3_tool_parser.pyandkimi_k3_tool_parser.pyall check_ends_with_partial_tokenagainst the buffer and emit the buffer. The four DeepSeekparsers were the only ones that did not.
Scope is limited to tool-calling traffic, since
apply_tool_parseronly runs when a toolparser is configured and the request carries
tools. There is no API change and no changeto the non-streaming path.
Test Coverage
tests/unittest/llmapi/apps/test_tool_parsers.py, which is already registered ascpu_onlyintests/integration/test_lists/test-db/l0_cpu.yml:test_deepseek_streaming_preserves_withheld_textasserts that concatenating thenormal_textof every streamed increment reproduces the input. It runs over the fourDeepSeek parsers and three delta shapes: a delta that is itself a prefix of a start
token, a delta that ends on one after other text, and text that starts like a start
token and then diverges from it. The second shape is what makes
DeepSeekV32ParserandDeepSeekV4Parserdiscard a longer run of text at once; the third is the case thatwithheld the buffer permanently, and it diverges from both tokens those two parsers
look for.
Eight of the twelve cases fail on
mainand all twelve pass with this change. The rest ofthe file is unchanged and still passes, including the existing DeepSeek tool-call tests.
One thing found in review is deliberately left out: a delta that contains ordinary text
before a complete tool call drops that text. It is caused by the tool-parsing branch
rather than by the withhold guard, it affects all four parsers, and it needs its own test
shape, so it is filed separately as #17580.
The four DeepSeek test classes previously inherited only
test_parse_streaming_increment_normal_textandtest_parse_streaming_increment_partial_bot_tokenfromBaseToolParserTestClass, andneither streams text across more than one delta, which is why this was not caught.
PR Checklist
Please review the following before submitting your PR:
PR description clearly explains what and why. If using CodeRabbit's summary, please make sure it makes sense.
PR Follows TRT-LLM CODING GUIDELINES to the best of your knowledge.
Test cases are provided for new code paths (see test instructions)
If PR introduces API changes, an appropriate PR label is added - either
api-compatibleorapi-breaking. Forapi-breaking, includeBREAKINGin the PR title.Any new dependencies have been scanned for license and vulnerabilities
CODEOWNERS updated if ownership changes
Documentation updated as needed
Update tava architecture diagram if there is a significant design change in PR.
The reviewers assigned automatically/manually are appropriate for the PR.
Please check this after reviewing the above items as appropriate for this PR.
GitHub Bot Help
To see a list of available CI bot commands, please comment
/bot help.Dev Engineer Review
QA Engineer Review
tests/unittest/llmapi/apps/test_tool_parsers.py.normal_textand one-shot parsing.test-db/orqa/entries are reported.Verdict: needs follow-up