-
Notifications
You must be signed in to change notification settings - Fork 1.7k
[AI] Server Prompt Templates #15402
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?
[AI] Server Prompt Templates #15402
Conversation
Generated by 🚫 Danger |
|
|
||
| /// A chat session that allows for conversation with a model. | ||
| @available(iOS 15.0, macOS 12.0, macCatalyst 15.0, tvOS 15.0, watchOS 8.0, *) | ||
| public class TemplateChatSession { |
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.
Just a quick thing I noticed: public final class TemplateChatSession: Sendable. This would likely require some refactoring with the history. I'm wondering if we should extract
firebase-ios-sdk/FirebaseAI/Sources/Chat.swift
Lines 29 to 52 in 455d291
| private let historyLock = NSLock() | |
| private nonisolated(unsafe) var _history: [ModelContent] = [] | |
| /// The previous content from the chat that has been successfully sent and received from the | |
| /// model. This will be provided to the model for each message sent as context for the discussion. | |
| public var history: [ModelContent] { | |
| get { | |
| historyLock.withLock { _history } | |
| } | |
| set { | |
| historyLock.withLock { _history = newValue } | |
| } | |
| } | |
| private func appendHistory(contentsOf: [ModelContent]) { | |
| historyLock.withLock { | |
| _history.append(contentsOf: contentsOf) | |
| } | |
| } | |
| private func appendHistory(_ newElement: ModelContent) { | |
| historyLock.withLock { | |
| _history.append(newElement) | |
| } | |
| } |
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.
Done in 004fb94
|
/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 server-side prompt templates, a significant feature addition. The changes include new models (TemplateGenerativeModel, TemplateImagenModel), request types, and chat session logic to support using templates with variables. The code is well-structured, with good separation of concerns, such as moving history management into its own History class. I've identified a couple of areas where force unwraps on URL creation could be made safer. Overall, this is a solid implementation with good test coverage.
| // TODO: Fix this. | ||
| urlString += "?alt=sse" | ||
| } | ||
| return URL(string: urlString)! |
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.
Using a force unwrap ! for URL creation can lead to a runtime crash if the URL string is malformed. It's safer to use guard let to safely unwrap the optional URL and provide a descriptive error message if it fails. This improves code robustness and debuggability.
guard let url = URL(string: urlString) else {
fatalError("Could not construct URL for TemplateGenerateContentRequest. URL string was: \(urlString)")
}
return urlThere 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.
I think we should take this as an action item for the other request types too since we're force unwrapping in those. My first reaction was that it was unnecessary but given that location and template (model for the other requests) are entered by the dev it's definitely possible that the URL construction could fail.
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.
|
/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 server-side prompt templates for both generative and image generation models. This is a significant feature addition, including new model types like TemplateGenerativeModel and TemplateImagenModel, new request structures, and a TemplateChatSession for conversational AI with templates. The changes also involve some nice refactoring, such as abstracting history management into a dedicated History class. The overall implementation is well-structured and comes with a good set of unit and integration tests. My feedback is focused on improving robustness by avoiding force-unwraps in URL construction, ensuring proper URL encoding of user-provided data, and making the TemplateVariable type more versatile by handling Float values. I've also included a minor suggestion to clean up test code.
FirebaseAI/Tests/TestApp/Tests/Integration/ServerPromptTemplateIntegrationTests.swift
Outdated
Show resolved
Hide resolved
|
/gemini summarize |
|
@andrewheard @cynthiajoan Still some test tweaks once the backend supports both API providers with the same templates and this won't merge until after M172, but should now be ready for review. |
Updated `GenerateImagesRequest` to encode the `variables` property as `inputs`, similar to how `TemplateGenerateContentRequest` handles it. This ensures consistency across the API.
Updated `GenerateImagesRequest` to correctly construct the URL, including the `projectID` and `location`. This mirrors the URL construction in `TemplateGenerateContentRequest` and fixes a 404 error that was occurring in the `testGenerateImages` integration test. The `TemplateImagenModel` was also updated to pass the `projectID` to the `GenerateImagesRequest` initializer.
| @available(iOS 15.0, macOS 12.0, macCatalyst 15.0, tvOS 15.0, watchOS 8.0, *) | ||
| extension GenerateContentRequest { | ||
| enum APIMethod: String { | ||
| case generateContent | ||
| case streamGenerateContent | ||
| case countTokens | ||
| } | ||
| } |
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.
I think we should consider undoing this (along with the addition of the file APIMethod.swift file since these custom verbs are specific to GenerateContentRequest.
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.
Done.
| if case let .vertexAI(_, location) = apiConfig.service { | ||
| urlString += "/locations/\(location)" | ||
| } | ||
| urlString += "/templates/\(template):templateGenerateContent" |
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.
This should be templateStreamGenerateContent when stream == true.
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.
Good catch! Done
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.
Please undo.
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.
Done.
|
|
||
| @available(iOS 15.0, macOS 12.0, macCatalyst 15.0, tvOS 15.0, watchOS 8.0, *) | ||
| class TemplateGenerateImagesRequest: @unchecked Sendable, GenerativeAIRequest { |
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.
Could we follow the same pattern as ImagenGenerationRequest? That way it'll be prepared for images saved to GCS in the future.
Does this need to be a class? I think it could be a struct like ImagenGenerationRequest.
Any reason it needs to be @unchecked Sendable?
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.
Done
| enum ImageAPIMethod: String { | ||
| case generateImages = "templatePredict" | ||
| } |
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.
I think this should be nested inside TemplateGenerateImagesRequest since it's very specific to SPT.
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.
Done
|
/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 enhancement to the Firebase AI SDK by adding support for server-side prompt templates. The changes are well-structured, with new classes for template-based models (TemplateGenerativeModel, TemplateImagenModel) and a dedicated TemplateChatSession. The refactoring of chat history into a separate History class is a great improvement for modularity and thread safety. Additionally, the move from force-unwrapping URLs to a throwing mechanism enhances the robustness of the networking layer. My review focuses on ensuring URL components are properly encoded, maintaining consistency in error handling, and improving the maintainability of test utilities. Overall, this is a high-quality contribution.
Co-authored-by: gemini-code-assist[bot] <176961590+gemini-code-assist[bot]@users.noreply.github.com>
Apple implementation of Server Prompt Templates inspired by Flutterfire PR.
Googlers: see API review at go/firebase-ai-server-prompt-template
Summary of Changes
This pull request significantly enhances the Firebase AI SDK by introducing support for server-side prompt templates. This allows developers to define and use reusable templates for generative AI models, making it easier to manage complex prompts and integrate dynamic data. The changes include new model types for generative and image AI, a dedicated chat session for template-based conversations, and a refactored, thread-safe history management system. These additions provide a more robust and flexible framework for building AI-powered applications.
Highlights
TemplateGenerativeModelandTemplateImagenModelclasses to support server-side prompt templates, allowing for more flexible and dynamic AI interactions.TemplateChatSessionto enable conversational interactions with generative models using predefined templates and dynamic variables, supporting both single messages and streaming responses.Historyclass, improving modularity, reusability, and thread safety for managing conversation history in both standard and template-based chat sessions.TemplateVariableenum to handle various data types (String, Int, Double, Bool, Array, Dictionary) for substituting values into prompt templates, including automatic conversion forFloattoDouble.TemplateGenerateContentRequest,TemplateGenerateImagesRequest) and internal API method enums (APIMethod,ImageAPIMethod) to support the new template-based API endpoints.Changelog
Historyclass for managing chat history, removing internal history management logic and related locks.appendHistorycalls to use the_historyinstance of the newHistoryclass.templateGenerativeModel()andtemplateImagenModel()to initialize new template-based AI models.APIMethodenum, which has been moved to a new, dedicated internal file.firebaseInfoproperty fromprivate lettoletto allow access from newly introduced template models.Historyclass to encapsulate chat history management, including thread-safe appending ofModelContentand aggregation of content chunks.TemplateChatSessionfor managing chat conversations with server-side prompt templates, providingsendMessageandsendMessageStreammethods.TemplateGenerativeModelfor interacting with generative AI models using templates, including methods for content generation and starting chat sessions.TemplateImagenModelfor interacting with image generation models using templates.TemplateVariableenum to handle various data types for template variables, including a conversion fromFloattoDoubleduring initialization.APIMethodenum (generateContent, streamGenerateContent, countTokens) here fromGenerateContentRequest.swift.ImageAPIMethodenum specifically for image generation API methods, such astemplatePredict.TemplateGenerativeModelandTemplateImagenModel, covering text generation, image generation, and chat sessions with templates and media.TemplateChatSessionclass, verifyingsendMessageandsendMessageStreamfunctionality.TemplateGenerativeModelclass, testinggenerateContentandgenerateContentStreammethods.TemplateImagenModelclass, specifically testing thegenerateImagesmethod.httpRequestHandlerto include anisTemplateRequestparameter, allowing for conditional URL path assertions to support template-based requests in mock tests.collectTextFromStreamto simplify collecting text fromAsyncThrowingStreamresponses in tests.Activity
andrewheardsuggested extracting chat history management into a separate type, which was subsequently implemented bypaulb777.gemini-code-assist[bot]highlighted potential issues with force unwrapping URLs in new request types and suggested percent-encoding template names for robustness.paulb777acknowledged these, noting consistency with existing code.gemini-code-assist[bot]to handleFloatvalues inTemplateVariableinitialization was addressed and implemented in the changes.gemini-code-assist[bot]included aTODOfor streaming and a hardcoded path in a test utility.