Python: Fix OllamaChatClient passing unsupported kwargs to ollama.AsyncClient…#4403
Python: Fix OllamaChatClient passing unsupported kwargs to ollama.AsyncClient…#4403max-montes wants to merge 1 commit intomicrosoft:mainfrom
Conversation
There was a problem hiding this comment.
Pull request overview
Fixes the Ollama Python adapter to avoid forwarding orchestration/middleware-injected **kwargs into the Ollama SDK call (ollama.AsyncClient.chat()), preventing unexpected-keyword TypeErrors during workflows like handoffs.
Changes:
- Stop passing
_inner_get_response()**kwargsthrough toollama.AsyncClient.chat()(streaming and non-streaming paths). - Add regression tests ensuring unsupported kwargs (e.g.,
allow_multiple_tool_calls) are not forwarded in either mode.
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 4 comments.
| File | Description |
|---|---|
| python/packages/ollama/agent_framework_ollama/_chat_client.py | Removes forwarding of arbitrary **kwargs into AsyncClient.chat(), aligning behavior with other provider adapters. |
| python/packages/ollama/tests/test_ollama_chat_client.py | Adds regression coverage verifying unsupported kwargs are ignored for both streaming and non-streaming calls. |
|
This looks like a duplicate of #4404. Please confirm and we can close this. |
….chat() Stop forwarding **kwargs from _inner_get_response() to ollama.AsyncClient.chat(). When the orchestration layer (e.g. HandoffBuilder) injects kwargs like allow_multiple_tool_calls=True, these were passed through unfiltered, causing a TypeError since ollama.AsyncClient.chat() does not accept them. This aligns with how other clients (e.g. OpenAIChatClient) handle kwargs — they only pass the prepared options_dict to the provider's API, not the raw **kwargs from the middleware chain. Fixes microsoft#4402 Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
7dad593 to
7984fbf
Compare
Hi @TaoChenOSU — this PR was actually submitted first (#4403 at 22:33 UTC vs Beyond timing, the approaches differ:
Happy to make any adjustments you'd like, but if you've chose to go with the other fix, I'll go ahead and close. |
moonbox3
left a comment
There was a problem hiding this comment.
Automated Code Review
Reviewers: 3 | Confidence: 91%
✓ Correctness
The diff correctly fixes issue #4402 by stopping the forwarding of unsupported
**kwargstoollama.AsyncClient.chat()and addingallow_multiple_tool_callsto the set of keys excluded during option preparation. The three new tests cover the non-streaming kwargs path, the streaming kwargs path, and the options-dict path. The changes are focused, logically sound, and consistent with the existing codebase patterns.
✓ Security Reliability
This diff is a security improvement that removes the blind forwarding of **kwargs to the underlying Ollama AsyncClient.chat() call, preventing callers from injecting arbitrary, unvalidated parameters into the downstream API. The addition of 'allow_multiple_tool_calls' to exclude_keys and the accompanying regression tests are well-structured. No security or reliability issues are introduced by this change.
✓ Test Coverage
The diff removes forwarding of **kwargs to ollama.AsyncClient.chat() and adds allow_multiple_tool_calls to the exclude_keys set in _prepare_options. Three new tests cover the non-streaming kwargs path, streaming kwargs path, and options-dict path respectively. Coverage is solid for the specific regression case (issue #4402). Minor gaps: no test verifies that arbitrary/unknown kwargs (beyond allow_multiple_tool_calls) are also silently dropped, which would better document the broader behavioral change of removing **kwargs forwarding entirely. test_cmc_ignores_unsupported_options omits a result assertion but the intent is clear.
Suggestions
- Consider logging a debug-level warning when kwargs are silently dropped, so that users who accidentally pass an unsupported parameter get some feedback during development rather than silent no-op behavior.
- Consider logging a debug-level warning when kwargs are silently dropped so that callers can diagnose unexpected behaviour during development, rather than having parameters disappear without any signal.
- Consider adding a test that passes an arbitrary unknown kwarg (e.g.,
some_future_flag=True) toget_responseand asserts it does not appear in theself.client.chat()call. This would document the broader behavioral change (all kwargs are now dropped, not justallow_multiple_tool_calls). - In
test_cmc_ignores_unsupported_options, consider addingassert result.text == "test"for consistency with the other two new tests, to confirm the response is still valid after filtering.
Automated review by moonbox3's agents
| exclude_keys = {"instructions", "tool_choice"} | ||
| # Keys to exclude from processing — these are either handled separately | ||
| # or not supported by the Ollama API. | ||
| exclude_keys = {"instructions", "tool_choice", "allow_multiple_tool_calls"} |
There was a problem hiding this comment.
this is not a approach we want to take, since that parameter is already set to None in the OllamaChatOptions, people will get notified by their IDE if that is set, if that is being set by something like a workflow then we need to update that. The reason we don't want to hardcode filtering like this is because we want to not block future updates, let's say at some point Ollama does implement support for this parameter, then that will not be usable, because we filter it out, so we decided we would prefer to use the None above to notify the user beforehand that it is likely not right, and then we just let the API tell the user exactly what's wrong.
Motivation and Context
Stop forwarding **kwargs from _inner_get_response() to ollama.AsyncClient.chat(). When the orchestration layer (e.g. HandoffBuilder) injects kwargs like allow_multiple_tool_calls=True, these were passed through unfiltered, causing a TypeError since ollama.AsyncClient.chat() does not accept them.
Description
This aligns with how other clients (e.g. OpenAIChatClient) handle kwargs — they only pass the prepared options_dict to the provider's API, not the raw **kwargs from the middleware chain.
Fixes #4402
Contribution Checklist