-
Notifications
You must be signed in to change notification settings - Fork 1.8k
Python: fix(python): Filter unsupported kwargs in OllamaChatClient._inner_get_response #4404
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
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -404,6 +404,93 @@ async def test_cmc_with_dict_tool_passthrough( | |
| assert call_kwargs["tools"] == [{"type": "function", "function": {"name": "custom_tool", "parameters": {}}}] | ||
|
|
||
|
|
||
| @patch.object(AsyncClient, "chat", new_callable=AsyncMock) | ||
| async def test_cmc_filters_unsupported_kwargs( | ||
| mock_chat: AsyncMock, | ||
| ollama_unit_test_env: dict[str, str], | ||
| chat_history: list[Message], | ||
| mock_chat_completion_response: OllamaChatResponse, | ||
| ) -> None: | ||
| """Verify that unsupported kwargs (e.g. allow_multiple_tool_calls) are | ||
| silently filtered out and never forwarded to ollama.AsyncClient.chat(). | ||
|
|
||
| Regression test for https://github.com/microsoft/agent-framework/issues/4402 | ||
| """ | ||
| mock_chat.return_value = mock_chat_completion_response | ||
| chat_history.append(Message(text="hello world", role="user")) | ||
|
|
||
| ollama_client = OllamaChatClient() | ||
| # Pass allow_multiple_tool_calls as a top-level kwarg — this is what HandoffBuilder does | ||
| await ollama_client.get_response( | ||
| messages=chat_history, | ||
| allow_multiple_tool_calls=True, | ||
| ) | ||
|
Comment on lines
+422
to
+427
|
||
|
|
||
| # Verify the call succeeded and allow_multiple_tool_calls was NOT forwarded | ||
| mock_chat.assert_called_once() | ||
| call_kwargs = mock_chat.call_args.kwargs | ||
| assert "allow_multiple_tool_calls" not in call_kwargs | ||
|
|
||
|
|
||
| @patch.object(AsyncClient, "chat", new_callable=AsyncMock) | ||
| async def test_cmc_streaming_filters_unsupported_kwargs( | ||
| mock_chat: AsyncMock, | ||
| ollama_unit_test_env: dict[str, str], | ||
| chat_history: list[Message], | ||
| mock_streaming_chat_completion_response: AsyncStream[OllamaChatResponse], | ||
| ) -> None: | ||
| """Verify that unsupported kwargs are filtered in streaming mode too. | ||
|
|
||
| Regression test for https://github.com/microsoft/agent-framework/issues/4402 | ||
| """ | ||
| mock_chat.return_value = mock_streaming_chat_completion_response | ||
| chat_history.append(Message(text="hello world", role="user")) | ||
|
|
||
| ollama_client = OllamaChatClient() | ||
| result = ollama_client.get_response( | ||
| messages=chat_history, | ||
| stream=True, | ||
| allow_multiple_tool_calls=True, | ||
| ) | ||
|
|
||
| async for chunk in result: | ||
| assert chunk.text == "test" | ||
|
|
||
| # Verify allow_multiple_tool_calls was NOT forwarded | ||
| mock_chat.assert_called_once() | ||
| call_kwargs = mock_chat.call_args.kwargs | ||
| assert "allow_multiple_tool_calls" not in call_kwargs | ||
|
|
||
|
|
||
| @patch.object(AsyncClient, "chat", new_callable=AsyncMock) | ||
| async def test_cmc_filters_unsupported_options( | ||
| mock_chat: AsyncMock, | ||
| ollama_unit_test_env: dict[str, str], | ||
| chat_history: list[Message], | ||
| mock_chat_completion_response: OllamaChatResponse, | ||
| ) -> None: | ||
| """Verify that unsupported keys inside the options dict (e.g. from | ||
| Agent.default_options or workflow cloning) are also stripped before | ||
| reaching ollama.AsyncClient.chat(). | ||
|
|
||
| Regression test for https://github.com/microsoft/agent-framework/issues/4402 | ||
| """ | ||
| mock_chat.return_value = mock_chat_completion_response | ||
| chat_history.append(Message(text="hello world", role="user")) | ||
|
|
||
| ollama_client = OllamaChatClient() | ||
| # Pass allow_multiple_tool_calls inside the options dict | ||
| await ollama_client.get_response( | ||
| messages=chat_history, | ||
| options={"allow_multiple_tool_calls": True}, | ||
| ) | ||
|
|
||
| # Verify the call succeeded and allow_multiple_tool_calls was NOT forwarded | ||
| mock_chat.assert_called_once() | ||
| call_kwargs = mock_chat.call_args.kwargs | ||
| assert "allow_multiple_tool_calls" not in call_kwargs | ||
|
|
||
|
|
||
| @patch.object(AsyncClient, "chat", new_callable=AsyncMock) | ||
| async def test_cmc_with_data_content_type( | ||
| mock_chat: AsyncMock, | ||
|
|
||
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 is not a approach we want to take, since that parameter is already set to
Nonein 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 theNoneabove 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.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.
So reading more of the above, the real issue is that the HandoffBuilder set's parameters that are not universally applicable, which is what we would need to fix.