-
Notifications
You must be signed in to change notification settings - Fork 594
refactor(plugins/google-genai): migrate to v2 API #3542
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
Conversation
2f8295a to
eb9aa5f
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.
Pull Request Overview
This PR migrates the Google Genkit plugin from the v1 to v2 API, refactoring how models, embedders, and background models are defined and managed. The migration simplifies the plugin architecture by removing the explicit Genkit instance parameter and transitioning to a more functional approach.
- Rename
model()functions tocreateModelRef()for consistency - Migrate from v1 plugin architecture to v2 with updated initialization and resolution patterns
- Update function signatures to remove Genkit instance parameters and return action objects directly
Reviewed Changes
Copilot reviewed 22 out of 22 changed files in this pull request and generated 2 comments.
Show a summary per file
| File | Description |
|---|---|
| js/plugins/google-genai/tests/vertexai/veo_test.ts | Update test mocks and function calls for v2 API migration |
| js/plugins/google-genai/tests/vertexai/lyria_test.ts | Replace model() calls with createModelRef() and update defineModel calls |
| js/plugins/google-genai/tests/vertexai/index_test.ts | Update test assertions and function calls for v2 API changes |
| js/plugins/google-genai/tests/vertexai/imagen_test.ts | Replace model function references and update defineModel signature |
| js/plugins/google-genai/tests/vertexai/gemini_test.ts | Update createModelRef calls and remove Genkit parameter from defineModel |
| js/plugins/google-genai/tests/vertexai/embedder_test.ts | Update defineEmbedder calls to remove Genkit parameter |
| js/plugins/google-genai/tests/googleai/veo_test.ts | Replace model() with createModelRef() and update test setup |
| js/plugins/google-genai/tests/googleai/index_test.ts | Update test structure for v2 plugin API and function renaming |
| js/plugins/google-genai/tests/googleai/imagen_test.ts | Replace model calls with createModelRef and update defineModel signature |
| js/plugins/google-genai/tests/googleai/gemini_test.ts | Update function calls and remove Genkit parameters from model definitions |
| js/plugins/google-genai/tests/googleai/embedder_test.ts | Update embedder definition calls to remove Genkit parameter |
| js/plugins/google-genai/src/vertexai/veo.ts | Implement v2 API with createModelRef and backgroundModel functions |
| js/plugins/google-genai/src/vertexai/lyria.ts | Migrate to v2 API removing Genkit parameter and using model function |
| js/plugins/google-genai/src/vertexai/index.ts | Complete plugin migration to v2 architecture with new init/resolve pattern |
| js/plugins/google-genai/src/vertexai/imagen.ts | Update to v2 API with createModelRef and model function |
| js/plugins/google-genai/src/vertexai/gemini.ts | Migrate gemini models to v2 API and remove Genkit parameters |
| js/plugins/google-genai/src/vertexai/embedder.ts | Update embedder implementation for v2 API with new function signatures |
| js/plugins/google-genai/src/googleai/veo.ts | Implement v2 changes with createModelRef and backgroundModel |
| js/plugins/google-genai/src/googleai/index.ts | Complete googleAI plugin migration to v2 architecture |
| js/plugins/google-genai/src/googleai/imagen.ts | Update imagen models to v2 API pattern |
| js/plugins/google-genai/src/googleai/gemini.ts | Migrate gemini implementation to v2 API structure |
| js/plugins/google-genai/src/googleai/embedder.ts | Update embedder for v2 API with new function signatures |
Tip: Customize your code reviews with copilot-instructions.md. Create the file or learn how to get started.
cabljac
left a comment
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.
lgtm, make sure to test in dev UI to ensure we're not changing the display names of anything (same goes for all model plugin migrations)
cabljac
left a comment
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.
generally lgtm - one comment about an any we've introduced, would rather not.
eb9aa5f to
3a8d64a
Compare
Co-authored-by: Jacob Cable <[email protected]>
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 only reviewed the googleai part. I assume you did similar things for vertexai, so once you apply the comments to both I'll have a look again.
(In general, please do not change the existing logic or structure or naming if possible. This has been crafted and tested very carefully and thoroughly. I'm finding all sorts of random logic changes and you need to put them all back the way they were so it's not broken or wrong.)
c28612f to
5596504
Compare
|
@ifielker 🤦♂️ All good points here. I've definitely overcomplicated this when I was unfamiliar with migrating. I've made the changes, so hopefully now it should be as you'd expect. I'll also double check my other migrations to make sure I haven't done something similar with those. |
5596504 to
3efe1e1
Compare
…b.com/firebase/genkit into @invertase/migrate-googlegenai-plugin
3efe1e1 to
14a0263
Compare
ifielker
left a comment
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 stopped my review partway through again. I'll pick up where I left off once you address these comments.
| export type KnownModels = keyof typeof KNOWN_MODELS; // For autocomplete | ||
|
|
||
| export function model( | ||
| export function createEmbedderRef( |
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.
No need to rename this.
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 basing it on #3542 (comment). Also, it seems more obvious to me that it would follow the same pattern used everywhere else in the code. I am aware that you've suggested importing model as pluginModel to avoid these name changes, but doesn't the name createModelRef/createEmbedderRef provide a clearer description of what the function does?
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.
Hey @CorieW let's pair up on this and review today
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.
Hey @ifielker we're happy to revert to the original internal name model here if you prefer.
Originally I think the change to add the Ref suffix came from looking at the compat-openai plugin example (although that naming doesn't use create in it either actually, i think it uses e.g openAIModelRef etc for factories like this.)
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 public API has model return a reference so also it'd be good to have consistency and keep it as model
| import { GenkitPlugin, genkitPlugin } from 'genkit/plugin'; | ||
| import { ActionType } from 'genkit/registry'; | ||
| import { listModels } from './client.js'; | ||
| import { EmbedderReference, ModelReference, z } from 'genkit'; |
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 are 3 groups of imports:
- things imported from non-local files (that don't end in .js)
- model imports: "import * as ... "
- things imported from local files (that end in .js)
Please keep them organized the same way.
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.
Ah, I see. Which is your preference:
- Keep them the same as googleai index.ts and vertexai index.ts.
- Organise them as you've suggested.
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've updated the code and organised the imports as you've outlined.
3f4a9bb to
7068764
Compare
7068764 to
fc0f970
Compare
|
Some tests seem to need fixing, investigating this now. |
|
Closing in favour of #3649 |
Resolves #3452
Checklist (if applicable):