Make FallbackModel fall back on all model API errors, not just HTTP status 400+
#3494
Conversation
| 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 |
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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(mainlytest_fallback). - will try to just add it to the exsiting tests instead of duplicating the amount of tests.
- will checkout the Bedrock case
There was a problem hiding this comment.
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
- handle connection errors for bedrock and openai
| 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: |
There was a problem hiding this comment.
Why not the top level APIError?
There was a problem hiding this comment.
Please also update all the other models!
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
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?
- handle api error for other models
DouweM
changed the title
| model_name: str | ||
| """The name of the model associated with the error.""" | ||
|
|
||
| def __init__(self, model_name: str, message: str | None = None): |
There was a problem hiding this comment.
Could we require a message instead of falling back on model_name: ...?
| 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: |
DouweM
changed the title
FallbackModel fall back on all model API errors, not just HTTP status 400+
Head branch was pushed to by a user without write access
Head branch was pushed to by a user without write access
2 participants
Fixes #3439
wraps openai.APIConnectionError in ModelHTTPError to enable fallback when on connection errors