-
-
Notifications
You must be signed in to change notification settings - Fork 11.8k
[responsesAPI][7] Browser, Container MCP tools for non harmony models #29989
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
base: main
Are you sure you want to change the base?
Conversation
|
Documentation preview: https://vllm--29989.org.readthedocs.build/en/29989/ |
92bb4e6 to
67211f7
Compare
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.
💡 Codex Review
Here are some automated review suggestions for this pull request.
ℹ️ About Codex in GitHub
Codex has been enabled to automatically review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".
| message = ResponseFunctionToolCallOutputItem( | ||
| id=f"fco_{random_uuid()}", | ||
| type="function_call_output", | ||
| call_id=f"call_{random_uuid()}", | ||
| output=result_str, |
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.
Reuse tool call_id when emitting tool output
In ParsableContext.call_python_tool the tool output is created with a new random call_id instead of the last_msg.call_id from the assistant tool call, and the same pattern is used in the search and container helpers. The next turn builds a tool message whose tool_call_id comes from this new ID, so it no longer matches the assistant tool call that triggered it, producing an invalid transcript and preventing the model from associating the tool response with its request.
Useful? React with 👍 / 👎.
67211f7 to
9fed1be
Compare
Signed-off-by: Andrew Xia <axia@meta.com>
Signed-off-by: Andrew Xia <axia@meta.com> Signed-off-by: Andrew Xia <axia@fb.com>
9fed1be to
a99ecde
Compare
| ): | ||
| return True | ||
| if last_message.type == "function_call": # noqa: SIM102 | ||
| if last_message.name in ( |
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 think right now we are ad-hoc but it's fine, because we only support 3 MCP tools. When we support generic MCP tools (for both GPTOSS and non GPTOSS) we could have better logic here
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.
could you leave a TODO here assigned to yourself
Signed-off-by: Andrew Xia <axia@fb.com>
Head branch was pushed to by a user without write access
Purpose
Test Plan
minimax server
gptoss server
container tool
browser tool