[AIRFLOW-6817] Lazy-load `airflow.DAG` to keep user-facing API untouched #7517

ashb · 2020-02-23T22:50:36Z

from airflow import DAG is such a common pattern that it's worth
making sure this stays working.

Since we are now on Python 3 we can use PEP-562 to have officially
supported lazy-loading of attributes on a module.

Since the example_dags will be referred to/copied by users I have
updated them all back to import from the airflow module, but have left
any internal uses untouched.

This approach taken from Celery: https://github.com/celery/celery/blob/f2ddd894c32f642a20f03b805b97e460f4fb3b4f/celery/__init__.py#L63-L78

Issue link: AIRFLOW-6817

Make sure to mark the boxes below before creating PR: [x]

Description above provides context of the change
Commit message/PR title starts with [AIRFLOW-NNNN]. AIRFLOW-NNNN = JIRA ID^*
Unit tests coverage for changes (not needed for documentation changes)
Commits follow "How to write a good git commit message"
Relevant documentation is updated including usage instructions.
I will engage committers as explained in Contribution Workflow Example.

^* For document-only changes commit message can start with [AIRFLOW-XXXX].

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.
Read the Pull Request Guidelines for more information.

ashb · 2020-02-23T22:57:43Z

Probably worth some people double checking this in their IDE of choice -- I did install VSCode and PyCharm Community Edition for this, but I am not an IDE user.

ashb · 2020-02-23T22:58:39Z

~~Oh, can remove a line from updating.md now~~

`from airflow import DAG` is _such_ a common pattern that it's worth making sure this stays working. Since we are now on Python 3 we can use PEP-562 to have officially supported lazy-loading of attributes on a module. Since the example_dags will be referred to/copied by users I have updated them all back to import from the airflow module, but have left any internal uses untouched

kaxil · 2020-02-24T00:00:23Z

Travis failure:

____________ ERROR collecting tests/api/client/test_local_client.py ____________
ImportError while importing test module '/opt/airflow/tests/api/client/test_local_client.py'.
Hint: make sure your test modules/packages have valid Python names.
Traceback:
tests/api/client/test_local_client.py:26: in <module>
    from airflow.example_dags import example_bash_operator
airflow/example_dags/example_bash_operator.py:23: in <module>
    from airflow import DAG
E   ImportError: cannot import name 'DAG'

potiuk · 2020-02-24T07:24:12Z

Fantastic! Works like a charm in both IntelliJ and VSCode! I think we need to just make pytest understand it as well. I will also add later some more protection against importing airflow.DAG in other places than example_dags. Also I have already idea how we can significantly speed up Pylint for local pre-commits (bringing back parallelism without risking cyclic dependencies).

potiuk · 2020-02-24T07:24:49Z

airflow/__init__.py

+def __getattr__(name):
+    # PEP-562: Lazy loaded attributes on python modules
+    if name == "DAG":
+        from airflow.models.dag import DAG # pylint: disable=redefined-outer-name


I think we also need to disable cyclic dependency pylint check as well (cyclic-import). I think disabling it in the first line of the file (just above the licence) will do the trick.

Worth to try but I have a feeling that this has to be disabled in all places where the cycle occurs.

Given this isn't at the top level does pylint still detect it as a cycle?

Let's see and disable it if we find we need it. I am not 100% sure -> the algorithm is quite a bit different than python import mechanism (otherwise we would not have the cyclic imports issue at all).

ashb · 2020-02-24T11:22:14Z

Oh fark, PEP-562 is only Py 3.7+

I'll conditionally pull in https://github.com/facelessuser/pep562 for py <3.7

(Hell, that might even work on Py2 if we want it to)

ashb · 2020-02-24T11:23:16Z

Fantastic! Works like a charm in both IntelliJ and VSCode! I think we need to just make pytest understand it as well.

@potiuk Do you just mean the test failures, or something else? I.e. I'm not sure what pytest needs to understand as it runs the code so doesn't need the hack.

… untouched

codecov-io · 2020-02-24T13:28:13Z

Codecov Report

Merging #7517 into master will increase coverage by <.01%.
The diff coverage is 100%.

@@            Coverage Diff             @@
##           master    #7517      +/-   ##
==========================================
+ Coverage   86.81%   86.81%   +<.01%     
==========================================
  Files         893      893              
  Lines       42190    42221      +31     
==========================================
+ Hits        36626    36654      +28     
- Misses       5564     5567       +3

