-
Notifications
You must be signed in to change notification settings - Fork 2.7k
[#17574][fix] Complete zero-argument tool calls in the streaming tool parser #17575
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: main
Are you sure you want to change the base?
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -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 | ||
|
Collaborator
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Checking this assertion against the running parser: after the fix
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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 It is a separate, pre-existing issue, and it is not specific to zero-argument The cause is the one you identified at So it is orthogonal to the argument-truthiness gate — it reproduces before this I have reworded the description: the buffer never advancing is the consequence |
||
|
|
||
|
|
||
| class TestQwen3CoderToolParser(BaseToolParserTestClass): | ||
| """Test suite for Qwen3CoderToolParser class.""" | ||
|
|
||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
get("arguments")also returnsNonewhen 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 withparameters='', the call never completes, and_bufferends as'<tool_call>\n{"name": "get_time"}\n</tool_call> All done.'- it only ever grows. Same for"arguments": null. Since:249is the only place the buffer advances, would it be worth normalising a missing/nullargumentsto{}inside theis_current_completebranch? The partial path stays safe there, because an unclosed JSON still falls through toelif prev_arguments:.There was a problem hiding this comment.
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-endidentically to the case this PR fixes. Reproduced on the patched tree with
Qwen3ToolParser, driving the chunks you gave:parameters_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:
Gating on
is_current_completeis what keeps the partial path safe, as younoted: 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 shapesnow emit
{}and leave_bufferat'\n</tool_call> All done.', matching theempty-object row.
test_streaming_zero_arg_toolis now parametrized over the three shapes(
empty_object,key_absent,explicit_null). The two new ones fail withoutthis 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_jsonresolves a missing key to{}(
base_tool_parser.py:83,act.get("arguments", {})), so streaming andone-shot now agree there — but for an explicit
nullit dumps"null", becauseact.get("arguments", {})returns the present-but-null value. Theparametrization 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.