You are viewing a plain text version of this content. The canonical link for it is here.
Posted to commits@airflow.apache.org by GitBox <gi...@apache.org> on 2020/02/23 22:50:38 UTC

[GitHub] [airflow] ashb opened a new pull request #7517: [AIRFLOW-6817] Lazy-load `airflow.DAG` to keep user-facing API untouched

ashb opened a new pull request #7517: [AIRFLOW-6817] Lazy-load `airflow.DAG` to keep user-facing API untouched
URL: https://github.com/apache/airflow/pull/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.
   
   This approach taken from Celery: https://github.com/celery/celery/blob/f2ddd894c32f642a20f03b805b97e460f4fb3b4f/celery/__init__.py#L63-L78
   
   ---
   Issue link: WILL BE INSERTED BY [boring-cyborg](https://github.com/kaxil/boring-cyborg)
   
   Make sure to mark the boxes below before creating PR: [x]
   
   - [x] Description above provides context of the change
   - [x] Commit message/PR title starts with `[AIRFLOW-NNNN]`. AIRFLOW-NNNN = JIRA ID<sup>*</sup>
   - [x] Unit tests coverage for changes (not needed for documentation changes)
   - [x] Commits follow "[How to write a good git commit message](http://chris.beams.io/posts/git-commit/)"
   - [x] Relevant documentation is updated including usage instructions.
   - [x] I will engage committers as explained in [Contribution Workflow Example](https://github.com/apache/airflow/blob/master/CONTRIBUTING.rst#contribution-workflow-example).
   
   <sup>*</sup> For document-only changes commit message can start with `[AIRFLOW-XXXX]`.
   
   ---
   In case of fundamental code change, Airflow Improvement Proposal ([AIP](https://cwiki.apache.org/confluence/display/AIRFLOW/Airflow+Improvements+Proposals)) is needed.
   In case of a new dependency, check compliance with the [ASF 3rd Party License Policy](https://www.apache.org/legal/resolved.html#category-x).
   In case of backwards incompatible changes please leave a note in [UPDATING.md](https://github.com/apache/airflow/blob/master/UPDATING.md).
   Read the [Pull Request Guidelines](https://github.com/apache/airflow/blob/master/CONTRIBUTING.rst#pull-request-guidelines) for more information.
   

----------------------------------------------------------------
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.
 
For queries about this service, please contact Infrastructure at:
users@infra.apache.org


With regards,
Apache Git Services

[GitHub] [airflow] ashb commented on a change in pull request #7517: [AIRFLOW-6817] Lazy-load `airflow.DAG` to keep user-facing API untouched

Posted by GitBox <gi...@apache.org>.
ashb commented on a change in pull request #7517: [AIRFLOW-6817] Lazy-load `airflow.DAG` to keep user-facing API untouched
URL: https://github.com/apache/airflow/pull/7517#discussion_r383308021
 
 

 ##########
 File path: airflow/__init__.py
 ##########
 @@ -42,3 +42,22 @@
 login: Optional[Callable] = None
 
 integrate_plugins()
+
+__all__ = ['__version__', 'login', 'DAG']
+
+
+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
 
 Review comment:
   Given this isn't at the top level does pylint still detect it as a cycle?

----------------------------------------------------------------
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.
 
For queries about this service, please contact Infrastructure at:
users@infra.apache.org


With regards,
Apache Git Services

[GitHub] [airflow] nuclearpinguin commented on a change in pull request #7517: [AIRFLOW-6817] Lazy-load `airflow.DAG` to keep user-facing API untouched

Posted by GitBox <gi...@apache.org>.
nuclearpinguin commented on a change in pull request #7517: [AIRFLOW-6817] Lazy-load `airflow.DAG` to keep user-facing API untouched
URL: https://github.com/apache/airflow/pull/7517#discussion_r383119737
 
 

 ##########
 File path: airflow/__init__.py
 ##########
 @@ -42,3 +42,22 @@
 login: Optional[Callable] = None
 
 integrate_plugins()
+
+__all__ = ['__version__', 'login', 'DAG']
+
+
+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
 
 Review comment:
   Worth to try but I have a feeling that this has to be disabled in all places where the cycle occurs.

----------------------------------------------------------------
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.
 
For queries about this service, please contact Infrastructure at:
users@infra.apache.org


With regards,
Apache Git Services

[GitHub] [airflow] potiuk commented on issue #7517: [AIRFLOW-6817] Lazy-load `airflow.DAG` to keep user-facing API untouched

Posted by GitBox <gi...@apache.org>.
potiuk commented on issue #7517: [AIRFLOW-6817] Lazy-load `airflow.DAG` to keep user-facing API untouched
URL: https://github.com/apache/airflow/pull/7517#issuecomment-590194706
 
 
   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). 

----------------------------------------------------------------
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.
 
For queries about this service, please contact Infrastructure at:
users@infra.apache.org


With regards,
Apache Git Services

[GitHub] [airflow] ashb edited a comment on issue #7517: [AIRFLOW-6817] Lazy-load `airflow.DAG` to keep user-facing API untouched

Posted by GitBox <gi...@apache.org>.
ashb edited a comment on issue #7517: [AIRFLOW-6817] Lazy-load `airflow.DAG` to keep user-facing API untouched
URL: https://github.com/apache/airflow/pull/7517#issuecomment-590126692
 
 
   ~Oh, can remove a line from updating.md now~

----------------------------------------------------------------
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.
 
For queries about this service, please contact Infrastructure at:
users@infra.apache.org


With regards,
Apache Git Services

[GitHub] [airflow] ashb commented on issue #7517: [AIRFLOW-6817] Lazy-load `airflow.DAG` to keep user-facing API untouched

Posted by GitBox <gi...@apache.org>.
ashb commented on issue #7517: [AIRFLOW-6817] Lazy-load `airflow.DAG` to keep user-facing API untouched
URL: https://github.com/apache/airflow/pull/7517#issuecomment-590274997
 
 
   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)

----------------------------------------------------------------
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.
 
For queries about this service, please contact Infrastructure at:
users@infra.apache.org


With regards,
Apache Git Services

[GitHub] [airflow] ashb merged pull request #7517: [AIRFLOW-6817] Lazy-load `airflow.DAG` to keep user-facing API untouched

Posted by GitBox <gi...@apache.org>.
ashb merged pull request #7517: [AIRFLOW-6817] Lazy-load `airflow.DAG` to keep user-facing API untouched
URL: https://github.com/apache/airflow/pull/7517
 
 
   

----------------------------------------------------------------
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.
 
For queries about this service, please contact Infrastructure at:
users@infra.apache.org


With regards,
Apache Git Services

[GitHub] [airflow] potiuk commented on issue #7517: [AIRFLOW-6817] Lazy-load `airflow.DAG` to keep user-facing API untouched

Posted by GitBox <gi...@apache.org>.
potiuk commented on issue #7517: [AIRFLOW-6817] Lazy-load `airflow.DAG` to keep user-facing API untouched
URL: https://github.com/apache/airflow/pull/7517#issuecomment-590351079
 
 
   Go ahead @ashb -> I will make pylint related changes as folow up. It's great we could do this! 

----------------------------------------------------------------
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.
 
For queries about this service, please contact Infrastructure at:
users@infra.apache.org


With regards,
Apache Git Services

[GitHub] [airflow] ashb commented on issue #7517: [AIRFLOW-6817] Lazy-load `airflow.DAG` to keep user-facing API untouched

Posted by GitBox <gi...@apache.org>.
ashb commented on issue #7517: [AIRFLOW-6817] Lazy-load `airflow.DAG` to keep user-facing API untouched
URL: https://github.com/apache/airflow/pull/7517#issuecomment-590126623
 
 
   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.

----------------------------------------------------------------
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.
 
For queries about this service, please contact Infrastructure at:
users@infra.apache.org


With regards,
Apache Git Services

[GitHub] [airflow] potiuk commented on a change in pull request #7517: [AIRFLOW-6817] Lazy-load `airflow.DAG` to keep user-facing API untouched

Posted by GitBox <gi...@apache.org>.
potiuk commented on a change in pull request #7517: [AIRFLOW-6817] Lazy-load `airflow.DAG` to keep user-facing API untouched
URL: https://github.com/apache/airflow/pull/7517#discussion_r383111273
 
 

 ##########
 File path: airflow/__init__.py
 ##########
 @@ -42,3 +42,22 @@
 login: Optional[Callable] = None
 
 integrate_plugins()
+
+__all__ = ['__version__', 'login', 'DAG']
+
+
+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
 
 Review comment:
   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.

----------------------------------------------------------------
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.
 
For queries about this service, please contact Infrastructure at:
users@infra.apache.org


With regards,
Apache Git Services

[GitHub] [airflow] codecov-io commented on issue #7517: [AIRFLOW-6817] Lazy-load `airflow.DAG` to keep user-facing API untouched

Posted by GitBox <gi...@apache.org>.
codecov-io commented on issue #7517: [AIRFLOW-6817] Lazy-load `airflow.DAG` to keep user-facing API untouched
URL: https://github.com/apache/airflow/pull/7517#issuecomment-590319469
 
 
   # [Codecov](https://codecov.io/gh/apache/airflow/pull/7517?src=pr&el=h1) Report
   > Merging [#7517](https://codecov.io/gh/apache/airflow/pull/7517?src=pr&el=desc) into [master](https://codecov.io/gh/apache/airflow/commit/a812f48052d0839ea4e9d6ddc6b6e6be78608ce7?src=pr&el=desc) will **increase** coverage by `<.01%`.
   > The diff coverage is `100%`.
   
   [![Impacted file tree graph](https://codecov.io/gh/apache/airflow/pull/7517/graphs/tree.svg?width=650&token=WdLKlKHOAU&height=150&src=pr)](https://codecov.io/gh/apache/airflow/pull/7517?src=pr&el=tree)
   
   ```diff
   @@            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](https://codecov.io/gh/apache/airflow/pull/7517?src=pr&el=tree) | Coverage Δ | |
   |---|---|---|
   | [...ders/databricks/example\_dags/example\_databricks.py](https://codecov.io/gh/apache/airflow/pull/7517/diff?src=pr&el=tree#diff-YWlyZmxvdy9wcm92aWRlcnMvZGF0YWJyaWNrcy9leGFtcGxlX2RhZ3MvZXhhbXBsZV9kYXRhYnJpY2tzLnB5) | `100% <100%> (ø)` | :arrow_up: |
   | [airflow/example\_dags/example\_python\_operator.py](https://codecov.io/gh/apache/airflow/pull/7517/diff?src=pr&el=tree#diff-YWlyZmxvdy9leGFtcGxlX2RhZ3MvZXhhbXBsZV9weXRob25fb3BlcmF0b3IucHk=) | `63.33% <100%> (ø)` | :arrow_up: |
   | [...irflow/example\_dags/example\_kubernetes\_executor.py](https://codecov.io/gh/apache/airflow/pull/7517/diff?src=pr&el=tree#diff-YWlyZmxvdy9leGFtcGxlX2RhZ3MvZXhhbXBsZV9rdWJlcm5ldGVzX2V4ZWN1dG9yLnB5) | `85% <100%> (ø)` | :arrow_up: |
   | [...ample\_dags/example\_branch\_python\_dop\_operator\_3.py](https://codecov.io/gh/apache/airflow/pull/7517/diff?src=pr&el=tree#diff-YWlyZmxvdy9leGFtcGxlX2RhZ3MvZXhhbXBsZV9icmFuY2hfcHl0aG9uX2RvcF9vcGVyYXRvcl8zLnB5) | `75% <100%> (ø)` | :arrow_up: |
   | [...ow/providers/docker/example\_dags/example\_docker.py](https://codecov.io/gh/apache/airflow/pull/7517/diff?src=pr&el=tree#diff-YWlyZmxvdy9wcm92aWRlcnMvZG9ja2VyL2V4YW1wbGVfZGFncy9leGFtcGxlX2RvY2tlci5weQ==) | `100% <100%> (ø)` | :arrow_up: |
   | [airflow/example\_dags/subdags/subdag.py](https://codecov.io/gh/apache/airflow/pull/7517/diff?src=pr&el=tree#diff-YWlyZmxvdy9leGFtcGxlX2RhZ3Mvc3ViZGFncy9zdWJkYWcucHk=) | `100% <100%> (ø)` | :arrow_up: |
   | [...w/example\_dags/example\_external\_task\_marker\_dag.py](https://codecov.io/gh/apache/airflow/pull/7517/diff?src=pr&el=tree#diff-YWlyZmxvdy9leGFtcGxlX2RhZ3MvZXhhbXBsZV9leHRlcm5hbF90YXNrX21hcmtlcl9kYWcucHk=) | `100% <100%> (ø)` | :arrow_up: |
   | [...w/example\_dags/example\_latest\_only\_with\_trigger.py](https://codecov.io/gh/apache/airflow/pull/7517/diff?src=pr&el=tree#diff-YWlyZmxvdy9leGFtcGxlX2RhZ3MvZXhhbXBsZV9sYXRlc3Rfb25seV93aXRoX3RyaWdnZXIucHk=) | `100% <100%> (ø)` | :arrow_up: |
   | [...enkins/example\_dags/example\_jenkins\_job\_trigger.py](https://codecov.io/gh/apache/airflow/pull/7517/diff?src=pr&el=tree#diff-YWlyZmxvdy9wcm92aWRlcnMvamVua2lucy9leGFtcGxlX2RhZ3MvZXhhbXBsZV9qZW5raW5zX2pvYl90cmlnZ2VyLnB5) | `65% <100%> (ø)` | :arrow_up: |
   | [...ders/microsoft/winrm/example\_dags/example\_winrm.py](https://codecov.io/gh/apache/airflow/pull/7517/diff?src=pr&el=tree#diff-YWlyZmxvdy9wcm92aWRlcnMvbWljcm9zb2Z0L3dpbnJtL2V4YW1wbGVfZGFncy9leGFtcGxlX3dpbnJtLnB5) | `100% <100%> (ø)` | :arrow_up: |
   | ... and [38 more](https://codecov.io/gh/apache/airflow/pull/7517/diff?src=pr&el=tree-more) | |
   
   ------
   
   [Continue to review full report at Codecov](https://codecov.io/gh/apache/airflow/pull/7517?src=pr&el=continue).
   > **Legend** - [Click here to learn more](https://docs.codecov.io/docs/codecov-delta)
   > `Δ = absolute <relative> (impact)`, `ø = not affected`, `? = missing data`
   > Powered by [Codecov](https://codecov.io/gh/apache/airflow/pull/7517?src=pr&el=footer). Last update [a812f48...cb1b8a4](https://codecov.io/gh/apache/airflow/pull/7517?src=pr&el=lastupdated). Read the [comment docs](https://docs.codecov.io/docs/pull-request-comments).
   

----------------------------------------------------------------
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.
 
For queries about this service, please contact Infrastructure at:
users@infra.apache.org


With regards,
Apache Git Services

[GitHub] [airflow] kaxil edited a comment on issue #7517: [AIRFLOW-6817] Lazy-load `airflow.DAG` to keep user-facing API untouched

Posted by GitBox <gi...@apache.org>.
kaxil edited a comment on issue #7517: [AIRFLOW-6817] Lazy-load `airflow.DAG` to keep user-facing API untouched
URL: https://github.com/apache/airflow/pull/7517#issuecomment-590132339
 
 
   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'
   ```

----------------------------------------------------------------
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.
 
For queries about this service, please contact Infrastructure at:
users@infra.apache.org


With regards,
Apache Git Services

[GitHub] [airflow] ashb commented on issue #7517: [AIRFLOW-6817] Lazy-load `airflow.DAG` to keep user-facing API untouched

Posted by GitBox <gi...@apache.org>.
ashb commented on issue #7517: [AIRFLOW-6817] Lazy-load `airflow.DAG` to keep user-facing API untouched
URL: https://github.com/apache/airflow/pull/7517#issuecomment-590275407
 
 
   > 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.

----------------------------------------------------------------
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.
 
For queries about this service, please contact Infrastructure at:
users@infra.apache.org


With regards,
Apache Git Services

[GitHub] [airflow] ashb commented on issue #7517: [AIRFLOW-6817] Lazy-load `airflow.DAG` to keep user-facing API untouched

Posted by GitBox <gi...@apache.org>.
ashb commented on issue #7517: [AIRFLOW-6817] Lazy-load `airflow.DAG` to keep user-facing API untouched
URL: https://github.com/apache/airflow/pull/7517#issuecomment-590126692
 
 
   Oh, can remove a line from updating.md now

----------------------------------------------------------------
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.
 
For queries about this service, please contact Infrastructure at:
users@infra.apache.org


With regards,
Apache Git Services

[GitHub] [airflow] potiuk commented on a change in pull request #7517: [AIRFLOW-6817] Lazy-load `airflow.DAG` to keep user-facing API untouched

Posted by GitBox <gi...@apache.org>.
potiuk commented on a change in pull request #7517: [AIRFLOW-6817] Lazy-load `airflow.DAG` to keep user-facing API untouched
URL: https://github.com/apache/airflow/pull/7517#discussion_r383111273
 
 

 ##########
 File path: airflow/__init__.py
 ##########
 @@ -42,3 +42,22 @@
 login: Optional[Callable] = None
 
 integrate_plugins()
+
+__all__ = ['__version__', 'login', 'DAG']
+
+
+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
 
 Review comment:
   I think we also need to disable cyclic dependency pylint check as well

----------------------------------------------------------------
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.
 
For queries about this service, please contact Infrastructure at:
users@infra.apache.org


With regards,
Apache Git Services

[GitHub] [airflow] potiuk commented on a change in pull request #7517: [AIRFLOW-6817] Lazy-load `airflow.DAG` to keep user-facing API untouched

Posted by GitBox <gi...@apache.org>.
potiuk commented on a change in pull request #7517: [AIRFLOW-6817] Lazy-load `airflow.DAG` to keep user-facing API untouched
URL: https://github.com/apache/airflow/pull/7517#discussion_r383309104
 
 

 ##########
 File path: airflow/__init__.py
 ##########
 @@ -42,3 +42,22 @@
 login: Optional[Callable] = None
 
 integrate_plugins()
+
+__all__ = ['__version__', 'login', 'DAG']
+
+
+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
 
 Review comment:
   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). 

----------------------------------------------------------------
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.
 
For queries about this service, please contact Infrastructure at:
users@infra.apache.org


With regards,
Apache Git Services

[GitHub] [airflow] kaxil commented on issue #7517: [AIRFLOW-6817] Lazy-load `airflow.DAG` to keep user-facing API untouched

Posted by GitBox <gi...@apache.org>.
kaxil commented on issue #7517: [AIRFLOW-6817] Lazy-load `airflow.DAG` to keep user-facing API untouched
URL: https://github.com/apache/airflow/pull/7517#issuecomment-590132339
 
 
   ```
   ____________ 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'
   ```

----------------------------------------------------------------
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.
 
For queries about this service, please contact Infrastructure at:
users@infra.apache.org


With regards,
Apache Git Services