Skip to content

Conversation

@potiuk
Copy link
Member

@potiuk potiuk commented Sep 9, 2020

Here is a POC/WIP of dynamic discovery for Provider packages .

This is a WIP of master version of the dynamic discovery of packages from #11163 . It needs completion. Details of the work needed are described in https://cwiki.apache.org/confluence/pages/viewpage.action?pageId=100827303


^ Add meaningful description above

Read the Pull Request Guidelines for more information.
In case of fundamental code change, 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 UPDATING.md.

@boring-cyborg boring-cyborg bot added area:webserver Webserver related Issues provider:microsoft-azure Azure-related issues provider:google Google (including GCP) related issues labels Sep 9, 2020
@potiuk potiuk requested a review from turbaszek September 9, 2020 08:37
Comment on lines +62 to +64
onerror=ignore):
try:
if ispkg:
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
onerror=ignore):
try:
if ispkg:
onerror=ignore):
if not ispkg:
continue
try:

WDYT?

import pkgutil


def get_provider_info(conn_type_to_hook, connection_types):
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This feels similar to existing code in PluginsManager, and serves a similar purpose. What do you think about move this code in to there?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sure - it's really a POC which I prepared quickly - happy to move it there.

@potiuk
Copy link
Member Author

potiuk commented Nov 2, 2020

@kaxil - I missed this at the beginning of the call - this PR had "beta1" but I do not think it is actualy needed in beta1 (and this is a POC so I am going to re-implement it anyway in smaller pieces). I planned the work there for different betas to add those incrementally and split it into separate issues (all in https://github.com/apache/airflow/projects/5)

@ashb
Copy link
Member

ashb commented Nov 3, 2020

@potiuk I've been having a quick look at using https://pluggy.readthedocs.io/en/latest/ (the plugin system from pytest) for this -- I'll let you know my thoughts

@potiuk
Copy link
Member Author

potiuk commented Nov 3, 2020

@potiuk I've been having a quick look at using https://pluggy.readthedocs.io/en/latest/ (the plugin system from pytest) for this -- I'll let you know my thoughts

Interesting. I have not yet come back to it - busy with CI optimisations, but will come back to it this week, so happy to hear your thoughts. this was just POC to see that it is possible but if there are ready-to-use solutions, I am happy to use them instead

@ashb
Copy link
Member

ashb commented Nov 3, 2020

The main problem we we've run in to with the existing plugin mechanism (we ship some custom plugins in our Astronomer images) is that any conflicting deps means plugins are unloadable:

Traceback (most recent call last):
  File "/home/ash/code/airflow/airflow/airflow/plugins_manager.py", line 187, in load_entrypoint_plugins
    plugin_class = entry_point.load()
  File "/home/ash/.virtualenvs/airflow-with-plugins/lib/python3.7/site-packages/pkg_resources/__init__.py", line 2442, in load
    self.require(*args, **kwargs)
  File "/home/ash/.virtualenvs/airflow-with-plugins/lib/python3.7/site-packages/pkg_resources/__init__.py", line 2465, in require
    items = working_set.resolve(reqs, env, installer, extras=self.extras)
  File "/home/ash/.virtualenvs/airflow-with-plugins/lib/python3.7/site-packages/pkg_resources/__init__.py", line 791, in resolve
    raise VersionConflict(dist, req).with_context(dependent_req)
pkg_resources.VersionConflict: (lazy-object-proxy 1.2.2 (/home/ash/.virtualenvs/airflow-with-plugins/lib/python3.7/site-packages), Requirement.parse('lazy_object_proxy~=1.3'))

Given the odds of some version conflict in a non-trival airflow install is quite high, using our existing plugin mechanism for more doesn't seem like a great idea.

I haven't looked in detail yet at Pluggy, but somehow it doesn't suffer this same problem, even though it supports entrypoints.

Also at first though I quite like the idea of a richer plugin mechanism in Airflow.

@potiuk potiuk closed this Nov 16, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

AIP-8 - Provider packages area:webserver Webserver related Issues provider:google Google (including GCP) related issues provider:microsoft-azure Azure-related issues

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Automatically discover available packages

3 participants