-
Notifications
You must be signed in to change notification settings - Fork 158
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
fix(py): Removing circular dependencies on Genkit in Plugin API #2041
base: main
Are you sure you want to change the base?
fix(py): Removing circular dependencies on Genkit in Plugin API #2041
Conversation
95e3251
to
ecdc389
Compare
Args: | ||
veneer: requested `genkit.veneer.Genkit` instance | ||
Entrypoint for initializing the plugin instance in Genkit | ||
Returns: | ||
None | ||
""" | ||
pass |
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.
Here I'm thinking about implementing the initialization here, but leaving _register_model() empty. I would create abstract methods for:
- register_models()
- register_indexers()
- register_retrievers()
- register_embedders()
and etc
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.
Let me try to create the PR with how I see it as well.
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.
Here I'm thinking about implementing the initialization here, but leaving _register_model() empty. I would create abstract methods for:
- register_models()
- register_indexers()
- register_retrievers()
- register_embedders()
and etc
Idea of making register_model singular is in abstracting a logic of registry attachment. We attach to registry per each model, thus it is still required. But it may make sense to define register_models, although i am afraid not every plugin has multiple models
) | ||
cls.register_action(action=action) | ||
|
||
@classmethod |
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.
What is the rationale behind using a classmethod here?
Each Genkit instance composes its own registry instance no?
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.
What is the rationale behind using a classmethod here?
Each Genkit instance composes its own registry instance no?
no, it is defined on the class-level
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.
And should we have more than one Genkit class?
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.
And should we have more than one Genkit class?
makes no sense - polymorphic behaviour will be granted by the plugins api
ecdc389
to
92e3b7d
Compare
…pendencies on Genkit in Plugin API
92e3b7d
to
508cd50
Compare
@pavelgj please take a look, it seems like we may remove this circular dependency as there is single registry per class. Is it intended/allowed/makes sense to have multiple instances of Genkit veneer instantiated during the runtime? |
Description:
Registry
Checklist (if applicable):