-
Notifications
You must be signed in to change notification settings - Fork 630
feat(py): Implemented PluginV2 infrastructure #4040
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?
Conversation
huangjeff5
commented
Jan 5, 2026
- Implemented PluginV2 interface (init, resolve, list)
- Refactored Anthropic plugin
- Tested manually using Anthropic plugin
Summary of ChangesHello @huangjeff5, I'm Gemini Code Assist1! I'm currently reviewing this pull request and will post my feedback shortly. In the meantime, here's a summary to help you and other reviewers quickly get up to speed! This pull request significantly upgrades the plugin architecture within Genkit by introducing a 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. 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. You can also get AI-powered code generation, chat, as well as code reviews directly in the IDE at no cost with the Gemini Code Assist IDE Extension. Footnotes
|
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 and valuable architectural improvement with the PluginV2 interface, which decouples plugins from the registry and improves testability and standalone usage. The refactoring of the Anthropic plugin serves as a great example of the new pattern. The addition of factory functions for models, embedders, and retrievers is a clean way to support this new interface.
However, there are a few critical and medium-severity issues that should be addressed. The most critical is the incorrect use of asyncio.run in an async context, which will lead to runtime errors. There are also opportunities to improve encapsulation in the plugin registration logic and a minor regression in the Anthropic plugin's configuration options. Addressing these points will make the new infrastructure more robust and maintainable.
| if inspect.iscoroutinefunction(plugin.init): | ||
| resolved_actions = asyncio.run(plugin.init()) | ||
| else: |
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.
asyncio.run() will raise a RuntimeError if called from within a running event loop. Since this code is in an async-focused module, it's likely that Genkit() will be instantiated within an async function, which would cause the application to crash. This same issue exists for plugin.resolve on line 134.
The initialization logic for V2 plugins needs to be async-aware. The initialization process should be made asynchronous. A common pattern is to have a lightweight __init__ and a separate async def _async_init(self) method that performs the actual async setup. Other methods like generate would then need to ensure _async_init has been called. This avoids blocking the event loop and prevents runtime errors.
| if action.name.startswith(f"{plugin.name}/"): | ||
| namespaced_name = action.name | ||
| else: | ||
| namespaced_name = f"{plugin.name}/{action.name}" | ||
| action._name = namespaced_name | ||
|
|
||
| # Register the action directly in the registry's entries | ||
| # (Don't use register_action() as that would create a new Action and double-wrap) | ||
| with self.registry._lock: | ||
| if action.kind not in self.registry._entries: | ||
| self.registry._entries[action.kind] = {} | ||
| self.registry._entries[action.kind][namespaced_name] = action |
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 method directly accesses and modifies private members of both the Action object (action._name) and the GenkitRegistry object (self.registry._lock, self.registry._entries). This breaks encapsulation, making the code more brittle and harder to maintain. This issue is also present in _base_async.py.
To improve encapsulation, I suggest adding a public method to GenkitRegistry for registering a pre-constructed Action object, for example add_action(self, action: Action). For re-namespacing, the Action class could have a method to return a new instance with a modified name (e.g., action.with_name(new_name)) to promote immutability and avoid direct mutation of private attributes.
| def __init__( | ||
| self, | ||
| models: list[str] | None = None, | ||
| **anthropic_params: str, | ||
| ) -> None: |
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 refactored __init__ method no longer accepts a models parameter. The previous implementation allowed users to specify a list of model names for eager initialization. The new init method now unconditionally initializes all supported Anthropic models. This is a regression in functionality, as users can no longer control which models are eagerly loaded to manage memory usage or startup time.
def __init__(
self,
models: list[str] | None = None,
**anthropic_params: str,
) -> None: