-
Notifications
You must be signed in to change notification settings - Fork 3.8k
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 function sample for server-side Remote Config + Vertex AI Gemini API #1135
Conversation
const userInput = request.query.prompt || ''; | ||
|
||
// Instantiate Vertex AI. | ||
const vertex_ai = new VertexAI({ project: project, 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.
Are project and location needed, or is that covered by Application Default Credentials?
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.
Yep, looks like they are needed and it doesn't automatically pull ADC. :/ Thinking it's because you can use Vertex in a different locale than your project.
// Export the function. | ||
exports.generateWithVertex = onRequest(async (request, response) => { | ||
|
||
try { |
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 would be great to add some kind of auth check here to encourage developers to protect their AI endpoints. #1134 (comment) does that with onCall
+ AppCheck + Auth, but this could check Auth with something like: https://firebase.google.com/docs/auth/web/service-worker-sessions#server_side_changes
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.
Oh, you do have the vertex_enabled
flag already. Auth would be ideal, but that flag is probably ok for now
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.
But the likelihood of folks leaving vertex_enabled to true exists...so I will get some auth in here! I think the service-worker-sessions method would actually work really well with the tutorial, so I'll try this out.
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.
So I wasn't able to get this working, but it looks like what I really want for this function (verifying the service account since this example uses a service account) is coming soon in firebase/firebase-admin-node#2466 - - so I cheated here and added a caveat to the README and created another callable function in call-vertex-remote-config-server that demonstrates App Check. And then I can plan to jump back in here and add the google-auth service account verification once it's in.
Do you think that works? Gonna ask for re-review because there are a lot of changes. :D
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.
Thank you for adding a Vertex + Server RC sample, Jen!
Co-authored-by: Jeff <3759507+jhuleatt@users.noreply.github.com>
…, and App Check. h/t to @hsubox76 for vertexai sample client example.
> functions-samples@1.0.0 bootstrap > pnpm -r install Scope: all 80 workspace projects Lockfile is up to date, resolution step is skipped Done in 1.4s
…e onRequest sample
Update README, add instructions for hosting deploy to emulator.
Add more information about building deliverables in client/dist and deploying w/the emulator.
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.
Looks great, just a small nit about the node version -- 18 is approaching end of life so was wondering if it'd make sense to bump it to 20?
I think it would...but it looks like we still reference 18 in the other function samples, and the basic template we create when we init functions with the Firebase CLI is still 18. Do you think it makes sense to bump the version in separate PRs that update 18 to 20 everywhere (and adds 22 to the CLI, as it's in preview) as a (bigger) separate effort? |
Oh that's a great point. In that case, absolutely updating in a separate PR would be the best path to go about it, I can look into updating the CLI as well. Thank you so much!! |
Thank YOU!!! Appreciate the review! :D |
Adds the function and snippets to support https://firebase.google.com/docs/remote-config/solution-server