Gigachat 3 tool parser and tests #29905
Conversation
Signed-off-by: Viacheslav Barinov <viacheslav.teh@gmail.com>
ajpqs requested review from DarkLight1337, NickLucche, aarnphm, chaunceyjiang and robertgshaw2-redhat as code owners 
| 👋 Hi! Thank you for contributing to the vLLM project. 💬 Join our developer Slack at https://slack.vllm.ai to discuss your PR in #pr-reviews, coordinate on features in #feat- channels, or join special interest groups in #sig- channels. Just a reminder: PRs would not trigger full CI run by default. Instead, it would only run You ask your reviewers to trigger select CI tests on top of Once the PR is approved and ready to go, your PR reviewer(s) can run CI to test the changes comprehensively before merging. To run CI, PR reviewers can either: Add If you have any questions, please reach out to us on Slack at https://slack.vllm.ai. 🚀 |
| Documentation preview: https://vllm--29905.org.readthedocs.build/en/29905/ |
mergify bot added documentation 
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request introduces a new tool parser for Gigachat 3 models, along with corresponding documentation and tests. The implementation for non-streaming tool calls is solid. However, the streaming implementation contains a critical bug in its JSON parsing logic that can lead to state corruption and cause streaming to fail for tool calls with complex arguments. I've provided a detailed comment on this issue. The rest of the changes, including the tests and documentation, look good.
| if cur_args.endswith("}"): # last '}' end of json | ||
| try: | ||
| candidate = cur_args[:-1].strip() | ||
| json.loads(candidate) | ||
| cur_args = candidate | ||
| except json.JSONDecodeError: | ||
| pass |
There was a problem hiding this comment.
The current logic for parsing streaming tool call arguments is brittle and has a state corruption bug that can cause streaming to fail.
- Fragile JSON Detection: The logic attempts to detect a complete JSON object by checking if the string ends with
}and then trying to parse the string without this last character. This is not robust and can fail if a string value within the JSON contains a}near the end (e.g.,{"key": "value}"}). Theexcept passthen silently ignores parsing failures. - State Corruption: More critically, when the parse is successful,
cur_argsis reassigned to the truncatedcandidatestring. This modifiedcur_argsis then used to update the streaming state. This corrupts the state becauseprev_argsin the next iteration will not be a prefix of the raw cumulative stream content, causing thecur_args.startswith(prev_args)check to fail and prematurely stop argument streaming.
I'm suggesting removing this block to prevent state corruption. This will ensure streaming does not break, but it may result in the final concatenated argument string having an extra trailing }. This should be addressed, ideally with a more robust streaming JSON parsing approach (e.g., brace-counting).
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
ℹ️ About Codex in GitHub
Codex has been enabled to automatically review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".
| clean_buffer = self.content_buffer.lstrip() | ||
| is_prefix = self.trigger_start.startswith(clean_buffer) | ||
| starts_with_trigger = clean_buffer.startswith(self.trigger_start) | ||
| if is_prefix or starts_with_trigger: | ||
| return None |
There was a problem hiding this comment.
The streaming parser only treats a prefix matching trigger_start (function call{) as the start of a tool call, and otherwise flushes buffered text as normal content (lines 124-128). When the model emits the documented function call<|role_sep|>\n{…} prefix (which the non-streaming regex explicitly supports), these deltas do not match trigger_start, so the parser streams function call<|role_sep|> to the client as plain text before the tool call is detected. That produces stray text and breaks tool-call semantics for the common role_sep variant of GigaChat3 outputs during streaming.
Useful? React with 👍 / 👎.
1 participant
Purpose
This PR adds a tool parser for parsing the function calls made by the Gigachat 3 models.
Models can generate only one function call without any text using format:
Test Plan
This PR also includes tests. Existing tests have not been modified or will be impacted.
Test Result
Output:
Essential Elements of an Effective PR Description Checklist
supported_models.mdandexamplesfor a new model.