[#17574][fix] Complete zero-argument tool calls in the streaming tool parser - #17575
Conversation
|
@zhaoyangwang-nvidia @JunyiXu-nv could one of you trigger a pipeline run when you have a moment? The change is one condition in Related but separate: #17572 / #17573 covers a text-loss bug in the DeepSeek tool parsers. The two do not overlap in files. |
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
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 (2)
🚧 Files skipped from review as they are similar to previous changes (1)
Included review availability: Your plan provides up to 12 included reviews per hour; 11 remain after this review. WalkthroughThe tool parser normalizes null, omitted, and empty arguments to ChangesTool argument normalization
Estimated code review effort: 2 (Simple) | ~10 minutes Merge Risk: ⚪ Minimal · up to This localized change completes zero-argument tool calls consistently in streaming and non-streaming parsing and adds focused coverage; no actionable merge-blocking risk remains beyond normal checks and review. Suggested reviewers: 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
Full details: Linked Issues checkExplanation The changes satisfy issue Full details: Out of Scope Changes checkExplanation The code changes remain within the linked issue scope. The non-streaming null normalization supports the required stream/non-stream consistency, and the separately identified post-completion content-routing issue is explicitly excluded. ✨ 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.
🧹 Nitpick comments (1)
tests/unittest/llmapi/apps/test_tool_parsers.py (1)
1053-1057: 🎯 Functional Correctness | 🔵 Trivial | ⚡ Quick winCover response content after the completed tool call.
Line 1057 checks that the opening marker is absent, but the test does not send response content after
</tool_call>. Add a follow-up chunk and assert that it producesnormal_textwith no tool calls. This directly covers the PR objective for subsequent response content.Suggested assertion
assert "<tool_call>" not in parser._buffer + follow_up = parser.parse_streaming_increment( + "The current time is 12:00.", tools) + assert follow_up.calls == [] + assert follow_up.normal_text == "The current time is 12:00."🤖 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 1053 - 1057, Extend the completed tool-call test around parser.detect_and_parse by appending a response-content chunk after </tool_call>, then assert the follow-up result contains that content as normal_text and has no tool calls. Keep the existing buffer-consumption assertions intact.
🤖 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 `@tests/unittest/llmapi/apps/test_tool_parsers.py`:
- Around line 1053-1057: Extend the completed tool-call test around
parser.detect_and_parse by appending a response-content chunk after
</tool_call>, then assert the follow-up result contains that content as
normal_text and has no tool calls. Keep the existing buffer-consumption
assertions intact.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Enterprise
Run ID: 302aa629-aef1-472d-af42-860b32485833
📒 Files selected for processing (2)
tensorrt_llm/serve/tool_parser/base_tool_parser.pytests/unittest/llmapi/apps/test_tool_parsers.py
zhaoyangwang-nvidia
left a comment
zhaoyangwang-nvidia
left a comment
There was a problem hiding this comment.
Approve with nits.
|
/bot run |
|
PR_Github #66612 [ run ] triggered by Bot. Commit: |
|
PR_Github #66612 [ run ] completed with state
|
|
@zhaoyangwang-nvidia the fold-in is pushed as On the previous one: #66612 came back If the new run is red too, could you paste the failing stage name or the first |
|
/bot run |
|
PR_Github #66921 [ run ] triggered by Bot. Commit: |
|
PR_Github #66921 [ run ] completed with state
|
|
@zhaoyangwang-nvidia when you have a moment, could I ask for one more #17573 ran three pipelines on its final commit Both runs here match that signature and stopped early: #66612 on Happy to act immediately if a run does surface something in the code. |
|
/bot run |
|
PR_Github #67235 [ run ] triggered by Bot. Commit: |
|
PR_Github #67235 [ run ] completed with state
|
8f763f5 to
1ae1af9
Compare
|
Note GitHub couldn't provide a complete incremental comparison for this pull request, so CodeRabbit is performing a full review instead. This review may take a little longer. |
|
@zhaoyangwang-nvidia I rebased onto current The reason I touched the branch this time rather than asking for a fourth run on The diff is unchanged. Re-verified against the rebased tree, not just the old one: the four cases this PR adds pass, reverting On the three runs against |
|
/bot run --disable-fail-fast |
|
PR_Github #68655 [ run ] triggered by Bot. Commit: |
|
PR_Github #68655 [ run ] completed with state
|
|
@zhaoyangwang-nvidia thanks for running #68655 with I can't see what failed: What I can verify locally on
If the failures turn out to be in stages unrelated to tool parsing, I'd rather not keep asking for re-runs blind — knowing which stage broke is enough for me to either fix it or say clearly that it's not from this PR. |
|
Correction to my comment above, before anyone spends time on it. I checked the two public signals and #68655 published no test tally. The So despite the ~2h53m runtime, this run does not look like it got to the tests either. The duration read I gave above was wrong — #68655 has the same countless-failure signature as #66612, #66921 and #67235, which points at build/infra rather than at this diff. That narrows the ask rather than changing it: if you can see the stage list for pipeline #56061, all I really need is which stage broke. If it's a build or setup stage, another |
…g tool parser
BaseToolParser.parse_streaming_increment gated the argument-streaming and
call-completion branch on the truthiness of the parsed argument object. An empty
object is falsy, so a tool call with no arguments never emitted its arguments and
was never consumed from the buffer: the client was left with arguments="", which
is not valid JSON, and has_tool_call stayed true for the rest of the request, so
every later chunk was routed back into the tool-call branch instead of being
emitted as content.
Gate on "is not None" instead. The partial path is unaffected because the
elif prev_arguments branch keeps its own falsy guard, so no premature "{}" is
emitted while the JSON is still incomplete.
Qwen3ToolParser is the parser that reaches this code, through _wrapped_streaming.
Glm4ToolParser and Glm47ToolParser already assert the expected behaviour for their
own streaming paths, so the new test mirrors theirs.
Signed-off-by: Yiğit ERDOĞAN <yigiterdogan023@gmail.com>
…e arguments key
Review follow-up. Gating on "is not None" fixed the empty-object shape but left
two equivalent ones dead-ending in exactly the same way: a model can express "no
arguments" by omitting the key entirely, or by emitting an explicit null. Both
make current_tool_call.get("arguments") return None, so the completion branch is
skipped, the buffer never advances past the call, and _buffer only grows for the
rest of the request.
Normalize a missing or null arguments object to {} once the call's JSON is
complete. Restricting it to is_current_complete keeps the partial path intact:
while the JSON is still incomplete a missing key only means "not streamed yet",
so it must not be mistaken for an empty object and flushed early.
This matches parse_base_json, which already resolves a missing key to {} on the
non-streaming path. It still dumps an explicit null as "null" there; the new
parametrization asserts that so the divergence stays visible.
Signed-off-by: Yiğit ERDOĞAN <yigiterdogan023@gmail.com>
…in parse_base_json
act.get("arguments", {}) only defaults when the key is absent, so a
present-but-null value was dumped as "null" while the streaming path
completes the same call with "{}". Both paths now agree on {}.
`or {}` normalizes every falsy value, not just None: "arguments" of [], ""
or 0 become {} where they used to be dumped as-is. All are malformed for a
tool call, and act.get("parameters") or ... on the left of the same
expression already has that property, so it is not a new hazard.
parse_base_json is shared by Qwen3, GLM4, GLM4.7 and DeepSeek V3/V3.1/V3.2;
none of their tests assert "null" for this shape.
Signed-off-by: Yiğit ERDOĞAN <yigiterdogan023@gmail.com>
1ae1af9 to
5d3e4f5
Compare
|
/bot run --disable-fail-fast |
|
Note GitHub couldn't provide a complete incremental comparison for this pull request, so CodeRabbit is performing a full review instead. This review may take a little longer. |
1 similar comment
|
Note GitHub couldn't provide a complete incremental comparison for this pull request, so CodeRabbit is performing a full review instead. This review may take a little longer. |
@Yigtwxx Sorry for the slow turnaround — I was out on PTO the last couple of days. Your read on #68655 is right: it never got to your tests. Here is the stage breakdown for pipeline #56061 (1ae1af9), and none of it points at your diff. Failing orchestrator stages: [Test-x86_64-Single-GPU] Remote Run and [Test-SBSA-Single-GPU] Remote Run, plus the follow-on Collect Test Result / Rerun Report / Test Coverage. Four independent causes, all of them before any test executes: Test collection produced an empty list. pytest --collect-only --test-list=.../l0_gb10_cleaned.txt returned no tests collected (4237 deselected), exit code 5, then Test collection failed for shard 1/1. Cannot proceed without valid test list. This is a CI test-db problem — the same family as the INVALID TEST NAME FILTERS error on unittest/disaggregated/test_openai_disagg_server.py that killed the earlier run #54765 in Check Test List. That said, the branch had gone stale again — it was 134 commits behind main, and several of these test-list/test-db breakages are already fixed upstream, so re-running the same commit just keeps re-testing a known-bad environment. So I went ahead and rebased it onto current main for you and re-triggered CI |
|
PR_Github #68945 [ run ] triggered by Bot. Commit: |
|
PR_Github #68945 [ run ] completed with state
|
|
@zhaoyangwang-nvidia thank you for the stage breakdown and for doing the rebase — that answered the question I was stuck on, and the #68945 on
So the rebase did not change the outcome, which I think rules out staleness as the cause — the four things you listed (empty test-list collection on Re-verified locally on the rebased head, in case it is useful:
Given that, I do not think there is anything left on my side to fix — but I would rather hear that from you than assume it. If the test-db and infra fixes you mentioned have landed since, another |
|
@Yigtwxx One correction: pipeline #56327 did reach the test stage, so the "no That is the Wan2.2 TI2V-5B / Cache-DiT visual-generation path — no overlap with It is also not specific to this PR: across builds 56290-56340 (different PRs) that same test file is 12 FAILED / 6 PASSED with an identical signature, and it is not in Nothing to fix here. Re-triggering now — if it lands on the ~1/3 of runs where Wan2.2 passes, this should go green. |
|
/bot run --disable-fail-fast |
|
PR_Github #69028 [ run ] triggered by Bot. Commit: |
|
PR_Github #69028 [ run ] completed with state |
3 participants
Description
Fixes #17574.
BaseToolParser.parse_streaming_incrementgates the argument-streaming andcall-completion branch on the truthiness of the parsed argument object
(
base_tool_parser.py:226). A tool call with no arguments never satisfies that gate, sothe branch at
:239-253is never entered. Two things follow: the arguments are neverstreamed, so the client is left with
arguments="", which is not valid JSON, andself._bufferis never advanced past the completed call, so the call never finishes andthe buffer only grows for the remainder of the request.
A model can express "no arguments" in three ways, and all three hit this. Driving
Qwen3ToolParserchunk by chunk, followed by a trailingAll done.:parameters_buffer{"name": "get_time", "arguments": {}}'''<tool_call>\n{"name": "get_time", "arguments": {}}\n</tool_call> All done.'{"name": "get_time"}'''<tool_call>\n{"name": "get_time"}\n</tool_call> All done.'{"name": "get_time", "arguments": null}'''<tool_call>\n{"name": "get_time", "arguments": null}\n</tool_call> All done.'An empty object is falsy; a missing key and an explicit
nullboth makecurrent_tool_call.get("arguments")returnNone.detect_and_parseon the same textreturns
parameters='{}'for the first two, so this is a divergence between the streamedand non-streamed response for the same generation.
The change is in two parts:
is not Nonerather than truthiness, which covers the empty object.nullarguments object to{}, but only once the call's JSON iscomplete. Restricting it to
is_current_completekeeps the partial path intact: whilethe JSON is still unclosed a missing key only means "not streamed yet", so it must not
be mistaken for an empty object and flushed early. An unclosed object still falls
through to the
elif prev_arguments:branch at:256, which keeps its own falsy guard.After the change all three shapes emit
{}and advance the buffer past the call.Qwen3ToolParseris the parser that reaches this code, through_wrapped_streaming, sothis covers the models that resolve to the
qwen3tool parser:qwen2,qwen3,qwen3_moe,qwen3_5,qwen3_5_moeandqwen3_next.Glm4ToolParserandGlm47ToolParserimplement their own streaming paths and already emit"{}"here, whichis where the expected behaviour comes from. No API change.
A third part was added on review, on the non-streaming side.
parse_base_json:83readsact.get("parameters") or act.get("arguments", {}), and that default only applies whenthe key is absent, so a present-but-null value was dumped as
"null"while streamingcompleted the same call with
"{}"— the same stream/non-stream divergence this PR isabout, left in place for a third of the cases. It now reads
act.get("parameters") or act.get("arguments") or {}. Two consequences worth stating:or {}normalizes every falsy value, so"arguments"of[],""or0become{}where they used to be dumped as-is — all malformed for a tool call, and the
act.get("parameters") or ...on the left already behaved that way — andparse_base_jsonis shared, so this reaches Qwen3, GLM4, GLM4.7 and DeepSeekV3/V3.1/V3.2. The DeepSeek and GLM parsers build their
match_resultwith a"parameters"key, so theargumentsleg is unreachable from them, and no test asserts"null"for this shape.Out of scope
Review raised a second symptom that an earlier revision of this description attributed to
the same cause: after a completed call, later chunks are routed back into the tool-call
branch instead of being emitted as content. That is real, but it has a different cause and
this PR does not fix it.
base_tool_parser.py:136-138also enters the tool-call branchwhen
current_tool_id > 0 and current_text.startswith(self.tool_call_separator). ForQwen3ToolParserthe separator is"\n"andeot_tokenis"\n</tool_call>", so theremainder after any completed call begins with the separator by construction of the
format. It reproduces on
mainwith ordinary non-empty arguments, independently of theargument-truthiness gate, and fixing it means changing when that clause is allowed to
match — a separate concern in a method shared by every parser inheriting the base
streaming path. Tracked separately as #17740.
Test Coverage
tests/unittest/llmapi/apps/test_tool_parsers.py, already registered ascpu_onlyintests/integration/test_lists/test-db/l0_cpu.yml:TestQwen3ToolParser::test_streaming_zero_arg_toolstreams a zero-argument call in fourchunks and asserts the emitted arguments are
{}, thatdetect_and_parseon the sametext agrees, and that the call is consumed from the buffer. It is parametrized over the
three shapes above (
empty_object,key_absent,explicit_null). It mirrorsTestGlm47ToolParser::test_streaming_zero_arg_toolandTestGlm4ToolParser::test_streaming_no_args, which pin the same behaviour for thoseparsers.
TestBaseToolParser::test_parse_base_json_null_argumentspins the non-streaming half,next to the existing
test_parse_base_json_missing_parameters: an explicit"arguments": Nonenow resolves to{}like an absent key does.All three streaming cases fail on
mainwithassert '' == '{}'and pass with thischange; all three now also assert
parameters == "{}"on the one-shot path, which nolonger diverges. Reverting the one-token
parse_base_jsonchange fails exactlytest_parse_base_json_null_argumentsandtest_streaming_zero_arg_tool[explicit_null].The rest of the file is unchanged and still passes, including every other parser that
inherits the base streaming path.
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
nullarguments to{}.QA Engineer Review
tests/unittest/llmapi/apps/test_tool_parsers.py.nullarguments.{}output, one-shot parsing, call completion, and buffer cleanup.test-db/orqa/entries were identified.