Skip to content
Merged
Show file tree
Hide file tree
Changes from 2 commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
6 changes: 5 additions & 1 deletion pydantic_ai_slim/pydantic_ai/models/openai.py
Original file line number Diff line number Diff line change
Expand Up @@ -54,7 +54,7 @@
from . import Model, ModelRequestParameters, StreamedResponse, check_allow_model_requests, download_item, get_user_agent

try:
from openai import NOT_GIVEN, APIStatusError, AsyncOpenAI, AsyncStream
from openai import NOT_GIVEN, APIConnectionError, APIStatusError, AsyncOpenAI, AsyncStream
from openai.types import AllModels, chat, responses
from openai.types.chat import (
ChatCompletionChunk,
Expand Down Expand Up @@ -546,6 +546,8 @@ async def _completions_create(
if (status_code := e.status_code) >= 400:
raise ModelHTTPError(status_code=status_code, model_name=self.model_name, body=e.body) from e
raise # pragma: lax no cover
except APIConnectionError as e:
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why not the top level APIError?

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Please also update all the other models!

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

will do, I only looked at google but I didn't see any logic handling it and remembered there's another PR adding support for Fallback for the google models, is that still open?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

looking closer at this, there are three exceptions that inherit from APIError

class APIResponseValidationError(APIError):
class APIStatusError(APIError):
class APIConnectionError(APIError):

we were already handling APIStatusError, now this PR is handling APIConnectionError, but IMO APIResponseValidationError doesn't really belong with the other two, and in a case of bad validation the model should retry instead of falling back.

to be clearer, the two we're handling are issues that likely won't fix themselves, like a bad credential, a bad request, connection or server issues, while a validation error should be rare and should fix itself. when it doesn't fix itself it provides information to the user: "this model doesn't handle your use case very well apparently".

does that make sense?

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yep makes sense

raise ModelHTTPError(status_code=0, model_name=self.model_name, body=str(e)) from e

def _process_response(self, response: chat.ChatCompletion | str) -> ModelResponse:
"""Process a non-streamed response, and prepare a message to return."""
Expand Down Expand Up @@ -1252,6 +1254,8 @@ async def _responses_create(
if (status_code := e.status_code) >= 400:
raise ModelHTTPError(status_code=status_code, model_name=self.model_name, body=e.body) from e
raise # pragma: lax no cover
except APIConnectionError as e:
raise ModelHTTPError(status_code=0, model_name=self.model_name, body=str(e)) from e
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

As I'm sure you also noticed when you wrote this, we're kind of abusing the ModelHTTPError class now because this is not actually an HTTP response with status code 0.

I suggest we add a new superclass of ModelHTTPError like ModelAPIError without a status_code field, and raise that instead.

We should update FallbackModel to have that in the default on_fallback list (and update the docs if needed)

And I think we should review every single model class and raise the new error for non-HTTP errors.

I think Bedrock currently incorrectly raises ModelHTTPError for non-HTTP errors as well, so that one will need further tweaking.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

yeah that's part of what we discussed that handling connection errors is a bit outside the original job of the FallbackModel

  • I'll create the new class and add it as default to the FallbackModel.on_fallback
  • I'll check wherever we're already testing the FallbackModel (mainly test_fallback).
  • will try to just add it to the exsiting tests instead of duplicating the amount of tests.
  • will checkout the Bedrock case

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

just pushed an update with these changes, CI fails cause of the flaky test_outlines so can't verify coverage, but the PR is in a reviewable state


def _get_reasoning(self, model_settings: OpenAIResponsesModelSettings) -> Reasoning | Omit:
reasoning_effort = model_settings.get('openai_reasoning_effort', None)
Expand Down
12 changes: 12 additions & 0 deletions tests/models/test_fallback.py
Original file line number Diff line number Diff line change
Expand Up @@ -570,6 +570,18 @@ async def test_fallback_condition_tuple() -> None:
assert response.output == 'success'


async def test_fallback_connection_error() -> None:
def connection_error_response(_model_messages: list[ModelMessage], _agent_info: AgentInfo) -> ModelResponse:
raise ModelHTTPError(status_code=0, model_name='test-connection-model', body='Connection timed out')

connection_error_model = FunctionModel(connection_error_response)
fallback_model = FallbackModel(connection_error_model, success_model)
agent = Agent(model=fallback_model)

response = await agent.run('hello')
assert response.output == 'success'


async def test_fallback_model_settings_merge():
"""Test that FallbackModel properly merges model settings from wrapped model and runtime settings."""

Expand Down
18 changes: 17 additions & 1 deletion tests/models/test_openai.py
Original file line number Diff line number Diff line change
Expand Up @@ -56,7 +56,7 @@
)

with try_import() as imports_successful:
from openai import APIStatusError, AsyncOpenAI
from openai import APIConnectionError, APIStatusError, AsyncOpenAI
from openai.types import chat
from openai.types.chat.chat_completion import ChoiceLogprobs
from openai.types.chat.chat_completion_chunk import (
Expand Down Expand Up @@ -1146,6 +1146,22 @@ def test_model_status_error(allow_model_requests: None) -> None:
assert str(exc_info.value) == snapshot("status_code: 500, model_name: gpt-4o, body: {'error': 'test error'}")


def test_model_connection_error(allow_model_requests: None) -> None:
mock_client = MockOpenAI.create_mock(
APIConnectionError(
message='Connection to http://localhost:11434/v1 timed out',
request=httpx.Request('POST', 'http://localhost:11434/v1'),
)
)
m = OpenAIChatModel('gpt-4o', provider=OpenAIProvider(openai_client=mock_client))
agent = Agent(m)
with pytest.raises(ModelHTTPError) as exc_info:
agent.run_sync('hello')
assert exc_info.value.status_code == 0
assert exc_info.value.model_name == 'gpt-4o'
assert 'Connection to http://localhost:11434/v1 timed out' in str(exc_info.value.body)


@pytest.mark.parametrize('model_name', ['o3-mini', 'gpt-4o-mini', 'gpt-4.5-preview'])
async def test_max_completion_tokens(allow_model_requests: None, model_name: str, openai_api_key: str):
m = OpenAIChatModel(model_name, provider=OpenAIProvider(api_key=openai_api_key))
Expand Down
Loading