-
Notifications
You must be signed in to change notification settings - Fork 51
refactor: plugin system updates #168
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
Merged
Merged
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
mikeknep
commented
Dec 31, 2025
29aa3b6 to
f4c501e
Compare
mikeknep
commented
Dec 31, 2025
b92472e to
21d80a3
Compare
johnnygreco
reviewed
Jan 5, 2026
johnnygreco
reviewed
Jan 5, 2026
johnnygreco
reviewed
Jan 5, 2026
johnnygreco
reviewed
Jan 5, 2026
johnnygreco
reviewed
Jan 5, 2026
4a2bdac to
63487fd
Compare
f67f352 to
54206bf
Compare
johnnygreco
approved these changes
Jan 6, 2026
Contributor
johnnygreco
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.
🛸
Thanks for this @mikeknep!
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
This PR updates the plugin system. Development notes are captured below.
Requirements
engine-related implementation object (ConfigurableTask,ColumnGenerator, etc.).Pluginobject that gets referenced in a single entry point.configmodule depend on anyenginemodule.a. Breaks "slim install" support, because
enginecode may include third-party deps that aconfig-only slim install will not include.b. Introduces a high risk of circular imports, because
enginecode depends onconfigmodules.Current state
The current plugin system violates REQ 3 (and by extension REQ 4):
(
->means "imports" aka "depends on")Updates here
Make the
Pluginobject "lazy" by defining the config and and task types as fully-qualified strings rather than objects.By using strings in the
Pluginobject fields, if the plugin is structured with multiple files (e.g.config.pyandtask.py)*, then the core library'sconfigcode that uses plugins (to extend discriminated union types) can load the plugin and resolve only the config class type; it would not need to resolve/load/import the plugin's task-related module whereenginebase classes are imported and subclassed.Example:
Strings instead of concrete types?
Yeah, a little sad, but seems a reasonable compromise given the benefits this unlocks.
We can add a Pydantic validator to verify that the referenced module does exist and that there is a class with the given name in that module.
For one step further, we ship a test helper function that we'd encourage plugin authors use in their unit tests:
(Similar to
pd.testing.assert_frame_equal.)To start, that test helper would ensure two things:
In the future, we could extend the helper to validate other things that are more complex than just Pydantic field type validations.
Remember: we can't implement this validation as a Pydantic validator because it would break the laziness. We can at least validate that the module exists (and this branch does so), but only the test helper can go further and actually fully resolve the two fields.
Plugin development lifecycle
A plugin author could continue defining everything in one Python file and things would still work in the library. The limitation would be that a plugin defined that way would not support slim installs, and so clients like NMP would not be able to use it. This might be perfectly fine for many plugins, especially in the early going. A reasonable "plugin development lifecycle" might be:
Plugin authors would only need to do step 2 if/when we want to make the plugin available to a client that requires a slim install (NMP, possibly others). That step 2 refactor would involve breaking the plugin implementation up into multiple files and (if necessary) making sure any heavyweight, task-only third party dependencies are included under an
engineextra.