Repository navigation
fix: Handle tools without parameters in OpenAI frontend tool_choice - #9056
Open
paolo-perrone wants to merge 2 commits into
Open
paolo-perrone wants to merge 2 commits into
paolo-perrone wants to merge 2 commits into
Conversation
[Medium impact] The PR appears safe to merge; the previous finding is fixed and no new actionable issues remain. SummaryFixes forced tool calls when a function omits
Reviews (2) · Last reviewed commit: "fix: Forbid extra keys in the no-paramet..." · Reviewed by Greptile |
Contributor
|
LGTM. Please respond to the review comment. |
This branch has not been deployed
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
What does the PR do?
If a chat request forces a tool that has no "parameters" field, the OpenAI frontend returns a 500. OpenAI lets you leave "parameters" out for a function that takes no arguments, and our schema allows it too (
FunctionObject.parametersis Optional), but_get_guided_json_from_toolcallstool.parameters.model_dump_json()without checking for None. Both the vLLM and the TRT-LLM request converters call it, so this hits every backend.Repro:
tools=[{"type": "function", "function": {"name": "get_current_time"}}]withtool_choice={"type": "function", "function": {"name": "get_current_time"}}(ortool_choice="required"). The response is a 500 with'NoneType' object has no attribute 'model_dump_json'.The fix returns an empty object schema,
{"type": "object", "properties": {}}, when parameters is missing, so the forced call comes back with{}as its arguments. That's the same thing vLLM does for "required" when a tool has no parameters.Checklist
<commit_type>: <Title>Commit Type:
Where should the reviewer start?
python/openai/openai_frontend/engine/utils/triton.py,_get_guided_json_from_tool.Test plan:
Added
test_guided_json_for_tool_without_parameterstotests/test_tool_calling.py. It covers a named tool_choice and "required" with a tool that has no parameters. It fails on main with the AttributeError and passes with the fix. Tools that do have parameters get exactly the same schema as before.Caveats:
Another option is to return None and skip guided decoding for these tools, which is what vLLM does for a named tool_choice. I went with the empty object schema so the arguments always come back as valid JSON. Happy to switch if you'd rather.
pre-commit's flake8 flags three existing lines in the same file (
!= Truetwice and an unusedexcept ... as e), which this PR doesn't touch. I left them alone to keep this to one change.