[Tool Parser][2/3] Use self.tools instead of request.tools in tool parsers#38189
[Tool Parser][2/3] Use self.tools instead of request.tools in tool parsers#38189chaunceyjiang merged 1 commit intovllm-project:mainfrom
Conversation
mergify
bot
added
deepseek
![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 refactors the tool parsing logic by centralizing the tools definition within the AbstractToolParser instance, making it accessible via self.tools instead of passing it as an argument to individual methods. This change simplifies method signatures and streamlines tool management across various tool parsers and their tests. A potential issue was identified in vllm/tool_parsers/abstract_tool_parser.py where the __init__ method's filtering logic for tools might incorrectly exclude valid FunctionTool definitions if they are provided as plain dictionaries, due to the behavior of isinstance with TypedDict.
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
bbrowning
left a comment
bbrowning
left a comment
There was a problem hiding this comment.
This looks straightforward and a reasonable follow-up to the 1st PR in this series. One note is the _WrappedParser in vllm/parser/abstract_parser.py doesn't pass in tools at all here. I think that's ok for now based on where that gets used in the Responses API path, but we may want to confirm whether that needs some changes as we get to step 3.
Thanks Ben, this is great reminder to update the unified parser. On a side note - step 3 is blocked util deprecation period is reached. |
|
This pull request has merge conflicts that must be resolved before it can be |
sfeng33
force-pushed
the
cleanup-tool-p2
branch
from
3f99df6 to
c20983c
Compare
|
Resolved conflict |
|
@chaunceyjiang would you have any additional comments or feedback on this PR? Thanks! |
chaunceyjiang
added
the
ready
|
This pull request has merge conflicts that must be resolved before it can be |
|
Please rebase the PR |
sfeng33
force-pushed
the
cleanup-tool-p2
branch
from
c20983c to
e19de24
Compare
|
@chaunceyjiang all tests have passed. |
…rsers (vllm-project#38189) Signed-off-by: sfeng33 <4florafeng@gmail.com> Signed-off-by: neweyes <328719365@qq.com>
…llm-project#38189) After the refactor in vllm-project#38189 to use self.tools instead of request.tools, anyOf regression tests need to provide tools at parser construction time so the schema is available for type resolution. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com> Signed-off-by: Ben Browning <bbrownin@redhat.com>
…rsers (vllm-project#38189) Signed-off-by: sfeng33 <4florafeng@gmail.com> Signed-off-by: EricccYang <yangyang4991@gmail.com>
…rsers (vllm-project#38189) Signed-off-by: sfeng33 <4florafeng@gmail.com> Signed-off-by: bhargav-patel-29 <bhargav.patel@tihiitb.org>
…rsers (vllm-project#38189) Signed-off-by: sfeng33 <4florafeng@gmail.com>
3 participants
Purpose
self.tools(set at construction) instead ofrequest.toolsat call time. This is part 2 of the tool parser cleanup series following [Tool Parser][1/3] Pass tools to ToolParser constructor #38029.ToolParser.__init__to filter and store onlyChatCompletionToolsParam | FunctionToolinstancesTest Plan