Skip to content
Open
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
14 changes: 13 additions & 1 deletion tensorrt_llm/serve/tool_parser/base_tool_parser.py
Original file line number Diff line number Diff line change
Expand Up @@ -223,7 +223,19 @@ def parse_streaming_increment(self, new_text: str,
cur_arguments = current_tool_call.get("arguments")
res = StreamingParseResult()

if cur_arguments:
# A finished call may carry no "arguments" key at all, or an
# explicit null. Both mean "no arguments" and are normalized to
# {} so the call still completes, matching what parse_base_json
# returns on the non-streaming path. While the JSON is still
# partial a missing key only means "not streamed yet", so it is
# left alone and the elif prev_arguments branch keeps handling it.
if is_current_complete and cur_arguments is None:
cur_arguments = {}

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

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

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

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

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.

# Calculate how much of the arguments we've already streamed
sent = len(
self.streamed_args_for_tool[self.current_tool_id])
Expand Down
57 changes: 57 additions & 0 deletions tests/unittest/llmapi/apps/test_tool_parsers.py
Original file line number Diff line number Diff line change
Expand Up @@ -1016,6 +1016,63 @@ def test_streaming_wrapped_form_unregressed(self, sample_tools, parser):
assert len(r_args.calls) == 1
assert json.loads(r_args.calls[0].parameters) == {"location": "SF"}

@pytest.mark.parametrize(
"arguments_chunk,oneshot_params",
[
(', "arguments": {}}', "{}"),
("}", "{}"),
# detect_and_parse dumps an explicit null as-is; only the streaming
# path normalizes it. Asserted here so the divergence stays visible.
(', "arguments": null}', "null"),
],
ids=["empty_object", "key_absent", "explicit_null"],
)
def test_streaming_zero_arg_tool(self, parser, arguments_chunk,
oneshot_params):
"""Test streaming a zero-argument tool call.

A model can express "no arguments" as an empty object, by omitting the
key, or as an explicit null. All three have to complete the call and
stream "{}".
"""
tools = [
ChatCompletionToolsParam(
type="function",
function=FunctionDefinition(
name="get_time",
description="Get current time",
parameters={
"type": "object",
"properties": {},
},
),
)
]
chunks = [
"<tool_call>\n",
'{"name": "get_time"',
arguments_chunk,
"\n</tool_call>",
]

results = [
parser.parse_streaming_increment(chunk, tools) for chunk in chunks
]

names = [c.name for r in results for c in r.calls if c.name]
assert "get_time" in names

# A zero-argument call still has to stream its arguments, otherwise the
# client is left with arguments="", which is not valid JSON.
params = "".join(c.parameters for r in results for c in r.calls)
assert params == "{}", f"Expected '{{}}', got {params!r}"

# The completed call must also be consumed from the buffer, otherwise the
# parser stays in the tool-call branch and swallows the rest of the output.
oneshot = parser.detect_and_parse("".join(chunks), tools)
assert oneshot.calls[0].parameters == oneshot_params
assert "<tool_call>" not in parser._buffer

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

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.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

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.



class TestQwen3CoderToolParser(BaseToolParserTestClass):
"""Test suite for Qwen3CoderToolParser class."""
Expand Down
Loading