-
Notifications
You must be signed in to change notification settings - Fork 1.7k
[AI] Structured Output streaming #15652
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: pb-ah-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 streaming support for structured outputs, which is a great feature. The implementation includes a new PartialJSONParser to handle incomplete JSON data from the stream, along with new generateObjectStream methods on GenerativeModel.
My review has identified a critical bug in the PartialJSONParser related to handling escaped characters in strings. I've also found a high-severity issue in GenerativeModel that can cause the final streamed object to be yielded twice. Additionally, I've suggested improvements to the unit tests for the new parser to increase its robustness.
Overall, the changes are well-structured, but addressing these issues is important for the feature to work correctly and reliably.
|
/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 streaming support for structured outputs (generateObjectStream). The core logic is implemented in a new private _generateObjectStream function in GenerativeModel.swift, which relies on a new PartialJSONParser to handle incomplete JSON chunks from the stream. New integration and unit tests are added to cover this functionality. My review focuses on the correctness and robustness of the new streaming and parsing logic. I've identified a potential issue in _generateObjectStream regarding redundant processing and the possibility of not yielding the final object in certain edge cases. I've also suggested expanding the unit tests for the new PartialJSONParser to cover more scenarios.
|
/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 streaming support for structured object generation, which is a great feature. The implementation includes a new PartialJSONParser to handle incomplete JSON streams and new generateObjectStream methods on GenerativeModel. The accompanying integration and unit tests are thorough.
My review includes a few points:
- A high-severity bug in the
PartialJSONParserthat can lead to incorrect parsing of numbers. - A medium-severity suggestion to consider performance for very large streaming responses.
- A low-severity comment about API consistency.
Overall, this is a solid implementation of a complex feature. Addressing the parsing bug is the most critical point.
| var fullText = "" | ||
| // Accumulate the response text. | ||
| for try await chunk in responseStream { | ||
| if let text = chunk.text { | ||
| fullText += text | ||
| } | ||
|
|
||
| // Attempt to parse and yield partial results. | ||
| let cleanText = GenerativeModel.cleanedJSON(from: fullText) | ||
| let parser = PartialJSONParser(input: cleanText) | ||
| if let jsonValue = parser.parse() { | ||
| do { | ||
| let rawContent = ModelOutput(jsonValue: jsonValue) | ||
| let contentValue = try contentProvider(rawContent) | ||
| continuation.yield(Response(content: contentValue, rawContent: rawContent)) | ||
| } catch { | ||
| // Ignore conversion errors for partial 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.
The current implementation accumulates the entire response text in fullText and re-parses it from the beginning with each new chunk. For very large streaming responses, this could become inefficient in terms of both memory (for fullText) and CPU (for re-parsing).
A potential future optimization could be to design the PartialJSONParser to work with a stream of characters or chunks directly, avoiding the need to buffer the entire response and re-parse known-good sections.
While the current approach is simple and likely sufficient for typical response sizes, it's worth keeping this in mind for future performance improvements.
|
/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 streaming support for structured JSON output. The changes include a new PartialJSONParser to handle incomplete JSON data from the stream, new generateObjectStream methods in GenerativeModel to provide an AsyncThrowingStream of response objects, and comprehensive unit and integration tests for the new functionality.
The implementation is well-structured. The partial parser is robust and well-tested, and the streaming logic in GenerativeModel correctly handles yielding both partial and final results. The new integration tests provide good coverage for the end-to-end streaming flow.
I have one minor suggestion to improve debuggability by adding a log for ignored errors during partial content conversion.
|
/gemini summarize |
Summary of ChangesThis pull request significantly enhances the Firebase AI SDK by introducing structured output streaming. It provides new 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
|
This pull request significantly enhances the Firebase AI SDK by introducing structured output streaming. It provides new
generateObjectStreammethods within theGenerativeModelclass, allowing developers to receive AI-generated content as a stream of strongly-typed objects conforming to a specified JSON schema. A key component of this feature is the newly implementedPartialJSONParser, which intelligently handles and parses incomplete JSON fragments as they arrive, ensuring a smooth and resilient streaming experience. This change enables more dynamic and interactive AI applications by providing immediate feedback as the model generates its response.Highlights
generateObjectStreammethods to allow models to stream structured JSON output, enhancing interaction with AI models by providing strongly-typed objects.PartialJSONParserto robustly handle and parse incomplete JSON responses received during streaming, enabling real-time processing of partial data.generateContentStream: An overload forgenerateContentStreamwas added to support passing an optionalGenerationConfig, providing more flexibility in content generation.generateObjectStreamfunctionality and thePartialJSONParser's robustness across various scenarios, including partial and malformed JSON.