-
-
Notifications
You must be signed in to change notification settings - Fork 8.5k
[Frontend] Expand tools even if tool_choice="none" #17177
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
[Frontend] Expand tools even if tool_choice="none" #17177
Conversation
👋 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 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 🚀 |
d0de334
to
d272dbe
Compare
ef2bf5f
to
25aec8d
Compare
Hi @aarnphm, I've fixed the CI issues and updated this PR. The tests are now passing - could you please take another look when you have a chance? Thanks! |
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.
Do you have an use-case that requires the tool definition even if tool_choice="none"
?
From a high level POV, having tool definition when I don't want to use tool seems like a waste of context for small/medium size model
Is the use case that you'd like to allow tool calls to be (potentially) generated, so you want all of the tool call definitions included, but you don't want the API server trying to parse any of it if they are there? I'm just trying to make sure I understand properly.. |
I'm not a big fan of the new option, but honestly, the new behavior makes more sense to me. If tools have been defined in a request, it seems like including that data makes sense. Whether it's included or not seems implied by whether they're included in the request. |
Currently we are achieving tool_choice="auto" behavior by sending two requests from reverse proxy server to vLLM's API server:
If the first response was an empty json list, we assume the tool use was unnecessary and just use the second response. This is required because we want to use guided decoding (instead of relying on tool_parser) to enforce that the function call strictly aligns with the expected signature. |
fyi But I think I understand this case. |
Honestly, I agree with @russellb . Instead of adding a new option, could we just change the default behavior to be more consistent by always including tool definitions regardless of tool_choice setting?
Yes. |
I found the discussion about why tools are dropped when tool_choice="none": #10000 (comment) |
The newer There's an example in here: vllm/examples/online_serving/structured_outputs/structured_outputs.py Lines 140 to 199 in 154d063
You can see in the example that the tool definition had to be manually included in the prompt. What you're proposing here, that it can (and should) be automatic, makes sense to me. I think changing the default behavior, plus using the |
Yes. I will modify this PR to remove the new option and change the default behaviour. I will check the |
I modified. Could you take another look? |
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.
I like this change, though I'll hold off on merging to give others more time to express an opinion
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.
I afraid that this is breaking, given that requests relying on the prior behaviour would start breaking if the set of prompts + user message exceed the max context length of the model (unless long context like YaRN are being used).
Intuitively, tool_choice="none"
implies that this request won't include any tool definitions. The current behaviour also aligns with OpenAI's logic for tool_choices="none"
(see here)
none
means the model will not call any tool and instead generates a message.
This doesn't mean that we have to strictly follow what OpenAI does. Just that if the goal is to be compatible, then I don't think this behaviour makes sense.
What about changing the default as proposed, but adding |
opt-out is breaking right? For any production deployment. I think we should make this opt-in, then follow deprecating policy fwiw then break it in 0.10 at least. @simon-mo do we have plans to release any patches before 0.10? I'm fine with making this the default in 0.10, given that 0.10 will be considered breaking regardless. |
@aarnphm You're right about the breaking change concern. Let me revert commit 377f4ac to restore the opt-in approach with the Regarding the transition to 0.10 - could you help me understand the process for: |
Signed-off-by: okada shintarou <[email protected]>
Signed-off-by: okada shintarou <[email protected]>
Signed-off-by: okada shintarou <[email protected]>
Signed-off-by: okada shintarou <[email protected]>
This reverts commit 377f4ac. Signed-off-by: okada shintarou <[email protected]>
Signed-off-by: okada shintarou <[email protected]>
Signed-off-by: okada shintarou <[email protected]>
Signed-off-by: okada shintarou <[email protected]>
9559897
to
e282ebb
Compare
Signed-off-by: okada shintarou <[email protected]>
Signed-off-by: okada shintarou <[email protected]>
tool_dicts = None | ||
elif (request.tool_choice == "none" | ||
and not self.expand_tools_even_if_tool_choice_none): | ||
assert request.tools is not None |
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.
Let's not use assert in performance path here, if this is mostly for types the we can gate it in TYPE_CHECKING
.
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.
@aarnphm Thanks for the review! I understand the performance concern, but I'd like to keep the assert here for a specific reason.
This assert serves as a defensive programming guard rather than just type checking. The logic is:
- First condition:
if request.tools is None
→tool_dicts = None
- Second condition:
elif (request.tool_choice == "none" and not self.expand_tools_even_if_tool_choice_none)
The assert ensures that if someone modifies the first condition in the future (e.g., adds another OR condition), we'll catch the logic error immediately with a clear AssertionError, rather than getting a confusing AttributeError: 'NoneType' object has no attribute '__len__'
when we call len(request.tools)
below.
While I understand the performance concern, in the context of vLLM's request processing pipeline, this single assertion check is dwarfed by the actual bottlenecks like model inference, GPU operations, and network I/O. The cost of one conditional check per request is negligible compared to the milliseconds/seconds spent on actual LLM processing.
Given that trade-off, I think the defensive programming benefit outweighs the minimal performance cost. What do you think?
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.
@aarnphm You're absolutely right. Looking at this again, adding TYPE_CHECKING import just for this assert would be overkill, and the assert itself isn't really necessary here. Let me remove it and keep the code simple. Thanks for the guidance!
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.
I'm good with this, can you look at the PR failure? seems like it is relevant to this PR.
Co-authored-by: Aaron Pham <[email protected]> Signed-off-by: okada shintarou <[email protected]>
8750191
to
90eabb2
Compare
Signed-off-by: okada shintarou <[email protected]>
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.
Ok, stamp from me, waiting for @russellb
Hi @russellb, Could you please take a look at the latest changes? The PR has been updated based on your previous suggestions about following the deprecation process. I believe this is now ready for your review. |
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.
apologies for the delay. looks good, thank you!
Signed-off-by: okada shintarou <[email protected]>
Add option to include tool definitions even when tool_choice is 'none'
Summary
This PR adds a new command-line option
--expand-tools-even-if-tool-choice-none
which allows including tool definitions in prompts even whentool_choice='none'
.Motivation
In the current implementation, when
tool_choice
is set to'none'
, all tool definitions are removed from the request, preventing the model from seeing the tool schemas. This change enables a workflow where:tool_choice='none'
)This is useful for:
Implementation
--expand-tools-even-if-tool-choice-none
(default: False)protocol.py
to no longer remove tools whentool_choice='none'
OpenAIServingChat
and passed it through from the API server