[GPT-OSS] Fix Pydantic union resolution for ResponseFunctionToolCall in Responses API#24158
[GPT-OSS] Fix Pydantic union resolution for ResponseFunctionToolCall in Responses API#24158JasonZhu1313 wants to merge 3 commits intovllm-project:mainfrom
Conversation
There was a problem hiding this comment.
Code Review
This pull request correctly identifies and fixes a Pydantic union resolution issue for ResponseFunctionToolCall by introducing a pre-processing validator. The fix is well-described and the test plan is clear. My review includes a suggestion to improve the implementation of the new validator for better performance and robustness by addressing an inefficient import and overly broad exception handling.
vllm/entrypoints/openai/protocol.py
Outdated
There was a problem hiding this comment.
The implementation of function_call_parsing can be improved for efficiency and robustness.
- Inefficient Import: The
from openai.types.responses.response_function_tool_call import ResponseFunctionToolCallis inside a loop. SinceResponseFunctionToolCallis already imported at the top of the file (line 21), this local import is redundant and should be removed. - Broad Exception Handling: Using
except Exception:is too broad as it can catch and silence unexpected errors, making debugging harder. It's better to catch specific exceptions likepydantic.ValidationErrorandTypeErrorwhich are expected fromResponseFunctionToolCall(**item).
Here is a refactored version of the function that addresses these points and improves readability. Note that ValidationError will need to be imported from pydantic (e.g., by adding it to the import on line 36).
def function_call_parsing(cls, data):
"""Function call parsing to ensure ResponseFunctionToolCall objects are created."""
from pydantic import ValidationError
input_data = data.get("input")
if not isinstance(input_data, list):
return data
new_input = []
for item in input_data:
if isinstance(item, dict) and item.get("type") == "function_call":
try:
# ResponseFunctionToolCall is already imported at the top.
new_input.append(ResponseFunctionToolCall(**item))
except (ValidationError, TypeError):
# If conversion fails, keep the original dict.
new_input.append(item)
else:
new_input.append(item)
data["input"] = new_input
return data
DarkLight1337
left a comment
There was a problem hiding this comment.
Thanks for fixing, can you add a unit test to avoid regressions?
heheda12345
left a comment
There was a problem hiding this comment.
Thanks for the exploration. Can we fix it by changing the order of the Union?
ResponseInputOutputItem: TypeAlias = Union[ResponseInputItemParam,
ResponseFunctionToolCall
ResponseReasoningItem,
]
jobuser [ ~ ]$ python /home/jobuser/test_fix/test_changes.py Parsed result: I tried and it doesn't work. Seems explicit validation is needed |
Added a unit test, thanks for reminding |
Head branch was pushed to by a user without write access
b62ed35 to
0a0faac
Compare
|
This pull request has merge conflicts that must be resolved before it can be |
|
Can you revert unrelated changes? |
Signed-off-by: JasonZhu1313 <jasonchu13@outlook.com>
599b98a to
efaa967
Compare
Reverted, I was following the suggestions from DCO and got this. |
Signed-off-by: JasonZhu1313 <jasonchu13@outlook.com>
Fixed pre-commit and I think this might not relate to my PR correct me if I am wrong. |
aarnphm
left a comment
There was a problem hiding this comment.
cc @chaunceyjiang wrt #20874 compatibility.
should we just parse the dict into response tool instead? below
| def function_call_parsing(cls, data): | ||
| """Function call parsing to ensure ResponseFunctionToolCall objects | ||
| are created.""" | ||
| input_data = data.get("input") |
There was a problem hiding this comment.
I'm confused this seems to live under ChatCompletionRequest, but the PR seems to be only Responses related.
I haven't looked at the responses code yet, but do we use this object in the responses API as well?
There was a problem hiding this comment.
Last week tool calling was not supported so I have to use the response API which supports tool call
There was a problem hiding this comment.
I think this is a great improvement.
I also encountered this issue here: https://github.com/vllm-project/vllm/pull/20874/files#diff-31e6bd0df09a47b5587701203d558701ac46e4f85bf7db83632da9990eaef198R1546.
I wanted to use isinstance(input, ResponseFunctionToolCall) for the check, but since input is a dict, isinstance cannot be used — I had to resort to input.get().
However, I’m confused: why is function_call_parsing located under ChatCompletionRequest rather than ResponsesRequest? Shouldn’t it belong to ChatCompletionRequest?
There was a problem hiding this comment.
@JasonZhu1313 could your full client code. this change doesn't seem to impact. why do we depend on ChatCompletionRequest?
| '{"query": "weather", "filters": ["temperature", "humidity"], "count": 5}', | ||
| '{"complex": {"nested": {"data": true}}, "array": [1, 2, 3]}' | ||
| ]) | ||
| def test_function_call_with_complex_arguments(arguments): |
There was a problem hiding this comment.
Refer to https://platform.openai.com/docs/guides/function-calling?api-mode=responses
FunctionCallOutput will also be used as input. Could you also add a test for FunctionCallOutput?
|
Also CC @yeqcharlotte |
| def function_call_parsing(cls, data): | ||
| """Function call parsing to ensure ResponseFunctionToolCall objects | ||
| are created.""" | ||
| input_data = data.get("input") |
There was a problem hiding this comment.
@JasonZhu1313 could your full client code. this change doesn't seem to impact. why do we depend on ChatCompletionRequest?
|
Also hit this and would love a fix for it. @JasonZhu1313 are you still looking into this? |
|
I reached out to @JasonZhu1313 who said he did not have to finish the PR. Opened a new one and addressed the comments from this PR - #26706. Would appreciate a review on that to unblock function calling using the responses API |
|
This pull request has been automatically marked as stale because it has not had any activity within 90 days. It will be automatically closed if no further activity occurs within 30 days. Leave a comment if you feel this pull request should remain open. Thank you! |
|
This pull request has been automatically closed due to inactivity. Please feel free to reopen if you intend to continue working on it. Thank you! |
Purpose
When using GPT OSS with tool calling, the vLLM API requires the messages input to be a list containing a mix of message types:
ResponseFunctionToolCallandResponseReasoningItem, which represent tool call content and reasoning content.These objects are serialized into JSON for the HTTP request and then deserialized into pydantic objects on the server side as the request hits FastAPI. During this serialization/deserialization cycle, I observed that
ResponseFunctionToolCallis being dropped down to a plain dictionary, losing its type information in the request payload.This causes downstream errors because:
The code later checks for messages specifically of type ResponseFunctionToolCall. code pointer
Tool call IDs must be tracked across messages (tool output IDs must match their corresponding tool call IDs).
If the type is lost, ID matching fails, leading to an exception being raised: raise ValueError(f"No call message found for {call_id}")
Issues
Input messages prepared for sending to server with the correct type :
As soon as it reaches to the server, the logging of ResponsesRequest shows the type information is dropped for ResponseFunctionToolCall, and it becomes a regular dict:
Which will cause the downstream code fail!!
Root Cause
The issue occurs during Pydantic union type resolution in
ResponseInputOutputItem. When validating function call data, Pydantic matchesResponseFunctionToolCallParam(a TypedDict within the broadResponseInputItemParamunion) before reachingResponseFunctionToolCall(the specific Pydantic model).The union type resolution order:
ResponseInputItemParam(containsResponseFunctionToolCallParamTypedDict) ✅ Matches firstResponseReasoningItemResponseFunctionToolCall❌ Never reachedSince TypedDict validation is more permissive than Pydantic model validation, function calls are parsed as plain dictionaries instead of
ResponseFunctionToolCallobjects.Solution
Add a pre-processing validator to
ResponsesRequestthat explicitly converts function call dictionaries toResponseFunctionToolCallobjects before Pydantic's union resolution.Test Plan
A simple script to reproduce the issue:
Test Result
Same code works after the fix, log shows the type is preserved:
Final result:
Fun test script before the fix:
Fun test script after the fix:
Essential Elements of an Effective PR Description Checklist
supported_models.mdandexamplesfor a new model.