Skip to content

[None][fix] JSON tool-call streaming: release the arguments of a call that ends the stream - #21490

Closed
JunyiXu-nv wants to merge 4 commits into
NVIDIA:mainfrom
JunyiXu-nv:dev-junyix-fix-json-tool-call-final-step-args
Closed

JunyiXu-nv wants to merge 4 commits into
NVIDIA:mainfrom
JunyiXu-nv:dev-junyix-fix-json-tool-call-final-step-args

Conversation

@JunyiXu-nv

@JunyiXu-nv JunyiXu-nv commented Nov 3, 2026 •

Copy link
Copy Markdown
Collaborator

Description

Symptom. With --tool_parser qwen3, a streamed /v1/chat/completions response loses tool-call arguments when the last increment of the stream carries a whole <tool_call>{"name": ..., "arguments": {...}}</tool_call> call, or the part of one in which its name completes. The client receives the call's name with empty arguments and finish_reason: "tool_calls", so it runs the tool with no arguments. If that increment carries two calls, the second one never reaches the client at all. Large increments are common: with stream_interval above 1 (many configs under examples/configs use 10 or 20), the last increment carries everything generated since the previous one, and speculative decoding adds accepted draft tokens on top. Streaming /v1/messages, which is reframed from the chat chunks, emits a tool_use block with empty input. Non-streaming responses are correct, and so is streaming /v1/responses, which takes this parser's calls from a whole-text parse at the end of the stream.

Cause. BaseToolParser.parse_streaming_increment is the streaming path Qwen3ToolParser uses for <tool_call> JSON calls (through super()). It sends a call's name in the increment that first parses it and releases the arguments in a later increment, and it completes at most one call per increment. finish() is a no-op for this path, so arguments that wait for a later increment are lost when the stream ends first, and a complete call behind a completed one in the same increment is dropped.

Change.

  • A call whose JSON is already complete when its name is parsed now goes out as one ToolCallItem that carries the name and the arguments.
  • An increment that completes a call keeps parsing what is left in the buffer (parse_streaming_increment now runs _parse_buffered_tool_call until a pass consumes nothing), so every complete call is released by the increment that completes it.
  • The end-of-call bookkeeping that both paths share moved into _complete_current_tool.

Calls that are still being generated stream as before: the name once it is complete, then argument fragments.

Parsers. Qwen3ToolParser, for <tool_call> calls, is the only parser in ToolParserFactory that runs this streaming path. Its bare-JSON fallback has its own path, which already sends the name and the arguments in the same increment. The other parsers override parse_streaming_increment and are not changed.

Behaviour. When one increment holds a whole call, this parser now returns a single item with the name and the arguments, where before it returned the name only and the arguments one increment later (or never). An increment holding several complete calls returns all of them. For this parser, a chat chunk therefore carries each call index at most once. Calls whose JSON is still partial at the end of an increment stream exactly as before. There is no API or configuration change.

Test Coverage

In tests/unittest/llmapi/apps/test_tool_parsers.py (CPU only, already in l0_cpu.yml):

  • TestBaseToolParser::test_parse_streaming_increment_releases_every_complete_call: the base class's own format with two whole calls and trailing text in one increment returns both calls with their arguments, plus the text, in that increment; finish() has nothing left.
  • TestQwen3StreamingMatchesWholeTextParse::test_streamed_calls_match_whole_text_parse[one_call, two_calls, text_around_calls]: the streamed calls, assembled by index as a client does (with finish() at the end), equal detect_and_parse on the whole text (positions, names, decoded arguments). This is checked for every chunking: the whole output in one increment, one character per increment, sizes 2/3/5/8/13, and every two-way split. No increment may repeat a tool index.
  • TestQwen3StreamingMatchesWholeTextParse::test_last_increment_sends_the_arguments_with_the_name[whole_call_in_last_increment, last_increment_has_the_rest]: a call first named by the last increment arrives in that increment as one item with its arguments.
  • TestQwen3StreamingMatchesWholeTextParse::test_two_calls_in_last_increment_are_both_sent: both calls of one increment are released, one item each.
  • TestQwen3StreamingMatchesWholeTextParse::test_chat_stream_carries_the_arguments_to_the_end[whole_output_in_one_step, calls_in_last_step, last_step_has_the_rest]: through chat_stream_post_processor, the SSE tool_calls assembled by index equal the whole-text parse, no chunk repeats an index, and the last chunk has finish_reason: "tool_calls".

Results: the 10 new tests fail on main (10 failed) and pass with this change (10 passed). On main, 34 of 121, 213 of 219 and 252 of 258 chunkings end with other calls than the whole-text parse. Reverting the fix (no arguments with the name, one pass per increment) makes all 10 fail again. Of 10 further bugs planted one at a time in the changed code, 9 are caught; the one missed only resets a value that is never read again. Over tests/unittest/llmapi/apps, the only outcome changes from main are these 10 tests going from failed to passed. That includes the Responses streaming and Anthropic adapter suites, and the existing Responses test that streams the whole Qwen3 output in one frame. Locally, a Hypothesis property run (not checked in) also found no mismatch: it generated outputs with 1-3 calls, nested arguments, arguments/parameters/missing/null keys, either key order, several JSON layouts and text around the calls, and split them at arbitrary points.

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

