-
Notifications
You must be signed in to change notification settings - Fork 651
feat(py/plugins/xai): Added plugin config, modified samples #4289
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
Summary of ChangesHello @zarinn3pal, I'm Gemini Code Assist1! I'm currently reviewing this pull request and will post my feedback shortly. In the meantime, here's a summary to help you and other reviewers quickly get up to speed! This pull request standardizes the Python samples to achieve feature parity with their JavaScript counterparts. It primarily focuses on refining the xAI plugin by introducing a dedicated configuration class for xAI models, improving the handling of model parameters and tool calls, and streamlining the definition of supported models. These changes enhance the robustness, maintainability, and consistency of the Genkit Python ecosystem. Highlights
🧠 New Feature in Public Preview: You can now enable Memory to help Gemini Code Assist learn from your team's feedback. This makes future code reviews more consistent and personalized to your project's style. Click here to enable Memory in your admin console. Using Gemini Code AssistThe full guide for Gemini Code Assist can be found on our documentation page, here are some quick tips. Invoking Gemini You can request assistance from Gemini at any point by creating a comment using either
Customization To customize Gemini Code Assist for GitHub experience, repository maintainers can create a configuration file and/or provide a custom code review style guide (such as PEP-8 for Python) by creating and adding files to a Limitations & Feedback Gemini Code Assist may make mistakes. Please leave feedback on any instances where its feedback is incorrect or counter productive. You can react with 👍 and 👎 on @gemini-code-assist comments. If you're interested in giving your feedback about your experience with Gemini Code Assist for Github and other Google products, sign up here. You can also get AI-powered code generation, chat, as well as code reviews directly in the IDE at no cost with the Gemini Code Assist IDE Extension. Footnotes
|
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.
Code Review
This pull request introduces a dedicated XAIConfig for the xAI plugin, which is a great improvement for handling model-specific parameters. The refactoring to use this config and the new build_generation_usage helper function cleans up the code nicely. The standardization of the Python samples is also a good step towards feature parity with other language samples. I've identified a potential bug in handling tool responses and a small improvement for exception handling. Overall, this is a solid PR with good refactoring and feature enhancement.
| chat_pb2.Message( | ||
| role=chat_pb2.MessageRole.ROLE_TOOL, | ||
| name=actual_part.tool_response.ref, | ||
| content=[chat_pb2.Content(text=str(actual_part.tool_response.output))], | ||
| ) |
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.
The tool_call_id is missing for tool responses. This is necessary for the model to associate the tool output with the correct tool request. The previous implementation used an incorrect name parameter, and while removing it is correct, the tool_call_id should be added. The ref from tool_response should be used as the tool_call_id.
chat_pb2.Message(
role=chat_pb2.MessageRole.ROLE_TOOL,
tool_call_id=actual_part.tool_response.ref,
content=[chat_pb2.Content(text=str(actual_part.tool_response.output))],
)| if not isinstance(config, XAIConfig): | ||
| try: | ||
| config = XAIConfig.model_validate(config) | ||
| except Exception: |
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.
Catching a broad Exception can hide configuration issues from the user. It's better to catch the specific pydantic.ValidationError here to make the code more robust and provide clearer feedback on invalid configurations. You'll need to import ValidationError from pydantic. Consider logging a warning when validation fails, so the user is aware that their provided configuration was ignored and default values are being used.
| except Exception: | |
| except ValidationError: |
| def get_weather(input: WeatherInput) -> dict: | ||
| """Get weather information for a location. | ||
| def get_weather(input: WeatherInput) -> str: | ||
| """Return a random realistic weather string for a city name. |
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 please update these flows in other samples as well?
| model = XAIModel(model_name=clean_name, client=self._xai_client) | ||
| model_info = get_model_info(clean_name) | ||
|
|
||
| from genkit.plugins.xai.models import XAIConfig |
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 imports to the top of the module
|
|
||
| def _create_sample_request() -> GenerateRequest: | ||
| """Create a sample generation request for testing.""" | ||
| from genkit.plugins.xai.models import XAIConfig |
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 imports to the top of the module
|
|
||
| @ai.flow() | ||
| async def weather_flow(location: Annotated[str, Field(default='San Francisco, CA')] = 'San Francisco, CA') -> str: | ||
| async def weather_flow(location: Annotated[str, Field(default='London')] = 'London') -> 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.
let's fix the location for all weather flows across the samples
PR to standardize python samples for feature parity with JS samples.