-
Notifications
You must be signed in to change notification settings - Fork 1.2k
Handle function calls without text #2557
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?
Handle function calls without text #2557
Conversation
UnexpectedModelBehavior
)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.
@ethanabrooks Can you show me the error you'd get without this? It's odd that Google could generate a model response with only tool calls and no text, that it then wouldn't accept when sent back on a subsequent request.
# If we only have function calls without text, add minimal text to satisfy Google API | ||
if has_function_calls and not has_text_parts: | ||
# Add a minimal text part to make the conversation valid for Google API | ||
parts.append({'text': 'I have completed the function calls above.'}) |
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.
Does this need text or could be empty?
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 do this inside _content_model_response based on the ModelResponse's parts rather than parsing the returned ContentDict? We already iterate over all items there, so we can keep track of what we've seen.
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.
Updated the description to include the full error. Yes your suggestion sounds like a good idea.
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.
Does this need text or could be empty?
Gave that a shot, but still got:
google.genai.errors.ClientError: 400 INVALID_ARGUMENT. {'error': {'code': 400, 'message': 'Unable to submit request because it must have a text parameter. Add a text parameter and try again. Learn more: https://cloud.google.com/vertex-ai/generative-ai/docs/model-reference/gemini', 'status': 'INVALID_ARGUMENT'}}
Could do something more minimal like 'Function call complete'
.
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 do this inside _content_model_response based on the ModelResponse's parts rather than parsing the returned ContentDict? We already iterate over all items there, so we can keep track of what we've seen.
Just pushed a new commit that does this. Might need to iterate on the tests.
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 is only a Vertex AI issue right? Could we gate it by checking if self.system
so that we don't send unnecessary tokens to the non-Vertex Gemini API?
Does it accept ' '
? :) I wonder how strict their check is, and I'd rather not waste any tokens
@ethanabrooks The error is odd, because there is in fact a We already have similar logic for the pydantic-ai/pydantic_ai_slim/pydantic_ai/models/google.py Lines 455 to 457 in eeeb32d
This looks like a bug on Google's side, but I agree we should work around it. |
@ethanabrooks By the way nice to see you using Pydantic AI with Temporal! Is that already using our brand-new integration? I'd love feedback on Slack! |
We just had a big planning meeting where talked about upgrading to v0.7 and using the new temporal features. Will keep you posted! |
Might need some help with the test-coverage issue. It seems to be referencing lines that I didn't change... Also I get the test-coverage error on main as well. |
9facb04
to
e456ad3
Compare
e456ad3
to
8f38d24
Compare
How strange, line 584 doesn't even have a |
@ethanabrooks Just a heads-up that I'll be out this coming week and will be back the 25th. Assuming this is not urgent I'll review it then. If it is, please ping |
Not a problem. I'll check again next week. |
This addresses the following error: