-
Notifications
You must be signed in to change notification settings - Fork 1.5k
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
Add prototype for Firebase Vertex AI #12357
Conversation
57e4c8e
to
44073e5
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.
Just a few minor remarks.
I love that most of the code for the samples can stay the same!
SafetyRatingsSection(ratings: ratings) | ||
} | ||
|
||
case GenerateContentError.invalidAPIKey: |
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 might no longer be applicable, and the message we issue in line 155 might mislead people.
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 call. I updated the message to refer to the GoogleService-Info.plist
instead since an invalidAPIKey
error in this SDK would be because of Firebase mis-configuration (or revocation of the Firebase app's API key).
I would probably leave this error case out entirely if the prototype weren't built on top of the existing generative-ai-swift
SDK.
) | ||
} | ||
|
||
case GenerateContentError.unsupportedUserLocation: |
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 wonder if this is still going to be an issue. Even when providing europe-west1
, the model happily answers.
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 removed this case from the switch statement since it'll never be thrown with Vertex AI. Gemini is available on Vertex AI in these Cloud regions but can be called from anywhere (e.g., I could specify europe-west1
but latency from North America will be higher).
/// This instance is configured with the default `FirebaseApp`. | ||
public static func generativeModel(modelName: String, location: String) -> GoogleGenerativeAI | ||
.GenerativeModel { | ||
return generativeModel(app: FirebaseApp.app()!, modelName: modelName, location: location) |
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.
Force unwrap makes me feel uneasy...
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.
Added a fatalError
instead of a force-unwrap. The following error message will also be logged in this scenario:
firebase-ios-sdk/FirebaseCore/Sources/FIRApp.m
Lines 238 to 245 in 86ae5de
FIRLogError(kFIRLoggerCore, @"I-COR000003", | |
@"The default Firebase app has not yet been " | |
@"configured. Add `FirebaseApp.configure()` to your " | |
@"application initialization. This can be done in " | |
@"in the App Delegate's application(_:didFinishLaunchingWithOptions:)` " | |
@"(or the `@main` struct's initializer in SwiftUI). " | |
@"Read more: https://goo.gl/ctyzm8."); | |
return nil; |
modelResouceName = modelResourceName | ||
} | ||
|
||
private static func modelResourceName(app: FirebaseApp, modelName: 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.
Shouldn't this rather be modelResourcePath
? In line 93, we return a path (or at least a path fragment).
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 chose this function name because it's a model resource name in resource-oriented design (see https://google.aip.dev/122).
Note: The field in the REST API is called model
instead of name
.
) | ||
return GenerativeModel( | ||
name: modelResouceName, | ||
apiKey: app.options.apiKey!, |
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.
Force unwrap makes me feel uneasy...
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.
Added a fatalError
instead of the force unwrap.
return modelName | ||
} | ||
guard let projectID = app.options.projectID else { | ||
print("The FirebaseApp is missing a project ID.") |
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.
In case of a misconfiguration like a missing project ID, shouldn't we at least issue a warning on the log?
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 changed this to a fatalError
as well. If projectID
is nil
then something is wrong with the Firebase app configuration.
return modelName | ||
} | ||
|
||
return "projects/\(projectID)/locations/\(location)/publishers/google/models/\(modelName)" |
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.
There doesn't seem to be a check for the location.
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.
Since location
isn't an optional parameter I added a check for ""
. Beyond that I'll leave it up to the backend to return a 404 error if an invalid location is specified.
|
||
// MARK: Request Hooks | ||
|
||
/// Adds an App Check token to the provided request, if App Check is included in the app. |
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.
Remove the comma before if
Adds an App Check token to the provided request if App Check is included in the app.
Also, do we add the token even if App Check is just included in the app? Or does it have to be enabled for us to send the token?
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.
It's good to send the App Check token, if it's included in the app, even if it's not enabled for the Firebase project / app. If it's not enabled at all then it'll just be ignored by the backend. If App Check is enabled but unenforced then developers will be able to see how many requests would be blocked if/when they enforce App Check.
This reverts commit af7d783. # Conflicts: # FirebaseVertexAI/Sources/VertexAI.swift # Package.swift
Co-authored-by: Paul Beusterien <[email protected]>
Closing. VertexAI support is now in main |
Do not merge. This is a prototype that uses Vertex AI with the Google Generative AI SDK.
Usage:
#no-changelog