BaseToolParser.parse_streaming_increment now processes buffered text until no more complete calls are consumed. When a call’s JSON is complete as its name is emitted, the parser emits its arguments with the name and completes the call. Completed calls with missing or falsy arguments normalize to {}. No exported or public declarations changed. No review findings or severity counts were supplied.

QA Engineer Review

tests/unittest/llmapi/apps/test_tool_parsers.py adds coverage for multiple complete calls in one increment, calls completed in the final increment, and Qwen3 streaming parity with whole-text parsing across varied chunking. Chat-stream tests check that call arguments are preserved. The test file is listed in tests/integration/test_lists/test-db/l0_cpu.yml. Test results were not supplied. Coverage verdict: sufficient for the reported streaming cases.

Per-File QA Perspective

  • tensorrt_llm/serve/tool_parser/base_tool_parser.py: Verify same-increment argument emission, continued parsing of buffered calls, {} normalization for completed calls, and completion-state cleanup. Check behavior in other parsers that use the base implementation.
  • tests/unittest/llmapi/apps/test_tool_parsers.py: Covers base-parser multi-call handling and Qwen3 call assembly across varied chunking, including final-increment and chat-stream cases. The file is listed in the CI test database.

… that ends the stream

BaseToolParser.parse_streaming_increment, the streaming path Qwen3ToolParser
uses for <tool_call> JSON calls, sent a call's name in the increment that
first parsed it and the arguments in a later one, and completed at most one
call per increment. finish() is a no-op for this path, so when that later
increment never came, because the call arrived in the last increment of the
stream, the arguments were lost; a second call in the same increment was
lost entirely. A streaming client then ran the tool with empty arguments,
or never saw the second call.

A call whose JSON is already complete when its name is parsed now goes out
as one item carrying its arguments, and an increment that completes a call
keeps parsing what is left in the buffer, so every complete call is
released by the increment that completes it. Calls still being generated
stream as before.

Signed-off-by: JunyiXu-nv <219237550+JunyiXu-nv@users.noreply.github.com>
@JunyiXu-nv
JunyiXu-nv requested a review from a team as a code owner November 3, 2026 01:16
@coderabbitai

coderabbitai Bot commented Nov 3, 2026 •

Copy link
Copy Markdown
Contributor

Review in Change Stack →

No actionable comments were generated in the recent review.

Walkthrough

The base tool parser processes buffered text until parsing makes no progress. It emits arguments when calls complete and resets per-call state. Tests cover multiple calls, varied chunking, final-increment completion, trailing text, and chat-stream output.

Changes

Streaming tool-call completion

Layer / File(s) Summary
Process and complete buffered calls
tensorrt_llm/serve/tool_parser/base_tool_parser.py
The parser processes buffered text until parsing makes no further progress. Completed calls include arguments, with missing or falsy arguments normalized to {}. Completion uses a centralized state-reset path.
Validate streamed call output
tests/unittest/llmapi/apps/test_tool_parsers.py
Tests compare streamed calls with whole-text parsing across chunkings. They check multiple calls in one increment, final-increment completion, trailing text, and chat-stream output.

Estimated code review effort: 3 (Moderate) | ~20 minutes

Change: Bug fix

Merge Risk: Minimal · up to ebe80

No actionable current-head defect was found in the reviewed parser behavior or its covered streaming integrations.

Pre-merge checks: Passed 5
Check name Status Explanation
Title check Passed The title clearly describes a real and important part of the change: releasing tool-call arguments when the stream ends. It is concise and uses the required ticket and type format, although it does no…
Description check Passed The description is complete and relevant. It explains the symptom, cause, implementation, affected parsers, behavior, and detailed test coverage. The checklist is only partially marked, but the requir…
Docstring Coverage Passed Docstring coverage is 84.62% which is sufficient. The required threshold is 80.00%. Docstring coverage is scoped to functions touched by this diff with cyclomatic complexity of at least 2. Documentati…
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: Title check

Explanation

The title clearly describes a real and important part of the change: releasing tool-call arguments when the stream ends. It is concise and uses the required ticket and type format, although it does not mention the additional multiple-call handling.

Full details: Description check

Explanation

The description is complete and relevant. It explains the symptom, cause, implementation, affected parsers, behavior, and detailed test coverage. The checklist is only partially marked, but the required issue, solution, and test information is present.

Full details: Docstring Coverage

Explanation

Docstring coverage is 84.62% which is sufficient. The required threshold is 80.00%. Docstring coverage is scoped to functions touched by this diff with cyclomatic complexity of at least 2. Documentation-only edits and deletions within surviving functions are included; unchanged diff context and fully deleted functions are excluded. Analyzed 13 functions across 2 files.

Finishing Touches
Generate unit tests (beta)
  • Create a new PR
  • Autofix · Keep fixing CodeRabbit findings and required CI, and resolving merge conflicts

