[Bugfix]: Fix the logic for deciding if tool parsing is used

[tomeras91]

The great work done in #5649 has a small bug with regards to deciding if tool parsing is enabled or not. This PR fixes said bug

I'm pretty sure the logic in main is a bug since I compared the logic used in

vllm/vllm/entrypoints/openai/serving_chat.py

Line 165 in cea95df

self.enable_auto_tools and self.tool_parser is not None):

to the one I fixed in

vllm/vllm/entrypoints/openai/serving_chat.py

Line 610 in cea95df

if not (self.enable_auto_tools

Both are checking the same thing - whether or not tool parsing is used. Yet, they have different truth values for the same truth values of self.enable_auto_tools and self.tool_parser. What I did is to align the logic in line 610 to the one in line 165. It also makes sense given the comment in line 608

[github-actions]

👋 Hi! Thank you for contributing to the vLLM project.
Just a reminder: PRs would not trigger full CI run by default. Instead, it would only run fastcheck CI which starts running only a small and essential subset of CI tests to quickly catch errors. You can run other CI tests on top of those by going to your fastcheck build on Buildkite UI (linked in the PR checks section) and unblock them. If you do not have permission to unblock, ping simon-mo or khluu to add you in our Buildkite org.

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 do one of these:

• Add ready label to the PR
• Enable auto-merge.

🚀

[tomeras91]

@mgoin - tagging you as I understand you were coordinating #5649

[DarkLight1337]

cc @K-Mistele

[K-Mistele]

So the intent here is, "if auto tool parsing is not enabled, and request.tool_choice is not a named tool choice parameter for guided decoding"

wouldn't this be, if (not (self.enable_auto_tools and self.tool_parser)) and not isinstance(request.tool_choice, ChatCompletionNamedToolChoiceParam): ?

This might be a little redundant in terms of the parentheses, wanted to avoid operator precendence issues.

alternatively, if (not self.enable_auto_tools or not self.tool_parser) and not isinstance(...)

[K-Mistele]

the goal is to make sure that either/both of self.enable_auto_tools and self.tool_parser are false.

[tomeras91]

Either one of your options is great IMO

The original logic was:
if not (self.enable_auto_tools or not self.tool_parser) and not isinstance(...).
Comparing to the second alternative you suggested:
if (not self.enable_auto_tools or not self.tool_parser) and not isinstance(...)
shows that the issue is that the first not is outside of the parentheses instead of inside them.

Unless you have any objections, I'll go with your second suggestion since it does have less parentheses and it's a really small change - just the location of the opening (

(Just to explain my line of thought - the reason I went with
not (self.enable_auto_tools and not self.tool_parser) and not isinstance(...)
is that is it similar to line 165:
not (self.enable_auto_tools and self.tool_parser is not None)
I now understand this is also a bug since the condition will be True if auto tools is enabled, which is not what we wanted.)

[K-Mistele]

LGTM :)

[mgoin]

Thanks!