[AIRFLOW-6128] Simplify plugins_manager and webserver plugin code #6696

potiuk · 2019-11-30T19:14:01Z

Make sure you have checked all steps below.

Jira

My PR addresses the following Airflow Jira issues and references them in the PR title. For example, "[AIRFLOW-XXX] My Airflow PR"
- https://issues.apache.org/jira/browse/AIRFLOW-6128

Description

Here are some details about my PR, including screenshots of any UI changes:

While adding type annotations, it was apparent that the code for
plugin manager and webserver initialisation was rather convoluted
and complex. This change fixes it as follow up after untangling
cyclic dependnencies.

As a side effect - it turned out that the test plugin view
in airflow RBAC implementation was not working since 1.10.1 - it threw "render
is not an attribute" error - this has been fixed in this commit
as well. The Plugins/TestView now works as expected. See
https://stackoverflow.com/questions/53701030/airflow-plugins-rbac-enabled-blueprint-not-working?answertab=active#tab-top

Tests

My PR adds the following unit tests OR does not need testing for this extremely good reason:

Commits

My commits all reference Jira issues in their subject lines, and I have squashed multiple commits if they address the same issue. In addition, my commits follow the guidelines from "How to write a good git commit message":
1. Subject is separated from body by a blank line
2. Subject is limited to 50 characters (not including Jira issue reference)
3. Subject does not end with a period
4. Subject uses the imperative mood ("add", not "adding")
5. Body wraps at 72 characters
6. Body explains "what" and "why", not "how"

Documentation

In case of new functionality, my PR adds documentation that describes how to use it.
- All the public functions and the classes in the PR contain docstrings that explain what it does
- If you implement backwards incompatible changes, please leave a note in the Updating.md so we can assign it to a appropriate release

potiuk · 2019-11-30T19:25:08Z

While working on adding more type annotations I've found more cyclic dependencies in Plugins manager. I managed to untangle them in this commit as a follow-up after the "executors" untangling.

As result - we have much smaller airflow.init.py, and the dependencies are much more straightforward and not cyclic:

airflow -> PluginsManager -> [operators/hooks/macros/sensors]

rather than

airflow -> [operators/hooks/macros/sensors]  -> PluginsManager -> [operators/hooks/macros/sensors]

I also made most of the affected files pylint + mypy compliant.

airflow/config_templates/airflow_local_settings.py

potiuk · 2019-11-30T23:36:40Z

Yeh. I actually used type-hints and mypy to verify if my changes are correct (except running the airflow). It is super-helpful to be confident about your changes while commiting them. I am using pre-commit extensively to make sure I do not make mistakes. It's a big value of pylint/mypy/flake that I am using.

I will split out typehint changes as the last step of review. Similarly as I do for #6596 . But this has to come as next steps. First I use mypy/pylint to get my changes verified in automated way and then I can split them out to make sure they are change/bisect friendly.

potiuk · 2019-12-01T16:06:48Z

@ashb-> separated out type adding to #6140. Now what we are left with is a change that internally splits-out plugins_manager and webserver initialisation to be easier to understand - includes type annotations and fix to the long-standing problem of TestView bleupring not working since 1.10.1 https://stackoverflow.com/questions/53701030/airflow-plugins-rbac-enabled-blueprint-not-working?answertab=active#tab-top

codecov-io · 2019-12-02T00:50:53Z

Codecov Report

❗ No coverage uploaded for pull request base (master@da088b3). Click here to learn what that means.
The diff coverage is 89.08%.

@@            Coverage Diff            @@
##             master    #6696   +/-   ##
=========================================
  Coverage          ?   83.93%           
=========================================
  Files             ?      668           
  Lines             ?    37747           
  Branches          ?        0           
=========================================
  Hits              ?    31683           
  Misses            ?     6064           
  Partials          ?        0

Impacted Files	Coverage Δ
airflow/www/views.py	`76.77% <ø> (ø)`
airflow/www/security.py	`91.5% <100%> (ø)`
airflow/plugins_manager.py	`91.09% <84.21%> (ø)`
airflow/www/app.py	`94.11% <92.78%> (ø)`

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update da088b3...2b5839e. Read the comment docs.

