[#17574][fix] Complete zero-argument tool calls in the streaming tool parser - #17575
[#17574][fix] Complete zero-argument tool calls in the streaming tool parser#17575Yigtwxx wants to merge 2 commits into
Conversation
…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>
|
@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. |
|
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 (2)
WalkthroughThe streaming tool parser now normalizes missing or ChangesZero-argument streaming tool calls
Estimated code review effort: 2 (Simple) | ~10 minutes Merge Risk: ⚪ Minimal · up to This localized parser fix aligns streamed zero-argument tool calls with the existing non-streaming behavior and adds targeted coverage; 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.
🧹 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.
| # An empty argument object is falsy but still has to be streamed and | ||
| # completed, otherwise a zero-argument tool call never finishes and its | ||
| # text stays in the buffer forever. | ||
| if cur_arguments is not None: |
There was a problem hiding this comment.
get("arguments") also returns None when the key is absent, or when the model emits "arguments": null - for a zero-argument call both are plausible outputs, and they dead-end in exactly the same way this PR fixes. I ran the parser standalone on ['<tool_call>\n', '{"name": "get_time"', '}', '\n</tool_call>', ' All done.'] with this patch applied: the name is emitted with parameters='', the call never completes, and _buffer ends as '<tool_call>\n{"name": "get_time"}\n</tool_call> All done.' - it only ever grows. Same for "arguments": null. Since :249 is the only place the buffer advances, would it be worth normalising a missing/null arguments to {} inside the is_current_complete branch? The partial path stays safe there, because an unclosed JSON still falls through to elif prev_arguments:.
There was a problem hiding this comment.
You are right, and it is worse than "also returns None" — both shapes dead-end
identically to the case this PR fixes. Reproduced on the patched tree with
Qwen3ToolParser, driving the chunks you gave:
| shape | streamed parameters |
final _buffer |
|---|---|---|
{"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.' |
{"name": "get_time", "arguments": {}} |
'{}' |
'\n</tool_call> All done.' |
So the first two never complete and the buffer only grows, exactly as you
describe. Applied your suggestion — normalising inside the completion branch:
cur_arguments = current_tool_call.get("arguments")
res = StreamingParseResult()
if is_current_complete and cur_arguments is None:
cur_arguments = {}
if cur_arguments is not None:
...Gating on is_current_complete is what keeps the partial path safe, as you
noted: while the 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 elif prev_arguments:. Both new shapes
now emit {} and leave _buffer at '\n</tool_call> All done.', matching the
empty-object row.
test_streaming_zero_arg_tool is now parametrized over the three shapes
(empty_object, key_absent, explicit_null). The two new ones fail without
this commit and pass with it; the full file is otherwise unchanged.
One divergence I left alone, since it is on the non-streaming path and outside
this PR's title: parse_base_json resolves a missing key to {}
(base_tool_parser.py:83, act.get("arguments", {})), so streaming and
one-shot now agree there — but for an explicit null it dumps "null", because
act.get("arguments", {}) returns the present-but-null value. The
parametrization asserts that as current behaviour so the divergence is visible
rather than silent. Happy to fold act.get("parameters") or act.get("arguments") or {} into this PR if you would rather have both paths consistent now.
| # parser stays in the tool-call branch and swallows the rest of the output. | ||
| assert parser.detect_and_parse("".join(chunks), | ||
| tools).calls[0].parameters == "{}" | ||
| assert "<tool_call>" not in parser._buffer |
There was a problem hiding this comment.
Checking this assertion against the running parser: after the fix _buffer is '\n</tool_call> All done.', so this holds - but the second symptom in the PR description ("every later chunk is routed back into the tool-call branch instead of being emitted as content") still reproduces. Feeding a trailing ' All done.' chunk returns normal_text='' and leaves it in the buffer. The cause is no longer has_tool_call(): it is the second clause at base_tool_parser.py:137-138, current_tool_id > 0 and current_text.startswith(self.tool_call_separator). The separator is '\n' and the remainder after a completed call is '\n</tool_call>', so the parser stays in the tool-call branch, hits MalformedJSON, and returns without touching the buffer. It behaves correctly when the remainder does not start with a newline. Is that a known separate issue? If so the PR description may be claiming a bit more than the change delivers; the assertion itself looks correctly calibrated to what does hold.
There was a problem hiding this comment.
Confirmed on all three points — your diagnosis is right and my PR description
over-claims. I have corrected it, and filed the second symptom as #17740.
It is a separate, pre-existing issue, and it is not specific to zero-argument
calls. The same drop happens with ordinary arguments on main:
in='<tool_call>\n' normal_text='' calls=[]
in='{"name": "get_weather", "arguments": {"city": "Paris"}}'
normal_text='' calls=[(0, 'get_weather', '')]
in='\n</tool_call>' normal_text='' calls=[(0, None, '{"city": "Paris"}')]
in=' It is sunny.' normal_text='' calls=[]
FINAL _buffer = '\n</tool_call> It is sunny.'
The cause is the one you identified at base_tool_parser.py:136-138. For
Qwen3ToolParser, tool_call_separator is "\n" and eot_token is
"\n</tool_call>", so the remainder after any completed call begins with the
separator by construction of the format. The second clause therefore always
holds, the parser stays in the tool-call branch, partial_json_loads raises
MalformedJSON, and :186 returns without touching the buffer. The
</tool_call> scrubbing in _wrapped_streaming never runs because
result.normal_text is empty. Your control case is the giveaway: with
'</tool_call>' instead of '\n</tool_call>' the trailing text is emitted and
_buffer drains to ''.
So it is orthogonal to the argument-truthiness gate — it reproduces before this
PR, after it, and with non-empty arguments — and fixing it means changing when
the separator clause is allowed to match, which is a different concern in a
method shared by every parser inheriting the base streaming path. I would rather
not fold it in here, so #17740 carries the repro and links back to this thread.
Say the word if you would prefer it handled in this PR after all.
I have reworded the description: the buffer never advancing is the consequence
this change actually fixes; the "later chunks routed back into the tool-call
branch" symptom is now attributed to the separator clause and pointed at #17740.
The assertion stays as-is, since as you say it is calibrated to what does hold.
…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>
2 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, and no change to the
non-streaming path.
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.
All three cases fail on
mainwithassert '' == '{}'and pass with this change. Theexplicit_nullcase additionally pins current one-shot behaviour:parse_base_jsonresolves a missing key to
{}(:83) but dumps a present-but-null value as"null", sothe parametrization records that divergence rather than leaving it silent. 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.