You are viewing a plain text version of this content. The canonical link for it is here.
Posted to commits@airflow.apache.org by "mobuchowski (via GitHub)" <gi...@apache.org> on 2023/07/19 10:54:43 UTC
[GitHub] [airflow] mobuchowski opened a new pull request, #32692: plugins: load plugins from providers
mobuchowski opened a new pull request, #32692:
URL: https://github.com/apache/airflow/pull/32692
In some cases - like when provider is included by `breeze prod-image build --install-providers-from-sources` plugins are not loaded, as they are not separate package with entrypoint then. This PR adds loading plugins from `provider.yml`. I think this is somehow similar to what @potiuk is doing with stuff like config or executors.
In some cases this would make entrypoint plugins double-load - so we check if the plugin is already loaded. We could avoid that by not generating entrypoints for plugins, but this would make them not load on earlier versions of Airflow.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@airflow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] uranusjr commented on a diff in pull request #32692: plugins: load plugins from providers
Posted by "uranusjr (via GitHub)" <gi...@apache.org>.
uranusjr commented on code in PR #32692:
URL: https://github.com/apache/airflow/pull/32692#discussion_r1272122138
##########
airflow/plugins_manager.py:
##########
@@ -267,6 +274,27 @@ def load_plugins_from_plugin_directory():
import_errors[file_path] = str(e)
+def load_providers_plugins():
+ from airflow.providers_manager import ProvidersManager
+
+ global import_errors
Review Comment:
I don’t think this is needed?
##########
airflow/providers_manager.py:
##########
@@ -1024,6 +1039,20 @@ def _discover_config(self) -> None:
if provider.data.get("config"):
self._provider_configs[provider_package] = provider.data.get("config")
+ def _discover_plugins(self) -> None:
+ """Retrieve all plugins defined in the providers."""
+ for provider_package, provider in self._provider_dict.items():
+ if provider.data.get("plugins"):
+ for plugin_dict in provider.data["plugins"]:
+ if _correctness_check(provider_package, plugin_dict["plugin-class"], provider):
+ self._plugins_set.add(
+ PluginInfo(
+ name=plugin_dict["name"],
+ plugin_class=plugin_dict["plugin-class"],
+ provider_name=provider_package,
+ )
+ )
Review Comment:
```suggestion
for plugin_dict in provider.data.get("plugins", ()):
if not _correctness_check(provider_package, plugin_dict["plugin-class"], provider):
continue
self._plugins_set.add(
PluginInfo(
name=plugin_dict["name"],
plugin_class=plugin_dict["plugin-class"],
provider_name=provider_package,
)
)
```
Should we also add a log message when `_correctness_check` fails?
##########
airflow/plugins_manager.py:
##########
@@ -267,6 +274,27 @@ def load_plugins_from_plugin_directory():
import_errors[file_path] = str(e)
+def load_providers_plugins():
+ from airflow.providers_manager import ProvidersManager
+
+ global import_errors
Review Comment:
I don’t think this is needed?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@airflow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] potiuk commented on pull request #32692: plugins: load plugins from providers
Posted by "potiuk (via GitHub)" <gi...@apache.org>.
potiuk commented on PR #32692:
URL: https://github.com/apache/airflow/pull/32692#issuecomment-1642467950
> not generating entrypoints for plugins, but this would make them not load on earlier versions of Airflow
BTW. We can't do it even for the future - because when Airflow gets providers installed as packages, using entrypoints is the only mechanism how it can discover plugins, there are no provider.yaml files any more in packaged version of Airflow (and can't be because providers are isolated and independent from Airflow and there might me different versions of both - community and 3rd-party providers installed together with Airflow, so the only way Airflow can find out what plugins are availabe is by querying the entrypoints.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@airflow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] mobuchowski merged pull request #32692: plugins: load plugins from providers
Posted by "mobuchowski (via GitHub)" <gi...@apache.org>.
mobuchowski merged PR #32692:
URL: https://github.com/apache/airflow/pull/32692
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@airflow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] mobuchowski commented on a diff in pull request #32692: plugins: load plugins from providers
Posted by "mobuchowski (via GitHub)" <gi...@apache.org>.
mobuchowski commented on code in PR #32692:
URL: https://github.com/apache/airflow/pull/32692#discussion_r1272367426
##########
airflow/providers_manager.py:
##########
@@ -1024,6 +1039,20 @@ def _discover_config(self) -> None:
if provider.data.get("config"):
self._provider_configs[provider_package] = provider.data.get("config")
+ def _discover_plugins(self) -> None:
+ """Retrieve all plugins defined in the providers."""
+ for provider_package, provider in self._provider_dict.items():
+ if provider.data.get("plugins"):
+ for plugin_dict in provider.data["plugins"]:
+ if _correctness_check(provider_package, plugin_dict["plugin-class"], provider):
+ self._plugins_set.add(
+ PluginInfo(
+ name=plugin_dict["name"],
+ plugin_class=plugin_dict["plugin-class"],
+ provider_name=provider_package,
+ )
+ )
Review Comment:
Done. Also added log warning without details - `_correctness_check` logs the details itself.
##########
airflow/plugins_manager.py:
##########
@@ -267,6 +274,27 @@ def load_plugins_from_plugin_directory():
import_errors[file_path] = str(e)
+def load_providers_plugins():
+ from airflow.providers_manager import ProvidersManager
+
+ global import_errors
Review Comment:
Removed.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@airflow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] potiuk commented on pull request #32692: plugins: load plugins from providers
Posted by "potiuk (via GitHub)" <gi...@apache.org>.
potiuk commented on PR #32692:
URL: https://github.com/apache/airflow/pull/32692#issuecomment-1642463867
Oh yeah. Good point. I briefly looked at it - looks like the right direction - I think there is a bit of a problem of where the plugins are loaded from during tests as some tests expect to load plugins from "example_dags/plugins" and I think this PR removes that path somehow,
It's going to be much clearer and more straightforward after #32669 will be merged, because one of the things it does is that it unifies the unit test (local venv) / and CI environment for tesst so that it is configured in one place only and dynamically loaded via pytest fixture from here:
https://github.com/apache/airflow/pull/32669/files#diff-1748727a9777b78ae8c17034fa1c5d4a4fd38de8ce986ef13e6531b347f65588R41
So it might be a wise thing to wait after #32669 is merged, rebase and try to fix it there (happy to help/brainstorm - I got quite a bit more familiar with how unit test configuration is loaded by implementing the PRs)
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@airflow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org