-
Notifications
You must be signed in to change notification settings - Fork 322
feat: add Gemini model provider #725
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?
feat: add Gemini model provider #725
Conversation
Would you be able to share a sample test script to test the model provider? :) |
@@ -67,6 +67,9 @@ docs = [ | |||
"sphinx-rtd-theme>=1.0.0,<2.0.0", | |||
"sphinx-autodoc-typehints>=1.12.0,<2.0.0", | |||
] | |||
gemini = [ | |||
"google-genai>=0.5.0", |
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.
can we upper bound this to 0.6.0 so we don't consume breaking changes. We got caught with this with A2A not too long ago.
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.
Makes sense, will be updated in the following commit. I also notice that there is a newer version we can use. I will just test that and make the change.
class GeminiModel(Model): | ||
"""Google Gemini model provider implementation.""" | ||
|
||
SAFETY_MESSAGES = {"safety", "harmful", "content policy", "blocked due to safety"} |
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.
can you link to where these are coming from? same with below
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.
Will be done in the following commit. I also notice that I used string matching, instead of matching the enums they provide so I will be doing a bit of refactoring here
messages: Messages, | ||
tool_specs: Optional[list[ToolSpec]] = None, | ||
system_prompt: Optional[str] = None, | ||
config: Optional[dict[str, Any]] = 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.
do we need config here? isn't it available through the class?
T = TypeVar("T", bound=BaseModel) | ||
|
||
|
||
class GeminiModel(Model): |
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.
Just generally, can you provide links throughout this. For the next contributor it will be easier to understand whats going on if for example GenerationConfig is linked to in the code
|
||
return tools | ||
|
||
def format_request( |
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.
why public?
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 just generally followed the pattern used by other model providers for this helper method for consistency
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.
Originally format_request
was an overridable method but that was changed for the 1.0 release. update_config
, get_config
, structured_output
, and stream
are the only methods that need to be public.
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.
Got it, thanks!
Gemini formatted function response. | ||
""" | ||
response_parts = [] | ||
for content in tool_result["content"]: |
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.
Similar to the high level I think links would help review and debug. But do we not need to consider response types like image type?
Description
This PR adds the Gemini model provider support to Strands
Related Issues
Documentation PR
TBD -- will edit description once its out
Type of Change
New feature
Testing
How have you tested the change? Verify that the changes do not break functionality or introduce warnings in consuming repositories: agents-docs, agents-tools, agents-cli
pre-commit run --all-files
hatch run prepare
hatch test
-- can verify all tests passhatch run test-integ
-- did not get a lot of useful information out of this (most tests skipped because of missing auth keys of those providers) but I did run my own integ test file for gemini that I addedhatch test tests_integ/models/test_model_gemini.py
. They all passNote to reviewer: re: integ tests -- a couple of the integ tests I added for Gemini can get flaky because of the nature of the tests, so I set the temperature to 0.15. They can still fail intermittently. If we do not feel the need to include a bunch of these tests and only include a few to be consistent with the other model provider integ tests, please let me know
Checklist
By submitting this pull request, I confirm that you can use, modify, copy, and redistribute this contribution, under the terms of your choice.