From 4c80e7017dbef43155f1a24e1c58c09c40fcf3d1 Mon Sep 17 00:00:00 2001 From: Yuge Zhang Date: Thu, 18 Sep 2025 11:57:37 +0800 Subject: [PATCH 1/2] Python: Fix falsy values being filtered out in ChatOptions.to_provider_settings (#787) * Python: Fix temperature=0 filtered out in chat options * fix failing tests --------- Co-authored-by: Chris <66376200+crickman@users.noreply.github.com> --- .../packages/main/agent_framework/_types.py | 9 +++- python/packages/main/tests/main/test_types.py | 53 +++++++++++++++++++ 2 files changed, 61 insertions(+), 1 deletion(-) diff --git a/python/packages/main/agent_framework/_types.py b/python/packages/main/agent_framework/_types.py index d3b3eab044..cf6aa1964f 100644 --- a/python/packages/main/agent_framework/_types.py +++ b/python/packages/main/agent_framework/_types.py @@ -1870,10 +1870,17 @@ class ChatOptions(AFBaseModel): # No tool choice if no tools are defined if self.tools is None or len(self.tools) == 0: default_exclude.add("tool_choice") + # No metadata and logit bias if they are empty + # Prevents 400 error + if not self.logit_bias: + default_exclude.add("logit_bias") + if not self.metadata: + default_exclude.add("metadata") + merged_exclude = default_exclude if exclude is None else default_exclude | set(exclude) settings = self.model_dump(exclude_none=True, by_alias=by_alias, exclude=merged_exclude) - settings = {k: v for k, v in settings.items() if v} + settings = {k: v for k, v in settings.items() if v is not None} settings.update(self.additional_properties) for key in merged_exclude: settings.pop(key, None) diff --git a/python/packages/main/tests/main/test_types.py b/python/packages/main/tests/main/test_types.py index 094d4ea494..87bf31f138 100644 --- a/python/packages/main/tests/main/test_types.py +++ b/python/packages/main/tests/main/test_types.py @@ -1138,6 +1138,59 @@ def test_chat_options_tool_choice_dict_mapping(ai_tool): assert settings["tool_choice"] == "required" +def test_chat_options_to_provider_settings_with_falsy_values(): + """Test that falsy values (except None) are included in provider settings.""" + options = ChatOptions( + temperature=0.0, # falsy but not None + top_p=0.0, # falsy but not None + presence_penalty=False, # falsy but not None + frequency_penalty=None, # None - should be excluded + additional_properties={"empty_string": "", "zero": 0, "false_flag": False, "none_value": None}, + ) + + settings = options.to_provider_settings() + + # Falsy values that are not None should be included + assert "temperature" in settings + assert isinstance(settings["temperature"], float) + assert settings["temperature"] == 0.0 + assert "top_p" in settings + assert isinstance(settings["top_p"], float) + assert settings["top_p"] == 0.0 + assert "presence_penalty" in settings + assert isinstance(settings["presence_penalty"], float) # converted to float + assert settings["presence_penalty"] == 0.0 + + # None values should be excluded + assert "frequency_penalty" not in settings + + # Additional properties - falsy values should always be included + assert "empty_string" in settings + assert settings["empty_string"] == "" + assert "zero" in settings + assert settings["zero"] == 0 + assert "false_flag" in settings + assert settings["false_flag"] is False + assert "none_value" in settings + assert settings["none_value"] is None + + +def test_chat_options_empty_logit_bias_and_metadata_excluded(): + """Test that empty logit_bias and metadata are excluded from provider settings.""" + options = ChatOptions( + ai_model_id="gpt-4o", + logit_bias={}, # empty dict should be excluded + metadata={}, # empty dict should be excluded + ) + + settings = options.to_provider_settings() + + # Empty logit_bias and metadata should be excluded + assert "logit_bias" not in settings + assert "metadata" not in settings + assert settings["model"] == "gpt-4o" + + # region AgentRunResponse From 1dba779f760c4712f07b3a2ff76e18559b1e3ad1 Mon Sep 17 00:00:00 2001 From: Yuge Zhang Date: Thu, 18 Sep 2025 12:13:30 +0800 Subject: [PATCH 2/2] Python: Enhance tool call handling and function result processing (#790) * Tool call enhancement * add tests * fix comments * resolve comments --------- Co-authored-by: Chris <66376200+crickman@users.noreply.github.com> Co-authored-by: Eric Zhu --- .../agent_framework/openai/_chat_client.py | 11 +- .../main/agent_framework/openai/_shared.py | 29 ++- .../tests/openai/test_openai_chat_client.py | 190 ++++++++++++++++++ 3 files changed, 216 insertions(+), 14 deletions(-) diff --git a/python/packages/main/agent_framework/openai/_chat_client.py b/python/packages/main/agent_framework/openai/_chat_client.py index 90776964be..1a4c63e5fe 100644 --- a/python/packages/main/agent_framework/openai/_chat_client.py +++ b/python/packages/main/agent_framework/openai/_chat_client.py @@ -185,10 +185,10 @@ class OpenAIBaseChatClient(OpenAIBase, BaseChatClient): if choice.finish_reason: finish_reason = FinishReason(value=choice.finish_reason) contents: list[Contents] = [] - if parsed_tool_calls := [tool for tool in self._get_tool_calls_from_chat_choice(choice)]: - contents.extend(parsed_tool_calls) if text_content := self._parse_text_from_choice(choice): contents.append(text_content) + if parsed_tool_calls := [tool for tool in self._get_tool_calls_from_chat_choice(choice)]: + contents.extend(parsed_tool_calls) messages.append(ChatMessage(role="assistant", contents=contents)) return ChatResponse( response_id=response.id, @@ -354,8 +354,13 @@ class OpenAIBaseChatClient(OpenAIBase, BaseChatClient): args["tool_calls"] = [self._openai_content_parser(content)] # type: ignore case FunctionResultContent(): args["tool_call_id"] = content.call_id - if content.result: + if content.result is not None: args["content"] = prepare_function_call_results(content.result) + elif content.exception is not None: + # Send the exception message to the model + # Otherwise we won't have any channels to talk to OpenAI + # TODO(yuge): This should ideally be customizable + args["content"] = "Error: " + str(content.exception) case _: if "content" not in args: args["content"] = [] diff --git a/python/packages/main/agent_framework/openai/_shared.py b/python/packages/main/agent_framework/openai/_shared.py index 9d5ab7e52f..0a62af4b93 100644 --- a/python/packages/main/agent_framework/openai/_shared.py +++ b/python/packages/main/agent_framework/openai/_shared.py @@ -50,21 +50,28 @@ __all__ = [ ] +def _prepare_function_call_results_as_dumpable(content: Contents | Any | list[Contents | Any]) -> Any: + if isinstance(content, list): + # Particularly deal with lists of BaseModel + return [_prepare_function_call_results_as_dumpable(item) for item in content] + if isinstance(content, dict): + return {k: _prepare_function_call_results_as_dumpable(v) for k, v in content.items()} + if isinstance(content, BaseModel): + return content.model_dump(exclude={"raw_representation", "additional_properties"}) + return content + + def prepare_function_call_results(content: Contents | Any | list[Contents | Any]) -> str | list[str]: """Prepare the values of the function call results.""" - if isinstance(content, list): - results: list[str] = [] - for item in content: - res = prepare_function_call_results(item) - if isinstance(res, list): - results.extend(res) - else: - results.append(res) - return results[0] if len(results) == 1 else json.dumps(results) if isinstance(content, BaseModel): - return content.model_dump_json(exclude_none=True, exclude={"raw_representation", "additional_properties"}) + # BaseModel is already dumpable, shortcut for performance + return content.model_dump_json(exclude={"raw_representation", "additional_properties"}) + + dumpable = _prepare_function_call_results_as_dumpable(content) + if isinstance(dumpable, str): + return dumpable # fallback - return json.dumps(content) + return json.dumps(dumpable) class OpenAISettings(AFBaseSettings): diff --git a/python/packages/main/tests/openai/test_openai_chat_client.py b/python/packages/main/tests/openai/test_openai_chat_client.py index dded958f23..3316058a74 100644 --- a/python/packages/main/tests/openai/test_openai_chat_client.py +++ b/python/packages/main/tests/openai/test_openai_chat_client.py @@ -1,11 +1,14 @@ # Copyright (c) Microsoft. All rights reserved. +import json import os +from datetime import datetime from typing import Annotated from unittest.mock import MagicMock, patch import pytest from openai import BadRequestError +from pydantic import BaseModel from agent_framework import ( AgentRunResponse, @@ -17,6 +20,7 @@ from agent_framework import ( ChatResponse, ChatResponseUpdate, DataContent, + FunctionResultContent, HostedWebSearchTool, TextContent, ToolProtocol, @@ -25,6 +29,7 @@ from agent_framework import ( from agent_framework.exceptions import ServiceInitializationError, ServiceResponseException from agent_framework.openai import OpenAIChatClient from agent_framework.openai._exceptions import OpenAIContentFilterException +from agent_framework.openai._shared import prepare_function_call_results skip_if_openai_integration_tests_disabled = pytest.mark.skipif( os.getenv("RUN_INTEGRATION_TESTS", "false").lower() != "true" @@ -592,6 +597,191 @@ async def test_exception_message_includes_original_error_details() -> None: assert original_error_message in exception_message +def test_chat_response_content_order_text_before_tool_calls(openai_unit_test_env: dict[str, str]): + """Test that text content appears before tool calls in ChatResponse contents.""" + # Import locally to avoid break other tests when the import changes + from openai.types.chat.chat_completion import ChatCompletion, Choice + from openai.types.chat.chat_completion_message import ChatCompletionMessage + from openai.types.chat.chat_completion_message_tool_call import ChatCompletionMessageToolCall, Function + + # Create a mock OpenAI response with both text and tool calls + mock_response = ChatCompletion( + id="test-response", + object="chat.completion", + created=1234567890, + model="gpt-4o-mini", + choices=[ + Choice( + index=0, + message=ChatCompletionMessage( + role="assistant", + content="I'll help you with that calculation.", + tool_calls=[ + ChatCompletionMessageToolCall( + id="call-123", + type="function", + function=Function(name="calculate", arguments='{"x": 5, "y": 3}'), + ) + ], + ), + finish_reason="tool_calls", + ) + ], + ) + + client = OpenAIChatClient() + response = client._create_chat_response(mock_response, ChatOptions()) + + # Verify we have both text and tool call content + assert len(response.messages) == 1 + message = response.messages[0] + assert len(message.contents) == 2 + + # Verify text content comes first, tool call comes second + assert message.contents[0].type == "text" + assert message.contents[0].text == "I'll help you with that calculation." + assert message.contents[1].type == "function_call" + assert message.contents[1].name == "calculate" + + +def test_function_result_falsy_values_handling(openai_unit_test_env: dict[str, str]): + """Test that falsy values (like empty list) in function result are properly handled.""" + client = OpenAIChatClient() + + # Test with empty list (falsy but not None) + message_with_empty_list = ChatMessage(role="tool", contents=[FunctionResultContent(call_id="call-123", result=[])]) + + openai_messages = client._openai_chat_message_parser(message_with_empty_list) + assert len(openai_messages) == 1 + assert openai_messages[0]["content"] == "[]" # Empty list should be JSON serialized + + # Test with empty string (falsy but not None) + message_with_empty_string = ChatMessage( + role="tool", contents=[FunctionResultContent(call_id="call-456", result="")] + ) + + openai_messages = client._openai_chat_message_parser(message_with_empty_string) + assert len(openai_messages) == 1 + assert openai_messages[0]["content"] == "" # Empty string should be preserved + + # Test with False (falsy but not None) + message_with_false = ChatMessage(role="tool", contents=[FunctionResultContent(call_id="call-789", result=False)]) + + openai_messages = client._openai_chat_message_parser(message_with_false) + assert len(openai_messages) == 1 + assert openai_messages[0]["content"] == "false" # False should be JSON serialized + + +def test_function_result_exception_handling(openai_unit_test_env: dict[str, str]): + """Test that exceptions in function result are properly handled. + + Feel free to remove this test in case there's another new behavior. + """ + client = OpenAIChatClient() + + # Test with exception (no result) + test_exception = ValueError("Test error message") + message_with_exception = ChatMessage( + role="tool", contents=[FunctionResultContent(call_id="call-123", exception=test_exception)] + ) + + openai_messages = client._openai_chat_message_parser(message_with_exception) + assert len(openai_messages) == 1 + assert openai_messages[0]["content"] == "Error: Test error message" + assert openai_messages[0]["tool_call_id"] == "call-123" + + +def test_prepare_function_call_results_with_basemodel(): + """Test prepare_function_call_results with BaseModel objects.""" + + class TestModel(BaseModel): + name: str + value: int + raw_representation: str = "should be excluded" + additional_properties: dict = {"should": "be excluded"} + + model_instance = TestModel(name="test", value=42) + result = prepare_function_call_results(model_instance) + + assert isinstance(result, str) + parsed = json.loads(result) + assert parsed["name"] == "test" + assert parsed["value"] == 42 + assert "raw_representation" not in parsed + assert "additional_properties" not in parsed + + +def test_prepare_function_call_results_with_nested_structures(): + """Test prepare_function_call_results with complex nested structures.""" + + class NestedModel(BaseModel): + id: int + raw_representation: str = "excluded" + + # Test with list of BaseModel objects + models = [NestedModel(id=1), [NestedModel(id=2)]] + result = prepare_function_call_results(models) + + assert isinstance(result, str) + parsed = json.loads(result) + assert len(parsed) == 2 + assert parsed[0]["id"] == 1 + assert isinstance(parsed[1], list) + assert len(parsed[1]) == 1 + assert parsed[1][0]["id"] == 2 + assert "raw_representation" not in parsed[0] + assert "raw_representation" not in parsed[1][0] + + +def test_prepare_function_call_results_with_dict_containing_basemodel(): + """Test prepare_function_call_results with dictionary containing BaseModel.""" + + class TestModel(BaseModel): + value: str + raw_representation: str = "excluded" + + # Test with dict containing BaseModel + complex_dict = {"model": TestModel(value="test"), "simple": "value", "number": 42} + + result = prepare_function_call_results(complex_dict) + + assert isinstance(result, str) + parsed = json.loads(result) + assert parsed["model"]["value"] == "test" + assert "raw_representation" not in parsed["model"] + assert parsed["simple"] == "value" + assert parsed["number"] == 42 + + +def test_prepare_function_call_results_string_passthrough(): + """Test that string values are passed through directly without JSON encoding.""" + result = prepare_function_call_results("simple string") + assert result == "simple string" + assert isinstance(result, str) + + +def test_prepare_function_call_results_with_none_values(): + """Test that None values in BaseModel fields are preserved to avoid validation errors during reloading.""" + + class Flight(BaseModel): + flight_id: str + departure: datetime | None + arrival: datetime | None + + # Test single BaseModel with None values (performance shortcut) + flight_with_nones = Flight(flight_id="123", departure=None, arrival=None) + result = prepare_function_call_results(flight_with_nones) + + assert isinstance(result, str) + parsed = json.loads(result) + assert parsed["flight_id"] == "123" + assert parsed["departure"] is None + assert parsed["arrival"] is None + + new_flight = Flight.model_validate_json(result) + assert new_flight == flight_with_nones + + def test_openai_content_parser_data_content_image(openai_unit_test_env: dict[str, str]) -> None: """Test _openai_content_parser converts DataContent with image media type to OpenAI format.""" client = OpenAIChatClient()