-
Notifications
You must be signed in to change notification settings - Fork 1.5k
Make FallbackModel fall back on all model API errors, not just HTTP status 400+
#3494
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 6 commits
d05f012
a25612e
396c83e
1cc49a4
4292d52
fac51ed
706124c
0a8cea4
0025fb3
0624643
b4c2f4b
b725769
91a99ad
b86cb12
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 |
|---|---|---|
|
|
@@ -13,7 +13,7 @@ | |
| from pydantic_core import to_json | ||
| from typing_extensions import assert_never, deprecated | ||
|
|
||
| from .. import ModelHTTPError, UnexpectedModelBehavior, _utils, usage | ||
| from .. import ModelAPIError, ModelHTTPError, UnexpectedModelBehavior, _utils, usage | ||
| from .._output import DEFAULT_OUTPUT_TOOL_NAME, OutputObjectDefinition | ||
| from .._run_context import RunContext | ||
| from .._thinking_part import split_content_into_text_and_thinking | ||
|
|
@@ -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, | ||
|
|
@@ -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: | ||
|
Collaborator
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Why not the top level
Collaborator
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Please also update all the other models!
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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?
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. looking closer at this, there are three exceptions that inherit from we were already handling 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?
Collaborator
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Yep makes sense |
||
| raise ModelAPIError(model_name=self.model_name, message=e.message) from e | ||
|
|
||
| def _process_response(self, response: chat.ChatCompletion | str) -> ModelResponse: | ||
| """Process a non-streamed response, and prepare a message to return.""" | ||
|
|
@@ -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 ModelAPIError(model_name=self.model_name, message=e.message) from e | ||
|
|
||
| def _get_reasoning(self, model_settings: OpenAIResponsesModelSettings) -> Reasoning | Omit: | ||
| reasoning_effort = model_settings.get('openai_reasoning_effort', None) | ||
|
|
||
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.
Could we require a message instead of falling back on
model_name: ...?