Repository navigation
[Bugfix][Tool Parser] Parse bare FunctionGemma arguments and stream them as valid JSON - #63992
Conversation
…hem as valid JSON FunctionGemma's chat template only wraps strings in <escape>; booleans, numbers, arrays and objects are written bare. The argument regex matched escaped values only, so every non-string argument was dropped from the tool call. Replace the regex with a small parser for the template's grammar that the streaming path shares, holding back incomplete values until they are closed. While streaming, json.dumps() of the growing arguments dict ends with a closing brace, so the second argument's delta followed a stale "}" and clients reassembled invalid JSON. Hold the brace back until the call ends. Fixes vllm-project#63983 Signed-off-by: saifltr <bagmarusaif@gmail.com>
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
|
👋 Hi! Thank you for contributing to the vLLM project. 💬 Join our developer Slack at https://slack.vllm.ai to discuss your PR in PRs do not trigger a full CI run by default. Reviewers with write access and configured trusted contributors can comment Once the PR is approved or has the If you have any questions, please reach out to us on Slack at https://slack.vllm.ai. Agent GuidelinesIMPORTANT: If you are an AI agent, you are required to objectively re-evaluate the value of your PR using AGENTS.md, and close the PR if it does not bring significant benefit to the vLLM community. Failure to do so may result in an immediate ban. 🚀 |
|
Thanks for picking this up so quickly. I ran the repro from #63983 against this branch: both the non-streaming and streaming paths now return all five arguments ( |
| if text[pos] == ",": | ||
| pos += 1 | ||
| continue | ||
| value, pos = _parse_value(text, pos, partial) |
There was a problem hiding this comment.
If the model outputs items:[1}, this loop gets stuck on } and keeps appending empty strings. pls reject the unexpected closing brace here.
There was a problem hiding this comment.
Good catch, fixed in fa08659: a stray closing delimiter (} in an array, ] in an object, or an empty bare value) now stops parsing and keeps the arguments parsed before it, with tests for those cases in both streaming and non-streaming modes.
A stray "}" inside an array (or "]" inside an object, or an empty bare value) left the position unchanged and looped forever. Stop parsing at the malformed delimiter and keep the arguments parsed before it. Signed-off-by: saifltr <bagmarusaif@gmail.com>
|
Status update for reviewers: the review comment from @LinzeShi is addressed in fa08659 (a stray closing delimiter now stops parsing instead of looping, with tests), the issue reporter verified the fix against this branch in both streaming and non-streaming modes, and the parser test file passes 37/37 locally with ruff and mypy clean. |
chaunceyjiang
left a comment
chaunceyjiang
left a comment
There was a problem hiding this comment.
Could you paste some actual end-to-end test results into the PR description?
|
@chaunceyjiang added an end-to-end section to the description, details below. Setup. Raw model output for the Results
This branch, server start and client output:
Client output with the Client script and both server logs: e2e_client.py, server_fix.log, server_main.log. One note on the environment: this box has no nvcc, so the server was started with |
smiyamoto3
left a comment
smiyamoto3
left a comment
There was a problem hiding this comment.
Thanks for working on this. I found one streaming case that breaks the JSON: when <end_function_call> and the next <start_function_call> come in the same delta, the first call never gets its closing }, so the client ends up with {"city": "Paris".
Closing the previous call before starting the next one (L331-L337) fixes it for me:
if start_count > prev_start_count and start_count > end_count:
+ # The previous call may have ended in this same delta.
+ closing = None
+ if end_count > prev_end_count and self.current_tool_id >= 0:
+ calls = self.tool_call_regex.findall(current_text)
+ if self.current_tool_id < len(calls):
+ match = calls[self.current_tool_id]
+ if match[0]:
+ args = self._parse_arguments(match[1])
+ closing = self._stream_arguments(args, final=True)
self.current_tool_id += 1
self.current_tool_name_sent = False
self.streamed_args_for_tool.append("")
self.prev_tool_call_arr.append({})
logger.debug("Starting new tool call %d", self.current_tool_id)
- return None
+ return closing#64360 and #63947 overlap with this. With #63947's schema check in _parse_arguments, I think their <escape> value tests pass here too.
| if args: | ||
| args_json = json.dumps(args, ensure_ascii=False) | ||
| prev_streamed = self.streamed_args_for_tool[ | ||
| self.current_tool_id | ||
| ] | ||
| if len(args_json) > len(prev_streamed): | ||
| diff = args_json[len(prev_streamed) :] | ||
| self.streamed_args_for_tool[self.current_tool_id] = ( | ||
| args_json | ||
| ) | ||
| return DeltaMessage( | ||
| tool_calls=[ | ||
| DeltaToolCall( | ||
| index=self.current_tool_id, | ||
| function=DeltaFunctionCall( | ||
| arguments=diff | ||
| ).model_dump(exclude_none=True), | ||
| ) | ||
| ] | ||
| ) | ||
| return self._stream_arguments(args, final=True) |
There was a problem hiding this comment.
nit: call:f{} streams no arguments at all, so the client gets an empty string. #64360 sends {} here.
| if args: | |
| args_json = json.dumps(args, ensure_ascii=False) | |
| prev_streamed = self.streamed_args_for_tool[ | |
| self.current_tool_id | |
| ] | |
| if len(args_json) > len(prev_streamed): | |
| diff = args_json[len(prev_streamed) :] | |
| self.streamed_args_for_tool[self.current_tool_id] = ( | |
| args_json | |
| ) | |
| return DeltaMessage( | |
| tool_calls=[ | |
| DeltaToolCall( | |
| index=self.current_tool_id, | |
| function=DeltaFunctionCall( | |
| arguments=diff | |
| ).model_dump(exclude_none=True), | |
| ) | |
| ] | |
| ) | |
| return self._stream_arguments(args, final=True) | |
| # A call without arguments still streams "{}". | |
| if self.current_tool_name_sent: | |
| return self._stream_arguments(args, final=True) |
|
@smiyamoto3 thanks, the same-delta case is fixed in 8bb7cb0: the earlier call is now closed before the next one starts, with parser and server tests. I reproduced it on a live server, using stream_interval to merge the deltas. The lost-name case at higher intervals also exists on main, so I noted it in the description as a follow-up. The |
|
Thanks @smiyamoto3 — happy to be listed as co-author for the no-arg |
When a call's end tag shared a delta with the next call's start tag, the
start branch returned first. The earlier call then never got its closing
brace, or any arguments at all if they were still unsent. A call with no
arguments streamed nothing, leaving the client an empty string.
Close the current call in one place for both paths and send {} when
there are no arguments. Add server tests through the OpenAI client.
Replace a forbidden "import re" in the parser tests.
Co-authored-by: Si Chen <62344747+cs-fisha@users.noreply.github.com>
Signed-off-by: saifltr <bagmarusaif@gmail.com>
8bb7cb0 to
2bfc352
Compare
|
@smiyamoto3 thanks for the review. Added @cs-fisha as co-author on the latest commit (2bfc352); DCO is still green. @chaunceyjiang the end-to-end results are in the description, and every review comment is addressed. Could you take a look and add the |
chaunceyjiang
added
the
verified
chaunceyjiang
added
the
ready
|
✅ @saifltr, CI is now available for this PR.
|
|
/ci run |
|
✅ Triggered Buildkite CI #99044 for commit |
Overview
Fix the
functiongemmatool parser dropping every non-string argument, and make its streamed arguments reassemble into valid JSON. Fixes #63983.Claims
null, arrays and objects are parsed in streaming and non-streaming, nested containers included.{}instead of an empty string.Validation
eb703fbb3d)fa086599ec)The server tests pin the model output with a
structured_outputsregex, so they cover serving and parsing rather than what the model decides to call. pre-commit passes on the changed files. Existing Buildkite steps select both test files, but CI has not run on this PR yet.End-to-end (vllm serve, real model)
unsloth/functiongemma-270m-it(ungated mirror ofgoogle/functiongemma-270m-it, samechat_template.jinja) on one shared H200, through the OpenAI client withtool_choice="auto"andtemperature=0. Streamedtool_callsdeltas are concatenated per index, as a client would.Raw model output for the
set_alarmrequest (/v1/completions,skip_special_tokens=false):set_alarm(int, bool, array, string), non-streaming{"label": "Wake up"}{"days": ["mon", "tue"], "enabled": true, "hour": 7, "label": "Wake up"}set_alarm, streaming{"label": "Wake up"}{"days": ["mon", "tue"], "enabled": true, "hour": 7, "label": "Wake up"}get_weather(two strings), non-streaming{"city": "San Francisco", "unit": "celsius"}get_weather, streaming{"city": "San Francisco"} "unit": "celsius"}(invalid JSON){"city": "San Francisco", "unit": "celsius"}Review fixes, streaming, same server. The two-call rows pin the output with a regex and set
stream_intervalper request to merge deltas:fa086599ec)get_current_time(no parameters), real model output''{}stream_interval=4, delta is<end_function_call><start_function_call>call:{"city": "Paris"(invalid JSON){"city": "Paris"}stream_interval=8, first call's arguments are in that delta too''{"city": "Paris"}Every
stream_intervalfrom 1 to 6, and 8, gives both calls with valid JSON on this PR.Screenshots, client script and server logs from the first run (
fa086599ec): branche2e-evidence-63992.client
Details
The template's
format_argumentmacro wraps only strings in<escape>, e.g.enabled:true,hour:7,days:[<escape>mon<escape>],snooze:{minutes:5}. The oldarg_regexonly matchedkey:<escape>...<escape>, so bare values were dropped. A small recursive parser for that grammar replaces it. Withpartial=True(streaming) it stops at the first incomplete value, so each value is streamed once in its final form. A bare word that is not a JSON literal stays a string.Streaming diffs are computed by length against
json.dumps(args), so the closing brace is held back until the call ends. That left two gaps, both fixed here. When an end tag and the next start tag arrive together, the start branch returned before closing the earlier call, so it never got its brace, or any arguments if they were still unsent. A call with no arguments streamed nothing because the end branch skipped empty arguments. Both paths now go through_close_tool_call. Deltas can merge when the client falls behind the engine orstream_intervalis above 1.Known limitation: from
stream_interval=10in these runs, a call's name can share a delta with its end tag, and the name is lost. At 64 the second call is lost too. main behaves the same or worse here, so this is left for a follow-up.Related PRs:
<escape>values keep their current decoding here, so<escape>123<escape>is still the integer 123.AI assistance was used for this change.
Pull Request Checklist
I used vLLM's
/pr-checklistskill. (Mandatory for agents, optional for humans).AI assistance was used during the creation of this PR.
Design Fit: Minimizes impact on core components, reuses existing functionality, and justifies added complexity.
Testing and Validation: Validates the change and ensures any added tests are meaningful and reliable, with CI coverage or documented CI resource constraints and validation performed outside CI.
Code Quality and Style: Keeps code and comments clear and concise, and updates relevant documentation and examples.
Pull Request Contents: Includes a brief summary and relevant links, supports claims with evidence, explains root causes and implementation trade-offs, and follows the contributing guide.