Tip

🎯 Customize your risk assessments

Guide how CodeRabbit assesses merge risk and security architecture risk. Highlight sensitive changes and explain your security boundaries using Merge Risk Instructions and Security Risk Instructions in your review settings. Security architecture risk assessments require the Advanced plan or above.

✅ Automatic approvals, with your rules

Let CodeRabbit approve pull requests once required threads are resolved, pre-merge checks pass, and your risk limits are met. Enable Automatic approvals in review settings, with overrides for teams or file paths.

CodeRabbit Verify

  • Run CodeRabbit Verify to test the changes in this pull request.
Recent review info
Run configuration
Commits

Reviewing files that changed from the base of the PR and between 08a843e and ebe8089.

Files selected for processing: 1
  • tests/unittest/llmapi/apps/test_tool_parsers.py

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

@coderabbitai coderabbitai Bot left a comment •

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

  • Fix comments: Fix CodeRabbit comments on this PR

…he whole-text parse

Streamed JSON tool calls now turn missing or falsy arguments (null, [], "", 0,
false) into {} once a call is complete, exactly as parse_base_json does for the
whole text. Before, a call read in one increment, or finished after its name
went out, streamed the raw falsy value, so a Qwen3 client got different
arguments from streaming and non-streaming requests. Falsy-argument outputs
join the streamed-versus-whole-text chunking test, which covers the
one-increment case.

Signed-off-by: JunyiXu-nv <219237550+JunyiXu-nv@users.noreply.github.com>
@JunyiXu-nv

Copy link
Copy Markdown
Collaborator Author

/bot run --disable-fail-fast

@tensorrt-cicd

Copy link
Copy Markdown
Collaborator

PR_Github #82163 [ run ] triggered by Bot. Commit: 988e7e1 Link to invocation

@coderabbitai coderabbitai Bot left a comment •

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

  • Fix comments: Fix CodeRabbit comments on this PR

…the whole-text parse

Add Qwen3 cases with text after the end token, in the final increment and in a
later one. The stream, finish() included, surfaces none of it as normal text,
matching detect_and_parse, which keeps only the text before the first call. The
parser is unchanged: the end token and the text after it stay in the buffer, as
they already do when they arrive in a later increment.

Signed-off-by: JunyiXu-nv <219237550+JunyiXu-nv@users.noreply.github.com>
@JunyiXu-nv

Copy link
Copy Markdown
Collaborator Author

/bot run --disable-fail-fast

@coderabbitai coderabbitai Bot left a comment •

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

  • Fix comments: Fix CodeRabbit comments on this PR

@tensorrt-cicd

Copy link
Copy Markdown
Collaborator

PR_Github #82188 [ run ] triggered by Bot. Commit: 08a843e Link to invocation

@tensorrt-cicd

Copy link
Copy Markdown
Collaborator

PR_Github #82163 [ run ] completed with state ABORTED. Commit: 988e7e1

Link to invocation

@tensorrt-cicd

Copy link
Copy Markdown
Collaborator

PR_Github #82188 [ run ] completed with state SUCCESS. Commit: 08a843e
/LLM/main/L0_MergeRequest_PR pipeline #67817 completed with status: 'FAILURE'

CI Report

⚠️ Action Required:

  • Please check the failed tests and fix your PR
  • If you cannot view the failures, ask the CI triggerer to share details
  • Once fixed, request an NVIDIA team member to trigger CI again

CI Agent Failure Analysis

Link to invocation

@JunyiXu-nv

Copy link
Copy Markdown
Collaborator Author

/bot run --disable-fail-fast

@tensorrt-cicd

Copy link
Copy Markdown
Collaborator

PR_Github #82254 [ run ] triggered by Bot. Commit: 08a843e Link to invocation

@tensorrt-cicd

Copy link
Copy Markdown
Collaborator

PR_Github #82254 [ run ] completed with state SUCCESS. Commit: 08a843e
/LLM/main/L0_MergeRequest_PR pipeline #67880 completed with status: 'SUCCESS'

CI Report

Link to invocation

Compare the streamed and whole-text calls with the expected get_weather call,
not only with each other, so a parser that drops a call followed by text in
both paths fails the test.

Signed-off-by: JunyiXu-nv <219237550+JunyiXu-nv@users.noreply.github.com>
@JunyiXu-nv

Copy link
Copy Markdown
Collaborator Author

/bot run --disable-fail-fast

@tensorrt-cicd

Copy link
Copy Markdown
Collaborator

PR_Github #82283 [ run ] triggered by Bot. Commit: ebe8089 Link to invocation

@tensorrt-cicd

Copy link
Copy Markdown
Collaborator

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

CI Report

Link to invocation

@JunyiXu-nv

Copy link
Copy Markdown
Collaborator Author

Closing in favour of #21975, which fixes the same loss once, in BaseToolParser.finish(), for every parser that keeps the default finish(), this one included.

@JunyiXu-nv JunyiXu-nv closed this Nov 10, 2026
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants