-
Notifications
You must be signed in to change notification settings - Fork 860
[MCP] Handle Ollama's deviation from the OpenAI tool streaming spec #3140
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
Conversation
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.
Pull Request Overview
This PR fixes an error when handling tool call argument concatenations for Ollama's streaming API by ensuring that a None value is replaced with an empty string before concatenation.
- Introduces a safety mechanism to handle None argument values in tool calls.
- Ensures subsequent JSON payload chunks are concatenated without errors.
| if final_tool_calls[idx].function.arguments is None: | ||
| final_tool_calls[idx].function.arguments = "" | ||
| continue | ||
| # safety before concatenating text to .function.arguments | ||
| if final_tool_calls[idx].function.arguments is None: | ||
| final_tool_calls[idx].function.arguments = "" |
Copilot
AI
Jun 4, 2025
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.
[nitpick] The check and assignment for function.arguments being None is repeated; consider refactoring this logic into a helper function to reduce duplication and improve maintainability.
| if final_tool_calls[idx].function.arguments is None: | |
| final_tool_calls[idx].function.arguments = "" | |
| continue | |
| # safety before concatenating text to .function.arguments | |
| if final_tool_calls[idx].function.arguments is None: | |
| final_tool_calls[idx].function.arguments = "" | |
| self._ensure_arguments_initialized(final_tool_calls[idx].function) | |
| continue | |
| # safety before concatenating text to .function.arguments | |
| self._ensure_arguments_initialized(final_tool_calls[idx].function) |
|
The docs for this PR live here. All of your documentation changes will be reflected on that endpoint. The docs are available until 30 days after the last update. |
|
what's the status on this PR? |
|
@Wauplin ready to review and merged! not sure about the impact though |
Wauplin
left a comment
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.
it looks fine when comparing to huggingface/huggingface.js#1512 but honestly had only a superficial look at it 🙈
python equivalent of huggingface/huggingface.js#1512.
Even though Ollama advertises an OpenAI-compatible streaming API, it sends a json payload in
delta.tool_calls[*].function.argumentsin the first chunk, not an empty string as in the OpenAI spec. when the second chunk arrives with the real payload, we try to do None += "", which obviously raises an Error. This PR should fix that.