feat(py): Refactored Plugins API to follow generic `Plugin` interface. #1950

kirgrim · 2025-02-12T15:52:33Z

Description

Refactored Plugin API to use generic genkit.core.plugin_abc.Plugin interface
Updated VertexAI implementation to follow the new specification

Checklist (if applicable):

PR title is following https://www.conventionalcommits.org/en/v1.0.0/
Tested (manually, unit tested, etc.)
Docs updated (updated docs or a docs bug required)

py/packages/genkit/src/genkit/core/plugin_abc.py

kamilkorski · 2025-02-12T16:56:16Z

py/packages/genkit/src/genkit/core/plugin_abc.py

+
+from genkit.core.schemas import GenerateRequest, GenerateResponse
+
+if typing.TYPE_CHECKING:


Why? just import the genkit

Why? just import the genkit

circular import issue: we import plugin into the genkit veneer for initializaion, here we need it only for type annotation

py/packages/genkit/src/genkit/core/plugin_abc.py

Irillit · 2025-02-13T08:23:17Z

py/packages/genkit/src/genkit/core/plugin_abc.py

+        self, veneer: Genkit, name: str, metadata: dict | None = None
+    ) -> None:
+        """
+        Generic method for defining arbitrary plugin's model


Please, change the description. It is no longer generic.

Please use Google style docstrings: https://google.github.io/styleguide/pyguide.html#381-docstrings

Please use Google style docstrings: https://google.github.io/styleguide/pyguide.html#381-docstrings

updated according to the guidelines

Irillit · 2025-02-13T09:17:20Z

py/packages/genkit/src/genkit/veneer/veneer.py

            for plugin in plugins:
-                plugin(self)
+                plugin.attach_to_veneer(veneer=self)


It looks like you add all the models here. I wonder, if attach_to_veneer() should accept Genkit here.You can attach it:

To registry inside the attach_to_veneer()

Just return values. We'll have the dictionary of the SUPPORTED_MODELS with ModelInfo, and, if we need, a Callable.

It looks like you add all the models here. I wonder, if attach_to_veneer() should accept Genkit here.You can attach it:

To registry inside the attach_to_veneer()

Just return values. We'll have the dictionary of the SUPPORTED_MODELS with ModelInfo, and, if we need, a Callable.

do not really support this approach as we have a different ways of defining models/embedders/etc based on plugin

it is not only models, also embedders and logic is not always that trivial according to JS implementations

Irillit · 2025-02-13T09:38:04Z

py/packages/genkit/src/genkit/veneer/veneer.py

@@ -66,9 +65,9 @@ def delete_runtime_file() -> None:
            self.thread = threading.Thread(target=self.start_server)
            self.thread.start()

-        if plugins is not None:
+        if plugins:


I think, there should be an error, if there are no plugins. Genkit becomes useless.

I think, there should be an error, if there are no plugins. Genkit becomes useless.

I do not think that we need to raise an error in such case: we do not know how developers are going to use our library and maybe they would just use server and insert models/plugins later on.

Although I would add warning in case no plugins are provided

Irillit · 2025-02-13T09:52:10Z

py/packages/genkit/src/genkit/veneer/veneer.py

 from genkit.core.reflection import make_reflection_server
 from genkit.core.registry import Registry
 from genkit.core.schemas import GenerateRequest, GenerateResponse, Message

-Plugin = Callable[['Genkit'], None]
-

 class Genkit:


In init of that class we also have a 'model' paramter, that is added as model field and never used. At the same time attach_to_veneer registers all the models. Do we have a special reason of keeping it? Should we filter it or pass it to some method?

In init of that class we also have a 'model' paramter, that is added as model field and never used. At the same time attach_to_veneer registers all the models. Do we have a special reason of keeping it? Should we filter it or pass it to some method?

Not sure, but would not update interface of this class as a part of this PR

Ok, I found it later. It seems that it looks for a model.

Irillit · 2025-02-13T12:02:53Z

py/packages/genkit/src/genkit/core/plugin_abc.py

+        )
+
+    @abc.abstractmethod
+    def _model_callback(self, request: GenerateRequest) -> GenerateResponse:


I wonder, if here we have only one callback. One provider has several models at once.

I wonder, if here we have only one callback. One provider has several models at once.

afaik, as a rule, each single callback receives GenerateRequest and returns GenerateResponse I would prefer not to complicate things and handle any special cases separately in the future

Well, look. VertexAI should support the following models (from the JS code):

export const SUPPORTED_V1_MODELS = { 'gemini-1.0-pro': gemini10Pro, }; export const SUPPORTED_V15_MODELS = { 'gemini-1.5-pro': gemini15Pro, 'gemini-1.5-flash': gemini15Flash, 'gemini-2.0-flash-001': gemini20Flash001, 'gemini-2.0-flash-lite-preview-02-05': gemini20FlashLitePreview0205, 'gemini-2.0-pro-exp-02-05': gemini20ProExp0205, };

Let me check, but probably they should all be in a registry in order you want to call them.

Well, look. VertexAI should support the following models (from the JS code):

export const SUPPORTED_V1_MODELS = { 'gemini-1.0-pro': gemini10Pro, }; export const SUPPORTED_V15_MODELS = { 'gemini-1.5-pro': gemini15Pro, 'gemini-1.5-flash': gemini15Flash, 'gemini-2.0-flash-001': gemini20Flash001, 'gemini-2.0-flash-lite-preview-02-05': gemini20FlashLitePreview0205, 'gemini-2.0-pro-exp-02-05': gemini20ProExp0205, };

Let me check, but probably they should all be in a registry in order you want to call them.

yes, definitely, but implementation of the production-ready VertexAI is a separate task. Here I just introduced interface and made our minimal working sample work

Irillit · 2025-02-13T15:23:10Z

py/packages/genkit/src/genkit/veneer/veneer.py

 from genkit.core.reflection import make_reflection_server
 from genkit.core.registry import Registry
 from genkit.core.schemas import GenerateRequest, GenerateResponse, Message

-Plugin = Callable[['Genkit'], None]
+logger = logging.getLogger(__name__)


 class Genkit:


I also heard Kamil proposed to use an interface for Genkit. If we do a separate AsyncGenkit veneer, that means that the interface might be a good idea.

I also heard Kamil proposed to use an interface for Genkit. If we do a separate AsyncGenkit veneer, that means that the interface might be a good idea.

yeah, it might, but also would keep it as a separate effort from this PR

yesudeep · 2025-02-13T20:06:29Z

Can you please rebase this PR? I think the PR checks should pass now

kirgrim requested review from yesudeep and Irillit February 12, 2025 15:52

github-actions bot added feature New feature or request python Python labels Feb 12, 2025

Irillit reviewed Feb 12, 2025

View reviewed changes

py/packages/genkit/src/genkit/core/plugin_abc.py Show resolved Hide resolved

kamilkorski reviewed Feb 12, 2025

View reviewed changes

py/packages/genkit/src/genkit/core/plugin_abc.py Outdated Show resolved Hide resolved

kirgrim requested a review from kamilkorski February 12, 2025 17:00

kirgrim force-pushed the hrymailo/feat/moving-py-genkit-plugins-to-classes branch from d5d1c5b to 6c20f99 Compare February 12, 2025 17:02

kirgrim requested a review from Irillit February 12, 2025 17:03

kirgrim force-pushed the hrymailo/feat/moving-py-genkit-plugins-to-classes branch from 6c20f99 to f0c4bb7 Compare February 12, 2025 17:14

Irillit reviewed Feb 12, 2025

View reviewed changes

py/packages/genkit/src/genkit/core/plugin_abc.py Show resolved Hide resolved

Irillit reviewed Feb 13, 2025

View reviewed changes

kirgrim force-pushed the hrymailo/feat/moving-py-genkit-plugins-to-classes branch from f0c4bb7 to 168cad7 Compare February 13, 2025 15:14

feat(py): Refactored Plugins API to follow generic Plugin interface.

be033a4

kirgrim force-pushed the hrymailo/feat/moving-py-genkit-plugins-to-classes branch from 168cad7 to be033a4 Compare February 13, 2025 15:17

Irillit reviewed Feb 13, 2025

View reviewed changes

kirgrim requested a review from Irillit February 13, 2025 15:25

yesudeep added this to the py-0.1.0 milestone Feb 13, 2025

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat(py): Refactored Plugins API to follow generic `Plugin` interface. #1950

feat(py): Refactored Plugins API to follow generic `Plugin` interface. #1950

kirgrim commented Feb 12, 2025

kamilkorski Feb 12, 2025

kirgrim Feb 12, 2025

Irillit Feb 13, 2025

yesudeep Feb 13, 2025

kirgrim Feb 13, 2025

Irillit Feb 13, 2025

kirgrim Feb 13, 2025

Irillit Feb 13, 2025

kirgrim Feb 13, 2025

Irillit Feb 13, 2025

kirgrim Feb 13, 2025

Irillit Feb 13, 2025

Irillit Feb 13, 2025

kirgrim Feb 13, 2025 •

edited

Loading

Irillit Feb 13, 2025

kirgrim Feb 13, 2025

Irillit Feb 13, 2025

kirgrim Feb 13, 2025

yesudeep commented Feb 13, 2025


		from genkit.core.schemas import GenerateRequest, GenerateResponse

		if typing.TYPE_CHECKING:

feat(py): Refactored Plugins API to follow generic Plugin interface. #1950

Are you sure you want to change the base?

feat(py): Refactored Plugins API to follow generic Plugin interface. #1950

Conversation

kirgrim commented Feb 12, 2025

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

kirgrim Feb 13, 2025 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

yesudeep commented Feb 13, 2025

feat(py): Refactored Plugins API to follow generic `Plugin` interface. #1950

feat(py): Refactored Plugins API to follow generic `Plugin` interface. #1950

kirgrim Feb 13, 2025 •

edited

Loading