adapt_openai_assistent_field_check#8080
Conversation
There was a problem hiding this comment.
Summary of Changes
Hello @Huixxi, 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 refines the ChatCompletionMessageGenericParam model to better align with OpenAI's API specifications regarding message content. It adjusts the content field to be optional by default but introduces a robust validation mechanism to ensure that content or tool_calls are appropriately provided based on the message's role, preventing invalid message constructions and addressing the need identified in issue #8074.
Highlights
- API Protocol Adaptation: The
contentfield within theChatCompletionMessageGenericParammodel has been updated to be explicitlyOptionaland default toNone. This change allows for scenarios where message content might not be present, such as whentool_callsare utilized instead. - Conditional Content Validation: I've introduced a new Pydantic
model_validatornamedvalidate_content_or_tool_callsto enforce specific content requirements based on the message'srole. Forassistantroles,contentis now optional only iftool_callsare provided. Conversely, forsystemandtoolroles, thecontentfield remains mandatory.
Using Gemini Code Assist
The 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 in your pull request via creating an issue comment (i.e. comment on the pull request page) using either /gemini <command> or @gemini-code-assist <command>. Below is a summary of the supported commands.
| Feature | Command | Description |
|---|---|---|
| Code Review | /gemini review |
Performs a code review for the current pull request in its current state. |
| Pull Request Summary | /gemini summary |
Provides a summary of the current pull request in its current state. |
| Comment | @gemini-code-assist | Responds in comments when explicitly tagged, both in issue comments and review comments. |
| Help | /gemini help |
Displays a list of available commands. |
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 .gemini/ folder in the base of the repository. Detailed instructions can be found here.
Limitations & Feedback
Gemini Code Assist is currently in preview and 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 to provide feedback.
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
-
Review the Privacy Notices, Generative AI Prohibited Use Policy, Terms of Service, and learn how to configure Gemini Code Assist in GitHub here. Gemini can make mistakes, so double check it and use code with caution. ↩
There was a problem hiding this comment.
Code Review
This pull request makes the content field in ChatCompletionMessageGenericParam optional and adds a Pydantic model validator to enforce that content is required unless tool_calls is specified for the assistant role. The validator logic has been reviewed and suggestions were made to improve robustness and clarity of error messages.
| """Validate that content is required unless tool_calls is specified for assistant role.""" | ||
| if self.role == "assistant": | ||
| # Check if tool_calls is empty or None | ||
| has_tool_calls = self.tool_calls is not None and len(self.tool_calls) > 0 |
There was a problem hiding this comment.
This line could raise an AttributeError. The validator is on the base class ChatCompletionMessageGenericParam, which doesn't always have a tool_calls attribute. While the ChatCompletionMessageAssistantParam subclass does, it's possible to instantiate the base class directly with role="assistant", which would cause a crash.
To make this code more robust, use getattr() for safe access and simplify the check using bool(), as an empty list or None will evaluate to False.
| has_tool_calls = self.tool_calls is not None and len(self.tool_calls) > 0 | |
| has_tool_calls = bool(getattr(self, "tool_calls", None)) |
There was a problem hiding this comment.
I think the suggestion doesn't cover the situation when self.tool_calls is an empty list.
There was a problem hiding this comment.
@CatherineSue may also take a look if you have spare time.
Motivation
[Feature] Maybe the content field in ChatCompletionMessageGenericParam should be Optional #8074
Modifications
[Feature] Maybe the content field in ChatCompletionMessageGenericParam should be Optional #8074
Checklist