potiuk · 2019-12-04T09:57:43Z

@ashb, @kaxil @feluelle @Fokko @mik-laj -> another spin-off after refactoring of executors. This time it's Plugins manager (look at the last commit only - I separated out adding type annotation to previous commit).

While adding type annotations, it was apparent that the code for plugin manager and webserver initialisation was rather convoluted and complex. This change fixes it as follow up after untangling cyclic dependnencies. As a side effect - it turned out that the test plugin view in airflow RBAC implementation was not working since 1.10.1 - it threw "render is not an attribute" error - this has been fixed in this commit as well. The Plugins/TestView now works as expected. See https://stackoverflow.com/questions/53701030/airflow-plugins-rbac-enabled-blueprint-not-working?answertab=active#tab-top

feluelle · 2020-01-06T15:56:42Z

airflow/plugins_manager.py

-            if file_ext != '.py':
-                continue
+def import_plugin(filepath: str, mod_name: str, root: str) -> None:
+    """Imports plugin module."""


Don't you think a full documentation is needed here?

feluelle · 2020-01-06T15:56:49Z

airflow/plugins_manager.py

-            log.error('Failed to import plugin %s', path)
-            import_errors[path] = str(e)
+def try_to_import_plugin(filepath: str, root: str) -> None:
+    """Tries to import plugin from the file specified. Skips it if not a valid python file."""


feluelle · 2020-01-06T15:57:54Z

airflow/plugins_manager.py

+
+def find_airflow_plugins() -> None:
+    """Crawl through the plugins folder to find AirflowPlugin derivatives"""
+    assert settings.PLUGINS_FOLDER, "Plugins folder is not set"


I thought we did decide to not use assert for raising exceptions.

feluelle · 2020-01-06T15:59:36Z

airflow/plugins_manager.py

+def get_stat_name_handler() -> Optional[Callable[[str], str]]:
+    """Retrieves the single configured stats_handler (there can be only one!)"""
+    assert len(stat_name_handlers) in {0, 1}, \
+        f'Specified more than one stat_name_handler ({stat_name_handlers}) which is not allowed.'


I thought we did decide to not use assert for raising exceptions.

Same here.

feluelle · 2020-01-06T16:00:58Z

airflow/plugins_manager.py

-        'Specified more than one stat_name_handler ({}) '
-        'is not allowed.'.format(stat_name_handlers))
+def get_stat_name_handler() -> Optional[Callable[[str], str]]:
+    """Retrieves the single configured stats_handler (there can be only one!)"""


Can you add the documentation for the return value, please?

feluelle · 2020-01-06T16:05:31Z

airflow/www/app.py

-
-    return app, appbuilder
+def configure_app(app_name: str, config: Optional[Dict[str, Union[str, bool]]], testing: bool):
+    """Configure the application."""


Please add a description to the parameters.

feluelle · 2020-01-06T16:05:50Z

airflow/www/app.py


-def root_app(env, resp):
+def root_app(_, resp: Callable[[bytes, List[Tuple[str, str]]], None]):
+    """Root application function - returns 404 if the path is not found."""


feluelle · 2020-01-06T16:06:12Z

airflow/www/app.py

+def cached_app(config: Optional[Dict[str, Union[str, bool]]] = None,
+               session: Optional[Session] = None,
+               testing: bool = False) -> Flask:
+    """Returns cached application if it's there. Creates it otherwise."""


feluelle · 2020-01-06T16:06:32Z

airflow/www/app.py

-    global appbuilder
+def cached_appbuilder(config: Optional[Dict[str, Union[str, bool]]] = None, testing=False) -> \
+        AppBuilder:
+    """Returns cached appbuilder."""


potiuk · 2020-01-12T06:57:59Z

Thanks @feluelle ! I will come back to it soon-ish (or late-ish maybe)

potiuk · 2020-02-17T07:29:43Z

Same story - I think I will close that one. Ane refactoring/removals of cyclic dependencies etc. will be far easier when we move things outside of the airflow.init.py

