From b2a883842b0434749bb189f473b08d8aa1c2a06f Mon Sep 17 00:00:00 2001 From: Francisco Javier Arceo Date: Thu, 9 Oct 2025 23:26:33 -0400 Subject: [PATCH] remove large try and except blocks and unnecessary _check_conversation_exists method Signed-off-by: Francisco Javier Arceo --- .../responses/openai_responses.py | 137 ++++++++---------- .../test_conversation_integration.py | 61 ++++---- 2 files changed, 90 insertions(+), 108 deletions(-) diff --git a/llama_stack/providers/inline/agents/meta_reference/responses/openai_responses.py b/llama_stack/providers/inline/agents/meta_reference/responses/openai_responses.py index 7e93db865..771a6e5de 100644 --- a/llama_stack/providers/inline/agents/meta_reference/responses/openai_responses.py +++ b/llama_stack/providers/inline/agents/meta_reference/responses/openai_responses.py @@ -25,7 +25,6 @@ from llama_stack.apis.agents.openai_responses import ( OpenAIResponseTextFormat, ) from llama_stack.apis.common.errors import ( - ConversationNotFoundError, InvalidConversationIdError, ) from llama_stack.apis.conversations import Conversations @@ -239,10 +238,8 @@ class OpenAIResponsesImpl: if not conversation.startswith("conv_"): raise InvalidConversationIdError(conversation) - conversation_exists = await self._check_conversation_exists(conversation) - if not conversation_exists: - raise ConversationNotFoundError(conversation) - + # Check conversation exists (raises ConversationNotFoundError if not) + _ = await self.conversations_api.get_conversation(conversation) input = await self._load_conversation_context(conversation, input) stream_gen = self._create_streaming_response( @@ -359,92 +356,84 @@ class OpenAIResponsesImpl: async def delete_openai_response(self, response_id: str) -> OpenAIDeleteResponseObject: return await self.responses_store.delete_response_object(response_id) - async def _check_conversation_exists(self, conversation_id: str) -> bool: - """Check if a conversation exists.""" - try: - await self.conversations_api.get_conversation(conversation_id) - return True - except ConversationNotFoundError: - return False - async def _load_conversation_context( self, conversation_id: str, content: str | list[OpenAIResponseInput] ) -> list[OpenAIResponseInput]: """Load conversation history and merge with provided content.""" - try: - conversation_items = await self.conversations_api.list(conversation_id, order="asc") + conversation_items = await self.conversations_api.list(conversation_id, order="asc") - context_messages = [] - for item in conversation_items.data: - if isinstance(item, OpenAIResponseMessage): - if item.role == "user": - context_messages.append( - OpenAIResponseMessage( - role="user", content=item.content, id=item.id if hasattr(item, "id") else None - ) + context_messages = [] + for item in conversation_items.data: + if isinstance(item, OpenAIResponseMessage): + if item.role == "user": + context_messages.append( + OpenAIResponseMessage( + role="user", content=item.content, id=item.id if hasattr(item, "id") else None ) - elif item.role == "assistant": - context_messages.append( - OpenAIResponseMessage( - role="assistant", content=item.content, id=item.id if hasattr(item, "id") else None - ) + ) + elif item.role == "assistant": + context_messages.append( + OpenAIResponseMessage( + role="assistant", content=item.content, id=item.id if hasattr(item, "id") else None ) + ) - # add new content to context - if isinstance(content, str): - context_messages.append(OpenAIResponseMessage(role="user", content=content)) - elif isinstance(content, list): - context_messages.extend(content) + # add new content to context + if isinstance(content, str): + context_messages.append(OpenAIResponseMessage(role="user", content=content)) + elif isinstance(content, list): + context_messages.extend(content) - return context_messages - - except Exception as e: - logger.error(f"Failed to load conversation context for {conversation_id}: {e}") - if isinstance(content, str): - return [OpenAIResponseMessage(role="user", content=content)] - return content + return context_messages async def _sync_response_to_conversation( self, conversation_id: str, content: str | list[OpenAIResponseInput], response: OpenAIResponseObject ) -> None: """Sync content and response messages to the conversation.""" - try: - conversation_items = [] + conversation_items = [] - # add user content message(s) - if isinstance(content, str): - conversation_items.append( - {"type": "message", "role": "user", "content": [{"type": "input_text", "text": content}]} - ) - elif isinstance(content, list): - for item in content: - if isinstance(item, OpenAIResponseMessage) and item.role == "user": - if isinstance(item.content, str): - conversation_items.append( - { - "type": "message", - "role": "user", - "content": [{"type": "input_text", "text": item.content}], - } - ) - elif isinstance(item.content, list): - conversation_items.append({"type": "message", "role": "user", "content": item.content}) + # add user content message(s) + if isinstance(content, str): + conversation_items.append( + {"type": "message", "role": "user", "content": [{"type": "input_text", "text": content}]} + ) + elif isinstance(content, list): + for item in content: + if not isinstance(item, OpenAIResponseMessage): + raise NotImplementedError(f"Unsupported input item type: {type(item)}") - # add assistant response message - for output_item in response.output: - if isinstance(output_item, OpenAIResponseMessage) and output_item.role == "assistant": - if hasattr(output_item, "content") and isinstance(output_item.content, list): + if item.role == "user": + if isinstance(item.content, str): conversation_items.append( - {"type": "message", "role": "assistant", "content": output_item.content} + { + "type": "message", + "role": "user", + "content": [{"type": "input_text", "text": item.content}], + } ) + elif isinstance(item.content, list): + conversation_items.append({"type": "message", "role": "user", "content": item.content}) + else: + raise NotImplementedError(f"Unsupported user message content type: {type(item.content)}") + elif item.role == "assistant": + if isinstance(item.content, list): + conversation_items.append({"type": "message", "role": "assistant", "content": item.content}) + else: + raise NotImplementedError(f"Unsupported assistant message content type: {type(item.content)}") + else: + raise NotImplementedError(f"Unsupported message role: {item.role}") - if conversation_items: - adapter = TypeAdapter(list[ConversationItem]) - validated_items = adapter.validate_python(conversation_items) + # add assistant response message + for output_item in response.output: + if isinstance(output_item, OpenAIResponseMessage) and output_item.role == "assistant": + if hasattr(output_item, "content") and isinstance(output_item.content, list): + conversation_items.append({"type": "message", "role": "assistant", "content": output_item.content}) + + if conversation_items: + adapter = TypeAdapter(list[ConversationItem]) + validated_items = adapter.validate_python(conversation_items) + try: await self.conversations_api.add_items(conversation_id, validated_items) - - except Exception as e: - logger.error(f"Failed to sync response {response.id} to conversation {conversation_id}: {e}") - # don't fail response creation if conversation sync fails - - return None + except Exception as e: + logger.error(f"Failed to sync response {response.id} to conversation {conversation_id}: {e}") + # don't fail response creation if conversation sync fails diff --git a/tests/unit/providers/agents/meta_reference/test_conversation_integration.py b/tests/unit/providers/agents/meta_reference/test_conversation_integration.py index f0f4ac90e..c29aca55b 100644 --- a/tests/unit/providers/agents/meta_reference/test_conversation_integration.py +++ b/tests/unit/providers/agents/meta_reference/test_conversation_integration.py @@ -53,33 +53,19 @@ def responses_impl_with_conversations( class TestConversationValidation: """Test conversation ID validation logic.""" - async def test_conversation_existence_check_valid(self, responses_impl_with_conversations, mock_conversations_api): - """Test conversation existence check for valid conversation.""" - conv_id = "conv_valid123" - - # Mock successful conversation retrieval - mock_conversations_api.get_conversation.return_value = Conversation( - id=conv_id, created_at=1234567890, metadata={}, object="conversation" - ) - - result = await responses_impl_with_conversations._check_conversation_exists(conv_id) - - assert result is True - mock_conversations_api.get_conversation.assert_called_once_with(conv_id) - - async def test_conversation_existence_check_invalid( + async def test_nonexistent_conversation_raises_error( self, responses_impl_with_conversations, mock_conversations_api ): - """Test conversation existence check for non-existent conversation.""" + """Test that ConversationNotFoundError is raised for non-existent conversation.""" conv_id = "conv_nonexistent" # Mock conversation not found mock_conversations_api.get_conversation.side_effect = ConversationNotFoundError("conv_nonexistent") - result = await responses_impl_with_conversations._check_conversation_exists(conv_id) - - assert result is False - mock_conversations_api.get_conversation.assert_called_once_with(conv_id) + with pytest.raises(ConversationNotFoundError): + await responses_impl_with_conversations.create_openai_response( + input="Hello", model="test-model", conversation=conv_id, stream=False + ) class TestConversationContextLoading: @@ -137,12 +123,8 @@ class TestConversationContextLoading: mock_conversations_api.list.side_effect = Exception("API Error") - result = await responses_impl_with_conversations._load_conversation_context(conv_id, input_text) - - assert len(result) == 1 - assert isinstance(result[0], OpenAIResponseMessage) - assert result[0].role == "user" - assert result[0].content == input_text + with pytest.raises(Exception, match="API Error"): + await responses_impl_with_conversations._load_conversation_context(conv_id, input_text) async def test_load_conversation_context_with_list_input( self, responses_impl_with_conversations, mock_conversations_api @@ -236,20 +218,31 @@ class TestMessageSyncing: async def test_sync_response_to_conversation_api_error( self, responses_impl_with_conversations, mock_conversations_api ): - """Test syncing when conversations API call fails.""" - conv_id = "conv_test123" - + mock_conversations_api.add_items.side_effect = Exception("API Error") mock_response = OpenAIResponseObject( id="resp_123", created_at=1234567890, model="test-model", object="response", output=[], status="completed" ) - # Mock API error - mock_conversations_api.add_items.side_effect = Exception("API Error") - - # Should not raise exception (graceful failure) - result = await responses_impl_with_conversations._sync_response_to_conversation(conv_id, "Hello", mock_response) + result = await responses_impl_with_conversations._sync_response_to_conversation( + "conv_test123", "Hello", mock_response + ) assert result is None + async def test_sync_unsupported_types(self, responses_impl_with_conversations): + mock_response = OpenAIResponseObject( + id="resp_123", created_at=1234567890, model="test-model", object="response", output=[], status="completed" + ) + + with pytest.raises(NotImplementedError, match="Unsupported input item type"): + await responses_impl_with_conversations._sync_response_to_conversation( + "conv_123", [{"not": "message"}], mock_response + ) + + with pytest.raises(NotImplementedError, match="Unsupported message role: system"): + await responses_impl_with_conversations._sync_response_to_conversation( + "conv_123", [OpenAIResponseMessage(role="system", content="test")], mock_response + ) + class TestIntegrationWorkflow: """Integration tests for the full conversation workflow."""