-
-
Notifications
You must be signed in to change notification settings - Fork 11.7k
[WIP][DEBUG] Tool converter #28372
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
[WIP][DEBUG] Tool converter #28372
Conversation
Signed-off-by: Andrew Xia <axia@fb.com>
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".
| def convert_tool_responses_to_completions_format(tool: dict) -> dict: | ||
| """ | ||
| Convert a flat tool schema: | ||
| {"type": "function", "name": "...", "description": "...", "parameters": {...}} | ||
| into: | ||
| {"type": "function", "function": {...}} | ||
| """ | ||
| return { | ||
| "type": "function", | ||
| "function": tool, |
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.
Preserve non-function tool types when converting schema
The new convert_tool_responses_to_completions_format wraps every tool dict as {"type": "function", "function": tool} regardless of the original tool["type"]. Responses requests may contain non-function tools such as code_interpreter, file_search, or MCP tools, and _make_request now runs this conversion over them before passing tool_dicts into _preprocess_chat. After this change those tools are reported to the model as type function, so built‑in tools can no longer be enabled or invoked correctly. The conversion should only wrap tools whose type is function and should not rewrite other tool types.
Useful? React with 👍 / 👎.
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.
Code Review
This pull request refactors tool-related utility functions and introduces a converter to adapt tool definitions to the format expected by the completions API. The changes include moving extract_tool_types to a shared utility file and adding convert_tool_responses_to_completions_format. My review has identified a critical issue where an exception handler has been commented out, likely for debugging, which poses a risk to server stability. Additionally, there's a high-severity bug in the new tool converter function that incorrectly handles non-function tool types. These issues should be addressed before merging.
| # try: | ||
| generator = await handler.create_responses(request, raw_request) | ||
| # except Exception as e: | ||
| # raise HTTPException( | ||
| # status_code=HTTPStatus.INTERNAL_SERVER_ERROR.value, detail=str(e) | ||
| # ) from e |
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.
This try...except block, which handles generic exceptions during response creation, has been commented out. While this can be helpful for debugging by allowing exceptions to propagate, it removes a critical error handling layer for the API endpoint. Without this, unexpected errors in handler.create_responses could lead to unhandled exceptions, potentially crashing the server or leaking internal error details to clients. This change should be reverted before merging into a main branch to ensure the server's stability and robustness.
| # try: | |
| generator = await handler.create_responses(request, raw_request) | |
| # except Exception as e: | |
| # raise HTTPException( | |
| # status_code=HTTPStatus.INTERNAL_SERVER_ERROR.value, detail=str(e) | |
| # ) from e | |
| try: | |
| generator = await handler.create_responses(request, raw_request) | |
| except Exception as e: | |
| raise HTTPException( | |
| status_code=HTTPStatus.INTERNAL_SERVER_ERROR.value, detail=str(e) | |
| ) from e |
| def convert_tool_responses_to_completions_format(tool: dict) -> dict: | ||
| """ | ||
| Convert a flat tool schema: | ||
| {"type": "function", "name": "...", "description": "...", "parameters": {...}} | ||
| into: | ||
| {"type": "function", "function": {...}} | ||
| """ | ||
| return { | ||
| "type": "function", | ||
| "function": tool, | ||
| } |
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.
The convert_tool_responses_to_completions_format function incorrectly assumes all tools are of type 'function' by hardcoding "type": "function" in the returned dictionary. This will cause issues for other tool types, such as 'mcp', by wrapping them in an incorrect 'function' tool structure, which will likely cause downstream processing errors. The function should be updated to handle only 'function' type tools and return other tool types unmodified.
| def convert_tool_responses_to_completions_format(tool: dict) -> dict: | |
| """ | |
| Convert a flat tool schema: | |
| {"type": "function", "name": "...", "description": "...", "parameters": {...}} | |
| into: | |
| {"type": "function", "function": {...}} | |
| """ | |
| return { | |
| "type": "function", | |
| "function": tool, | |
| } | |
| def convert_tool_responses_to_completions_format(tool: dict) -> dict: | |
| """ | |
| Convert a flat tool schema: | |
| {"type": "function", "name": "...", "description": "...", "parameters": {...}} | |
| into: | |
| {"type": "function", "function": {...}} | |
| """ | |
| if tool.get("type") == "function": | |
| return { | |
| "type": "function", | |
| "function": tool, | |
| } | |
| return tool |
Signed-off-by: chaunceyjiang <chaunceyjiang@gmail.com>
Purpose
Test Plan
Test Result
Essential Elements of an Effective PR Description Checklist
supported_models.mdandexamplesfor a new model.