potiuk requested review from ashb, feluelle, kaxil and mik-laj November 30, 2019 19:14

ashb reviewed Nov 30, 2019

View reviewed changes

airflow/config_templates/airflow_local_settings.py Outdated Show resolved Hide resolved

potiuk force-pushed the unentangle-plugins-manager branch from 27a8c79 to 8467268 Compare December 1, 2019 16:01

potiuk changed the title ~~[AIRFLOW-6128] Untangle plugins_manager and web from cyclic dependencies. Based on [AIRFLOW-6004]~~ [AIRFLOW-6128] Untangle plugins_manager and web from cyclic dependencies. Based on [AIRFLOW-6004] [AIRFLOW-6140] Dec 1, 2019

potiuk force-pushed the unentangle-plugins-manager branch 2 times, most recently from 48c1c7f to 7ef059f Compare December 1, 2019 18:57

potiuk changed the title ~~[AIRFLOW-6128] Untangle plugins_manager and web from cyclic dependencies. Based on [AIRFLOW-6004] [AIRFLOW-6140]~~ [AIRFLOW-6128] Simplify plugins_manager and webserver plugin code. Based on [AIRFLOW-6004] [AIRFLOW-6140] Dec 1, 2019

potiuk mentioned this pull request Dec 1, 2019

[AIRFLOW-6005] Common classes should always be imported from airflow, Depends on [AIRFLOW-6128] #6597

Closed

5 tasks

potiuk force-pushed the unentangle-plugins-manager branch 3 times, most recently from 2598055 to 8598720 Compare December 1, 2019 22:32

potiuk force-pushed the unentangle-plugins-manager branch 3 times, most recently from d400eb1 to edd9730 Compare December 3, 2019 21:25

potiuk changed the title ~~[AIRFLOW-6128] Simplify plugins_manager and webserver plugin code. Based on [AIRFLOW-6004] [AIRFLOW-6140]~~ [AIRFLOW-6128] Simplify plugins_manager and webserver plugin code. [AIRFLOW-6140] Dec 4, 2019

potiuk changed the title ~~[AIRFLOW-6128] Simplify plugins_manager and webserver plugin code. [AIRFLOW-6140]~~ [AIRFLOW-6128] Simplify plugins_manager and webserver plugin code. Depends on [AIRFLOW-6140] Dec 4, 2019

potiuk force-pushed the unentangle-plugins-manager branch from edd9730 to 0eea374 Compare December 4, 2019 22:02

potiuk changed the title ~~[AIRFLOW-6128] Simplify plugins_manager and webserver plugin code. Depends on [AIRFLOW-6140]~~ [AIRFLOW-6128] Simplify plugins_manager and webserver plugin code Dec 4, 2019

potiuk force-pushed the unentangle-plugins-manager branch from 0eea374 to 2b5839e Compare December 9, 2019 11:24

github-actions bot added the area:webserver Webserver related Issues label Jan 5, 2020

feluelle reviewed Jan 6, 2020

View reviewed changes

potiuk closed this Feb 17, 2020

potiuk mentioned this pull request Feb 21, 2020

[AIRFLOW-6817] - Move airflow __init__.py imports to sub-packages #7456

Merged

6 tasks

[AIRFLOW-6128] Simplify plugins_manager and webserver plugin code #6696

[AIRFLOW-6128] Simplify plugins_manager and webserver plugin code #6696

Uh oh!

Conversation

potiuk commented Nov 30, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Jira

Description

Tests

Commits

Documentation

Uh oh!

potiuk commented Nov 30, 2019

Uh oh!

Uh oh!

potiuk commented Nov 30, 2019

Uh oh!

potiuk commented Dec 1, 2019

Uh oh!

codecov-io commented Dec 2, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

potiuk commented Dec 4, 2019

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

potiuk commented Jan 12, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

potiuk commented Feb 17, 2020

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

potiuk commented Nov 30, 2019 •

edited

Loading

codecov-io commented Dec 2, 2019 •

edited

Loading

potiuk commented Jan 12, 2020 •

edited

Loading