-
Notifications
You must be signed in to change notification settings - Fork 1.3k
Add genai.errors handling in GoogleModel #3139
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
base: main
Are you sure you want to change the base?
Add genai.errors handling in GoogleModel #3139
Conversation
- Add error handling helper method `_handle_google_error` - Convert Google API errors to ModelHTTPError with proper status codes - Map specific function-related errors (400-level) appropriately - Keep original error details in response body - Add test cases for API error handling Resolves: pydantic#3088
7ef0b9b to
ae45a9b
Compare
| try: | ||
| return await func(model=self._model_name, contents=contents, config=config) # type: ignore | ||
| except ( | ||
| errors.APIError, | ||
| errors.UnknownFunctionCallArgumentError, | ||
| errors.UnsupportedFunctionError, | ||
| errors.FunctionInvocationError, | ||
| errors.UnknownApiResponseError, | ||
| ) as e: | ||
| raise self._handle_google_error(e) from e |
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.
I think we can do what we do in the other models, and just have a single except.
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.
I initially believed I needed to explicitly handle all of the more specific exceptions defined in google.genai.errors (e.g., UnknownFunctionCallArgumentError, UnknownApiResponseError) to provide more detailed error messages.
However, after reviewing the implementation for other models (like Groq and OpenAI, which only handle their respective APIStatusError subclasses to catch HTTP status codes ≥400), I realize I can simplify the approach.
I'll only focus on the main google.genai.errors.APIError to align with the standard pattern for re-raising a ModelHTTPError.
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.
Hi @Kludex ~
thank you for your review
I've changed the code to only return a ModelHttpError for errors that have an explicit Status Code, similar to how other models handle it.
9eabd64 to
09e053f
Compare
tests/models/test_google.py
Outdated
| ), | ||
| ], | ||
| ) | ||
| async def test_model_status_error( |
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.
should I make MockGoogle and inject mock client like groq test code do?
def test_model_status_error(allow_model_requests: None) -> None:
mock_client = MockGroq.create_mock(
APIStatusError(
'test error',
response=httpx.Response(status_code=500, request=httpx.Request('POST', 'https://example.com/v1')),
body={'error': 'test error'},
)
)
m = GroqModel('llama-3.3-70b-versatile', provider=GroqProvider(groq_client=mock_client))
agent = Agent(m)
with pytest.raises(ModelHTTPError) as exc_info:
agent.run_sync('hello')
assert str(exc_info.value) == snapshot(
"status_code: 500, model_name: llama-3.3-70b-versatile, body: {'error': 'test error'}"
)- Align with other model implementations (openai, groq) by removing unnecessary error transformation logic Resolves: pydantic#3088
09e053f to
eecaa4a
Compare
|
For testing, I think for http exceptions, mocking an HTTP response would be okay. Otherwise, you can't consistently reproduce this. |
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.
This PR is being reviewed by Cursor Bugbot
Details
You are on the Bugbot Free tier. On this plan, Bugbot will review limited PRs each billing cycle.
To receive Bugbot reviews on all of your PRs, visit the Cursor dashboard to activate Pro and start your 14-day free trial.
| try: | ||
| return await func(model=self._model_name, contents=contents, config=config) # type: ignore | ||
| except errors.APIError as e: | ||
| if (status_code := e.code) >= 400: |
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.
|
Hi @Kludex I identified that the test code was causing the CI failures, so I removed it entirely. Now, it seems the coverage test is failing as a result. Should I add the test code back? |
Yes, that's what I meant with my last comment. |
google.genai.errors. APIErrorhandling atGoogleModeltests/models/test_googe.py: test_model_status_errorResolves: #3088