fix: guarding optional imports for hooks#627
Conversation
Signed-off-by: Paul S. Schweigert <paul@paulschweigert.com>
|
The PR description has been updated. Please fill out the template for your PR to be reviewed. |
Merge ProtectionsYour pull request matches the following merge protections and will not be merged until they are valid. 🟢 Enforce conventional commitWonderful, this rule succeeded.Make sure that we follow https://www.conventionalcommits.org/en/v1.0.0/
|
| except ImportError: | ||
| from pydantic import BaseModel as _BaseModel | ||
|
|
||
| class PluginPayload(_BaseModel): # type: ignore[no-redef] |
There was a problem hiding this comment.
I would add a log warning here in case someone is trying to define an instance of MelleaBasePayload without cpex installed (and without realizing it isn't installed), and they get unexpected behavior.
There was a problem hiding this comment.
I'm in favor of implementing this the same as the other cpex dependent classes are when cpex isn't installed: https://github.com/generative-computing/mellea/blob/main/mellea/plugins/base.py#L153-L243
We should stub the class and have it raise an import error.
There was a problem hiding this comment.
Don't we need to update this to pip install 'mellea[hooks]'?
There was a problem hiding this comment.
Yes, we should do a find and replace. There's a couple of spots where this is contextforge instead of cpex. Good catch!
|
This looks reasonable to me. I submitted a PR to add the optional log warning and fix the messages ImportError messages. I'll see if @araujof has any other suggestions. Only other (potential) issue I saw is if something tries to call |
| except ImportError: | ||
| from pydantic import BaseModel as _BaseModel | ||
|
|
||
| class PluginPayload(_BaseModel): # type: ignore[no-redef] |
There was a problem hiding this comment.
I'm in favor of implementing this the same as the other cpex dependent classes are when cpex isn't installed: https://github.com/generative-computing/mellea/blob/main/mellea/plugins/base.py#L153-L243
We should stub the class and have it raise an import error.
There was a problem hiding this comment.
Yes, we should do a find and replace. There's a couple of spots where this is contextforge instead of cpex. Good catch!
|
|
||
| class _MethodHookAdapter(Plugin): | ||
| """Adapts a single ``@hook``-decorated bound method from a ``Plugin`` class. | ||
| if _HAS_PLUGIN_FRAMEWORK: |
There was a problem hiding this comment.
Lets do the stubs with import errors for these two classes as well.
|
@psschwei, I pushed the changes mentioned in my review. Please feel free to disagree and revert my commit. I ran tests with and without cpex and everything passed. |
|
works for me |
9588284
* fix: guarding optional imports for hooks Signed-off-by: Paul S. Schweigert <paul@paulschweigert.com> * fix: issues with mellea when hooks not installed --------- Signed-off-by: Paul S. Schweigert <paul@paulschweigert.com> Co-authored-by: Jake LoRocco <jake.lorocco@ibm.com>
3 participants
Misc PR
Type of PR
Description
The cpex package (cpex.framework) provides the ContextForge plugin framework — base classes (Plugin, PluginPayload, PluginResult) and infrastructure (hook registry, payload policies) that Mellea's plugin system builds on. It's an optional dependency.
The bug: two files defined classes that inherit from cpex types at module level, outside the if _HAS_PLUGIN_FRAMEWORK: guard:
When cpex isn't installed, those names don't exist, so import mellea raises NameError and the entire package is broken. The fix was to provide a frozen pydantic.BaseModel fallback for PluginPayload and wrap the two adapter classes inside the existing if _HAS_PLUGIN_FRAMEWORK: guard (they're only ever instantiated through already-guarded code paths).
Testing