Impacted Files	Coverage Δ
...ders/databricks/example_dags/example_databricks.py	`100% <100%> (ø)`	⬆️
airflow/example_dags/example_python_operator.py	`63.33% <100%> (ø)`	⬆️
...irflow/example_dags/example_kubernetes_executor.py	`85% <100%> (ø)`	⬆️
...ample_dags/example_branch_python_dop_operator_3.py	`75% <100%> (ø)`	⬆️
...ow/providers/docker/example_dags/example_docker.py	`100% <100%> (ø)`	⬆️
airflow/example_dags/subdags/subdag.py	`100% <100%> (ø)`	⬆️
...w/example_dags/example_external_task_marker_dag.py	`100% <100%> (ø)`	⬆️
...w/example_dags/example_latest_only_with_trigger.py	`100% <100%> (ø)`	⬆️
...enkins/example_dags/example_jenkins_job_trigger.py	`65% <100%> (ø)`	⬆️
...ders/microsoft/winrm/example_dags/example_winrm.py	`100% <100%> (ø)`	⬆️
... and 38 more

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 a812f48...cb1b8a4. Read the comment docs.

potiuk · 2020-02-24T14:37:42Z

Go ahead @ashb -> I will make pylint related changes as folow up. It's great we could do this!

…hed (apache#7517) `from airflow import DAG` is _such_ a common pattern that it's worth making sure this stays working. Since we are now on Python 3 we can use PEP-562 to have officially supported lazy-loading of attributes on a module. Since the example_dags will be referred to/copied by users I have updated them all back to import from the airflow module, but have left any internal uses untouched.

boring-cyborg bot added provider:Apache provider:amazon AWS/Amazon - related issues labels Feb 23, 2020

ashb requested review from kaxil, mik-laj and potiuk February 23, 2020 22:50

kaxil approved these changes Feb 23, 2020

View reviewed changes

ashb removed provider:amazon AWS/Amazon - related issues provider:Apache labels Feb 23, 2020

ashb force-pushed the keep-dag-in-airflow-lazily branch from 452820e to 6cde5ec Compare February 23, 2020 22:59

boring-cyborg bot added provider:Apache provider:amazon AWS/Amazon - related issues labels Feb 23, 2020

ashb removed provider:amazon AWS/Amazon - related issues provider:Apache labels Feb 23, 2020

potiuk reviewed Feb 24, 2020

View reviewed changes

fixup! [AIRFLOW-6817] Lazy-load airflow.DAG to keep user-facing API…

77df817

… untouched

boring-cyborg bot added provider:Apache provider:amazon AWS/Amazon - related issues labels Feb 24, 2020

fixup! [AIRFLOW-6817] Lazy-load airflow.DAG to keep user-facing API…

cb1b8a4

… untouched

potiuk approved these changes Feb 24, 2020

View reviewed changes

ashb merged commit 3320e43 into apache:master Feb 24, 2020

ashb deleted the keep-dag-in-airflow-lazily branch February 24, 2020 14:55

casassg mentioned this pull request Apr 2, 2020

[AIP-31] Create @task decorator for functionally defined operators #8057

Closed

Taragolis mentioned this pull request Feb 20, 2024

Deprecate PY* constants into the airflow module #37575

Merged

[AIRFLOW-6817] Lazy-load airflow.DAG to keep user-facing API untouched #7517

[AIRFLOW-6817] Lazy-load airflow.DAG to keep user-facing API untouched #7517

Uh oh!

Conversation

ashb commented Feb 23, 2020 • edited by boring-cyborg bot Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

ashb commented Feb 23, 2020

Uh oh!

ashb commented Feb 23, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

kaxil commented Feb 24, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

potiuk commented Feb 24, 2020

Uh oh!

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

Uh oh!

Choose a reason for hiding this comment

Uh oh!

turbaszek Feb 24, 2020

Choose a reason for hiding this comment

Uh oh!

ashb Feb 24, 2020

Choose a reason for hiding this comment

Uh oh!

potiuk Feb 24, 2020

Choose a reason for hiding this comment

Uh oh!

ashb commented Feb 24, 2020

Uh oh!

ashb commented Feb 24, 2020

Uh oh!

codecov-io commented Feb 24, 2020

Codecov Report

Uh oh!

potiuk commented Feb 24, 2020

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

[AIRFLOW-6817] Lazy-load `airflow.DAG` to keep user-facing API untouched #7517

[AIRFLOW-6817] Lazy-load `airflow.DAG` to keep user-facing API untouched #7517

ashb commented Feb 23, 2020 •

edited by boring-cyborg bot

Loading

ashb commented Feb 23, 2020 •

edited

Loading

kaxil commented Feb 24, 2020 •

edited

Loading

potiuk Feb 24, 2020 •

edited

Loading