-
Notifications
You must be signed in to change notification settings - Fork 1.5k
Transform and to text parts for Google models that don't support tools #3591
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?
Conversation
| # Only the first function call requires a signature | ||
| function_call_requires_signature = False | ||
| else: | ||
| part['text'] = f'Tool {item.tool_name} called with args {item.args_as_json_str()}' |
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.
Because the arguments and return value can be very long, I want those on a separate line.
Also because there could be parallel tool calls with the same name, we should include the tool call ID.
I'd also want to format this in a more special way so the model understands this is not actually a user/assistant "message".
So I suggest doing something similar to how we send text files to OpenAI:
pydantic-ai/pydantic_ai_slim/pydantic_ai/models/openai.py
Lines 1006 to 1014 in c27e5e4
| def _inline_text_file_part(text: str, *, media_type: str, identifier: str) -> ChatCompletionContentPartTextParam: | |
| text = '\n'.join( | |
| [ | |
| f'-----BEGIN FILE id="{identifier}" type="{media_type}"-----', | |
| text, | |
| f'-----END FILE id="{identifier}"-----', | |
| ] | |
| ) | |
| return ChatCompletionContentPartTextParam(text=text, type='text') |
And we're doing that for Google as well: #3269.
tests/models/test_google.py
Outdated
| ) | ||
|
|
||
|
|
||
| def test_google_mapping_when_does_not_support_tools(): |
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.
We should also test the mapping of the tool result
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.
@DouweM Is there a way I can generate a casette file (I was initially attempting to right my own, but after thinking about it that feels...wrong)? I wanted to test for this behavior via Agent.run.
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.
@sidg1215 If you run the test with uv run pytest <path>::<test> --record-mide=recreate, vcrpy will automatically record them for you :)
| text = '\n'.join( | ||
| [ | ||
| f'-----BEGIN TOOL RETURN name="{part.tool_name}" id="{part.tool_call_id}"-----', | ||
| f'response: {part.model_response_object()}', |
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.
Let's just use part.model_response_str() without the response: prefix
| text = '\n'.join( | ||
| [ | ||
| f'-----BEGIN TOOL CALL name="{item.tool_name} "id="{item.tool_call_id}""-----', | ||
| f'args: {item.args_as_json_str()}', |
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.
We can drop the args: prefix
| else: | ||
| text = '\n'.join( | ||
| [ | ||
| f'-----BEGIN TOOL CALL name="{item.tool_name} "id="{item.tool_call_id}""-----', |
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.
We've got a few too many quotes here
| provider_name='google-gla', | ||
| ), | ||
| 'google-gla', | ||
| True, |
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.
Let's default the arg to True so we don't have to pass it here
| ToolReturnPart( | ||
| tool_name='get_user_country', | ||
| content='Mexico', | ||
| tool_call_id=IsStr(), |
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.
Let's use an actual dummy value here that we'll see in the output below, instead of literal IsStr()
| ) | ||
| ) | ||
| else: | ||
| text = '\n'.join( |
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.
Let's move this to an as_text() method on ToolCallPart and explain in the docstring what it's for. In case we want to someday do this in other models as well. Same for ToolReturnPart
Fixes #3149.