[Tool Parser][1/3] Pass tools to ToolParser constructor#38029
[Tool Parser][1/3] Pass tools to ToolParser constructor#38029robertgshaw2-redhat merged 4 commits intovllm-project:mainfrom
Conversation
Signed-off-by: sfeng33 <4florafeng@gmail.com>
|
Hi @sfeng33, the pre-commit checks have failed. Please run: uv pip install pre-commit>=4.5.1
pre-commit install
pre-commit run --all-filesThen, commit the changes and push to your branch. For future commits, Tip Is
|
Signed-off-by: sfeng33 <4florafeng@gmail.com>
|
Warning Gemini encountered an error creating the review. You can try again by commenting |
|
/gemini review |
There was a problem hiding this comment.
Code Review
This pull request refactors the ToolParser initialization by introducing a tools argument to the ToolParser constructor and its subclasses. This change allows the tool parsing logic to be aware of the specific tools provided in the request, updating constructor calls and type hints across various tool parser implementations and related serving files. The review suggests an improvement to enhance encapsulation by initializing the StreamingXMLToolCallParser with the tools argument directly in its constructor for better configuration and future simplification.
|
Hi @sfeng33, the pre-commit checks have failed. Please run: uv pip install pre-commit>=4.5.1
pre-commit install
pre-commit run --all-filesThen, commit the changes and push to your branch. For future commits, Tip Is
|
Signed-off-by: sfeng33 <4florafeng@gmail.com>
|
Hi @sfeng33, the pre-commit checks have failed. Please run: uv pip install pre-commit>=4.5.1
pre-commit install
pre-commit run --all-filesThen, commit the changes and push to your branch. For future commits, Tip Is
|
bbrowning
left a comment
There was a problem hiding this comment.
It looks like we got all the parsers updated. All unit tests pass for me locally, and I also ran integration tests under tests/entrypoints/openai/tool_parsers and tests/tool_use and all of those that I could run on my hardware passed. The ones I couldn't run are just related to my resources available, and not to any change here.
So, this looks good to me! It sets us up for the next refactoring but as far as I can tell is completely transparent to the end users.
As you move on to phase 2 of this, another thing I realized to keep in mind is we have some external tool parser plugins. This new parameter is optional in the constructor, so that part should be fine. We'll just need to be mindful about whether we want to break or keep semantics working for external tool parsers as all this gets wired through. Until or unless those external tool parser plugins get updated, we'll have to always handle the case where our list of tools is None defensively.
Thanks for the thorough review and integration testing, Ben! Great point about external tool parser plugins, I'll make sure to handle tools=None defensively in PR 2 so we don't break any out-of-tree parsers. |
…#38029) Signed-off-by: sfeng33 <4florafeng@gmail.com>
…#38029) Signed-off-by: sfeng33 <4florafeng@gmail.com>
…#38029) Signed-off-by: sfeng33 <4florafeng@gmail.com> Signed-off-by: Michel Belleau <michel.belleau@malaiwah.com>
…#38029) Signed-off-by: sfeng33 <4florafeng@gmail.com> Signed-off-by: Monishver Chandrasekaran <monishverchandrasekaran@gmail.com>
…#38029) Signed-off-by: sfeng33 <4florafeng@gmail.com> Signed-off-by: Nithin Chalapathi <nithin.ch10@gmail.com>
…#38029) Signed-off-by: sfeng33 <4florafeng@gmail.com>
…#38029) Signed-off-by: sfeng33 <4florafeng@gmail.com>
…#38029) Signed-off-by: sfeng33 <4florafeng@gmail.com> Signed-off-by: Vinay Damodaran <vrdn@hey.com>
…#38029) Signed-off-by: sfeng33 <4florafeng@gmail.com> Signed-off-by: EricccYang <yangyang4991@gmail.com>
…#38029) Signed-off-by: sfeng33 <4florafeng@gmail.com> Signed-off-by: bhargav-patel-29 <bhargav.patel@tihiitb.org>
The ToolParser base class now accepts (tokenizer, tools) from PR vllm-project#38029, but Gemma4ToolParser only accepted (tokenizer). This caused a 400 error when tool calls were attempted. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
The ToolParser base class now accepts (tokenizer, tools) from PR vllm-project#38029, but Gemma4ToolParser only accepted (tokenizer). This caused a 400 error when tool calls were attempted. Signed-off-by: Michael Hospedales <hospedales@me.com>
…#38029) Signed-off-by: sfeng33 <4florafeng@gmail.com>
Purpose
This is the first PR toward decoupling tool parsers from the API request object, which is necessary to reuse tool parsing logic in the unified output parser (#32713).
Today, tool parsers receive tool definitions only at extraction time via the request parameter in extract_tool_calls() /
extract_tool_calls_streaming(). This tight coupling to the request object prevents the parsers from being composed into a unified parser that handles reasoning, tool calls, and text output together — the unified parser shouldn't need to know about API request internals.
This is PR 1 of 3:
No behavioral change in this PR — purely additive plumbing.
Test Plan