-
Notifications
You must be signed in to change notification settings - Fork 16.3k
Move over plugins manager to a shared library #59956
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
Move over plugins manager to a shared library #59956
Conversation
I think what we should do now is to define A bit more context why and what is my "North Star" here. I think eventually we should have two (or maybe even 3) types of plugins. I thought about it in the past when discussing the task isolation with @ashb and splitting the distributions, and I am quite sure we will have to do it sooner or later (and rather sooner) and have different distribution for "core" and "task.sdk" plugins - when things are different than regular "providers" (i.e. when we have provider-specific executors, listeners and macros). This is related to something we discussed yesterday with @kacpermuda in #59921 (comment) -> and it's clearly visible there because as of Airflow 3.2. openlineage is a bit in a Shroedinger state - it both needs, and does not need I don't think we are "ready" to discuss the exact way how to do the split - with naming and implementation details - (we need to complete core isolation from task.sdk first). But I think I have a very clear idea how it can work on high level (and would be the ultimate distribution split I had in mind when we started discussing task-isolation). My thinking is that we might have two types of distribution - current That would be very, very visible with edge provider - which (if we do all the split till the end) should have all 3 (!) types of distributions (cc: @jscheffl):
Similarly:
A little bit of problem there is that those "separate" distributions will necessarlily need to share some code. But this problem is largely solved now with That is my We do not have to decide on all that now - naming, folders, etc. can (and should be) discussed later when we complete the current isolation work. But we can do some preliminary work - i.e. start separating "types" of plugins.
Yes. |
shared/plugins_manager/src/airflow_shared/plugins_manager/plugins_manager.py
Show resolved
Hide resolved
jscheffl
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.
Looks good also in my view.
shared/plugins_manager/src/airflow_shared/plugins_manager/plugins_manager.py
Outdated
Show resolved
Hide resolved
That's a lot of good detail and very valid! I had similar thoughts and would probably want to have a plugins sdk some time in the future (I was discussing this with @uranusjr just a while earlier) and the motivation to do that was #59093. The thought was to have a plugin SDK that allows loading components into protected processes without serialization. But we can probably chat about that just a little later. @potiuk would you be OK to throw these thoughts you have into a issue for broader reach and formulation of some plan? Let me know if you would want me to take a look too |
I think it's a bit too early for that - see a discussion in devlist - this goes far beyond the current set of changes, and also it might change as we implement those "necessary" isolation changes now. I prefer to keep it here as a comment that I can find out later when we turn it into a plan for 3.3 - but I think that will be way better time and we will have way more "datapoints" to define the actual plan - and this should be preceded by a discussion where we will iterate over high level proposal first. So ... happy to do such a high-level proposal and turn it into a plan - but probabl;y closer to the time when we will be finalizing 3.2 and have all the task-isolation steps done. |
|
@potiuk sounds good, please keep me in the loop on the same :) |
jscheffl
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.
Second pass review looks good for me.
|
Thanks for review folks, merging this one. |
Move `import importlib` outside the `if load_examples:` block so it's available when loading plugins from the plugins directory. This is a regression introduced in apache#59956, causing plugins to fail to load.
Why
As of today, workers unfortunately need plugins for macros (template rendering), listeners, and operator extra links. Now, the plugins manager is present in core airflow and sdk code needs to have a cross package import for it.
What
I am trying to move the generic plugin management code to a shared library (
shared/plugins_manager) to support client-server separation betweenairflow-coreandtask-sdk. This ensures both components use the same plugin discovery and integration logic without code duplication which can happen if we just copy paste the entire code in both packages.Changes of note
What was moved into shared library
For example:
AirflowPluginbase classAirflowPluginSource,PluginsDirectorySource,EntryPointSource_load_plugins_from_plugin_directory(),_load_entrypoint_plugins()integrate_macros_plugins(),integrate_listener_plugins()is_valid_plugin(),make_module()The principle I used here was that we have to keep the library standalone and hence move no provider discovery, logic to get plugins (read from providers too), anything similar like this.
In order to do this, we maintain wrappers in core airflow and sdk now and the code within each of those use their own versions.
Some function signature changed for dependency injection
Some methods needed a change in signature to be able to inject library specific values to these functions.
Shared code couldn't import
airflow.settingsorairflow.configuration.conf, so:PluginsDirectorySource.__init__(self, path)(self, path, plugins_folder: str)_load_plugins_from_plugin_directory()(plugins_folder: str, load_examples: bool = False, example_plugins_module: str | None = None)integrate_macros_plugins()(macros_module: ModuleType, macros_module_name_prefix: str, plugins: list[AirflowPlugin])integrate_listener_plugins()(listener_manager: ListenerManager, plugins: list[AirflowPlugin])These values are injected via core airflow / task sdk wrappers
Providers
Added
AirflowPlugintocommon.compat.sdk. Added# use next versionto affected providers.TODO / Open Qns
from airflow.plugins_manageror move tofrom airflow.sdk.plugins_manager? I am doubtful because plugins can also be used for CORE only modifications like react_apps or fastapi_apps.^ Add meaningful description above
Read the Pull Request Guidelines for more information.
In case of fundamental code changes, an Airflow Improvement Proposal (AIP) is needed.
In case of a new dependency, check compliance with the ASF 3rd Party License Policy.
In case of backwards incompatible changes please leave a note in a newsfragment file, named
{pr_number}.significant.rstor{issue_number}.significant.rst, in airflow-core/newsfragments.