-
Notifications
You must be signed in to change notification settings - Fork 1.7k
[AI] Structured output from FirebaseGenerable #15650
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: ah/ai-generable
Are you sure you want to change the base?
Conversation
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. |
|
/gemini review |
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 valuable feature by adding a generateObject overload for FirebaseGenerable types, enabling structured output without a dependency on FoundationModels. The implementation is well-executed, with clean and effective use of Swift's Codable for schema conversion. The changes to JSONSchema to support on-the-fly generation of its internal representation are a great improvement, fixing a potential crash. The addition of comprehensive integration tests ensures the new functionality is robust and works as expected across different models. Overall, this is a high-quality contribution. I have one minor suggestion regarding the JSON response parsing for improved robustness.
|
/gemini review |
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 structured output generation for types conforming to the new FirebaseGenerable protocol. The changes include a new generateObject method in GenerativeModel, schema conversion logic in JSONSchema, and helper methods for parsing the JSON response. A potential crash in JSONSchema encoding has also been fixed by ensuring an internal representation is always created. New integration tests are added to verify the end-to-end functionality. The changes are well-implemented and tested. I have one minor suggestion to improve documentation clarity.
|
/gemini review |
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 structured output support using FirebaseGenerable, refactors JSON parsing, and adds corresponding integration tests. The changes are well-structured. My review includes suggestions to improve documentation clarity, refactor duplicated code for better maintainability, and enhance test consistency.
FirebaseAI/Tests/TestApp/Tests/Integration/GenerateObjectIntegrationTests.swift
Show resolved
Hide resolved
|
/gemini review |
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 support for structured output using FirebaseGenerable, refactoring existing logic for FoundationModels.Generable to share common functionality. The changes include consolidating JSON response parsing, cleaning up JSON strings by removing markdown, and adding comprehensive integration tests for the new capabilities. The code is well-organized, and the documentation has been updated accordingly. I have one minor suggestion to enhance code style.
|
/gemini review |
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 structured output generation for types conforming to FirebaseGenerable, aligning its functionality with the existing support for FoundationModels.Generable. The changes include a significant refactoring to consolidate JSON response parsing, cleaning, and generation configuration logic into shared helper functions, which greatly improves code clarity and maintainability. Additionally, robust integration tests have been added to verify the new functionality, including support for Apple's @Generable macro. My feedback focuses on a suggestion to further reduce code duplication between the two generateObject methods.
|
/gemini review |
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 structured output support for FirebaseGenerable, refactoring the existing generateObject method to accommodate this and reduce code duplication. The changes include consolidating JSON response parsing, implementing a more robust JSON cleanup mechanism, and adding comprehensive integration tests to verify compatibility with both FirebaseGenerable and Apple's @Generable macro. Overall, the changes are well-executed and significantly enhance the SDK's capabilities. I have one suggestion to further improve the robustness of the JSON cleaning logic.
|
/gemini review |
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 significant and well-executed refactoring to support structured output via FirebaseGenerable. The consolidation of logic into a shared _generateObject method is a great improvement for code clarity and maintainability. The addition of comprehensive integration tests for both FirebaseGenerable and Apple's @Generable is also a valuable contribution. I've identified one high-severity issue that could lead to a runtime crash if a user defines a schema with duplicate property names. My review includes suggestions to address this by adding validation and making the schema generation function throwing.
|
/gemini review |
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 support for structured output with FirebaseGenerable, refactors GenerativeModel to consolidate shared logic, and adds comprehensive integration tests. The changes are well-structured and significantly improve the SDK's capabilities. The refactoring of generateObject into a private helper is a good design choice that enhances code reuse and maintainability. The new integration tests are thorough, covering both the new FirebaseGenerable and existing FoundationModels.Generable paths. Overall, this is an excellent contribution. I have one minor suggestion to further improve the new test file's maintainability.
FirebaseAI/Tests/TestApp/Tests/Integration/GenerateObjectIntegrationTests.swift
Outdated
Show resolved
Hide resolved
|
/gemini review |
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 is a great improvement. It refactors the generateObject functionality to support both FoundationModels.Generable and a new FirebaseGenerable protocol, consolidating the implementation into a shared private helper function. The logic for cleaning and parsing JSON responses from the model has been centralized and made more robust. The changes to JSONSchema to correctly build the internal schema representation and remove a potential fatalError are excellent. The addition of a comprehensive suite of integration tests for the new functionality is also a major plus.
I have a couple of minor suggestions for improving code efficiency and clarity, but overall this is a high-quality contribution.
|
/gemini review |
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 is a well-executed refactoring and feature addition. It successfully consolidates the logic for generateObject to support both FoundationModels.Generable and the new FirebaseGenerable, significantly reducing code duplication. The introduction of helper functions for cleaning and parsing JSON responses improves robustness and maintainability. The addition of comprehensive integration tests, including for Apple's @Generable macro compatibility, is a great enhancement. The code is clean, well-structured, and the documentation updates are clear. I have one minor suggestion to improve code conciseness in the new test file.
FirebaseAI/Tests/TestApp/Tests/Integration/GenerateObjectIntegrationTests.swift
Show resolved
Hide resolved
|
/gemini summarize |
Summary of ChangesThis pull request significantly enhances the Firebase AI SDK by introducing structured output capabilities for generative models. It allows developers to define Swift types that the model can populate directly, improving type safety and simplifying data handling. The changes involve refactoring core generation logic, consolidating JSON processing, and strengthening JSON schema validation, all backed by new integration tests to ensure reliability and compatibility with existing frameworks. 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. Changelog
Activity
|
| let kind: Kind? | ||
| let source: String? | ||
| let schema: JSONSchema.Internal? | ||
|
|
||
| init(kind: Kind, source: String) { |
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.
Question (non-blocking), why are these fields optional? Could a explanation via code comment be added? The initializer takes in non-optionals, and defaults the constant schema to be nil. There is property unwrapping elsewhere in the implementation that I was wondering if could be avoided.
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.
Addressing in #15656
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 JSONSchema.Internal approach was entirely a workaround because I didn't have time to implement the rest before vacation but it looks like we're going with it now?
#no-changelog