-
Notifications
You must be signed in to change notification settings - Fork 1.7k
[Firebase AI] Add internal JSON Schema support in GenerationConfig
#15404
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
# Conflicts: # FirebaseAI/Tests/Unit/GenerationConfigTests.swift
4343ab6
to
397ae19
Compare
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 adds internal support for response_json_schema
in GenerationConfig
. The changes include adding the new property, a corresponding internal initializer, and updating the Encodable
conformance. The implementation is well-tested with new unit and integration tests that cover various schema types. The changes look good and the new feature is well-implemented. I've left one comment regarding code duplication in the integration tests which could be addressed to improve maintainability.
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 adds internal support for JSON Schema in GenerationConfig
by introducing a responseJSONSchema
property. The changes include a new internal initializer to handle this property and ensure it's mutually exclusive with the existing responseSchema
.
The integration tests have been significantly refactored to be parameterized, allowing them to run against both OpenAPI schemas and the new JSON schemas. This is a great improvement for test coverage and maintainability.
My review includes suggestions to improve the custom Decodable
implementation for the MailingAddress
test helper. Specifically, I recommend decoupling it from the testing framework by throwing a DecodingError
on ambiguous data instead of using test assertions directly within the initializer. This will make the decoding logic more robust and align with standard Swift practices.
/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 adds internal support for JSON Schema in GenerationConfig
, introducing a new responseJSONSchema
property and a corresponding internal initializer. The changes are well-tested with new unit tests and a significant, well-executed refactoring of the integration tests to accommodate both OpenAPI and JSON schema formats. The test improvements, especially in SchemaTests.swift
, enhance maintainability and clarity. I have one suggestion to reduce code duplication in the GenerationConfig
initializers.
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.
Does this complete Step 1 of the workplan?
LGTM with double let warning addressed
@paulb777 Yes, exactly, this is Step 1. (I also updated the plan to note that I did go ahead and drop the function calling schema part in this PR -- I still have the code stashed though) |
Added internal support for JSON Schema (
response_json_schema
) inGenerationConfig
.#no-changelog