Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[AIRFLOW-31] Use standard imports for hooks/operators #1272

Merged
merged 2 commits into from
Jun 17, 2016

Conversation

jlowin
Copy link
Member

@jlowin jlowin commented Apr 1, 2016

See #1238 for details.

This PR replaces Airflow's import machinery with standard Python imports. This has an impact in four places:

airflow.hooks.*
airflow.operators.*
airflow.contrib.hooks.*
airflow.contrib.operators.*

Operators that could formerly be accessed from these directories must now be accessed explicitly from submodules:

from airflow.operators import PigOperator # NO
from airflow.operators.pig_operator import PigOperator # YES

Old style imports will still work but will warn:

In [4]: airflow.operators.PigOperator
/Users/jlowin/anaconda3/bin/ipython:1: DeprecationWarning: PigOperator: Importing 
PigOperator directly from 'airflow.operators' has been deprecated. Please import 
from 'airflow.operators.[operator_module]' instead. Support for direct imports will 
be dropped entirely in Airflow 2.0.
  #!/bin/bash /Users/jlowin/anaconda3/bin/python.app

Out[4]: pig_operator.PigOperator

run_unit_tests.sh exports an environment variable AIRFLOW_USE_NEW_IMPORTS which turns off the old-style (deprecated) imports completely. Therefore, unit tests must use the new style. I've tried to adjust the tests already but there are some I can't test locally so I will see what Travis has issues with and make adjustments.

Plugins are handled similarly. Let's say the user creates a plugin MyPlugin with some operators. Currently those operators are available at airflow.operators.*. Through this PR, that behavior is maintained (through Airflow 2.0) but a new module is dynamically created with the operators at airflow.operators.myplugin.*

Lastly, I took advantage of the fact I was going into lots of files to add licenses to all hooks and operators.

@landscape-bot
Copy link

Code Health
Repository health decreased by 0.26% when pulling 34a2223 on jlowin:standard-imports into 154dec0 on airbnb:master.

@jlowin jlowin force-pushed the standard-imports branch from 34a2223 to 0242562 Compare April 3, 2016 14:53
@landscape-bot
Copy link

Code Health
Repository health increased by 0.12% when pulling 0242562 on jlowin:standard-imports into ab1c90b on airbnb:master.

@landscape-bot
Copy link

Code Health
Repository health increased by 0.09% when pulling a829842 on jlowin:standard-imports into f657c16 on airbnb:master.

@jlowin jlowin force-pushed the standard-imports branch from a829842 to 054926c Compare May 2, 2016 12:54
@landscape-bot
Copy link

Code Health
Repository health increased by 0.04% when pulling 054926c on jlowin:standard-imports into f657c16 on airbnb:master.

@jlowin jlowin force-pushed the standard-imports branch from 054926c to 7ba55bf Compare May 2, 2016 13:45
@landscape-bot
Copy link

Code Health
Repository health increased by 0.09% when pulling 7ba55bf on jlowin:standard-imports into f657c16 on airbnb:master.

@jlowin jlowin force-pushed the standard-imports branch from 7ba55bf to 378f09a Compare May 2, 2016 20:53
@landscape-bot
Copy link

Code Health
Repository health increased by 0.08% when pulling 378f09a on jlowin:standard-imports into f657c16 on airbnb:master.

@jlowin jlowin changed the title Use standard imports for hooks/operators [AIRFLOW-31] Use standard imports for hooks/operators May 2, 2016
@coveralls
Copy link

coveralls commented May 2, 2016

Coverage Status

Coverage decreased (-4.0%) to 63.165% when pulling 378f09aa80fea95f95f8bac073319c3ede372b02 on jlowin:standard-imports into f657c16 on airbnb:master.

@jlowin jlowin force-pushed the standard-imports branch from 378f09a to 992e606 Compare June 14, 2016 19:14
@jlowin jlowin removed their assignment Jun 14, 2016
@jlowin jlowin force-pushed the standard-imports branch 7 times, most recently from 9eda57a to f2049c1 Compare June 16, 2016 13:24
@jlowin jlowin force-pushed the standard-imports branch 2 times, most recently from 7b8e62f to 1a32a83 Compare June 16, 2016 15:40
@jlowin jlowin force-pushed the standard-imports branch from 1a32a83 to 851adc5 Compare June 16, 2016 18:55
In Python 3, errors don’t have a `message` attribute
@codecov-io
Copy link

Current coverage is 63.71%

Merging #1272 into master will decrease coverage by 4.32%

@@             master      #1272   diff @@
==========================================
  Files           116        117     +1   
  Lines          8330       8425    +95   
  Methods           0          0          
  Messages          0          0          
  Branches          0          0          
==========================================
- Hits           5668       5368   -300   
- Misses         2662       3057   +395   
  Partials          0          0          

Powered by Codecov. Last updated by 949479c...f26b7a2

@jlowin
Copy link
Member Author

jlowin commented Jun 17, 2016

@criccomini at long last this appears to be ready for merge. The hardest part was that unit tests must use standard imports (I turn off the backwards-compatible import style via env var) and it was a real pain getting them all to comply. But it appears to be good to go.

@criccomini
Copy link
Contributor

Awesome! I'm +1 then. :)

Make sure to squash the commits, though.

@asfgit asfgit merged commit f26b7a2 into apache:master Jun 17, 2016
asfgit pushed a commit that referenced this pull request Jun 17, 2016
@paulbramsen
Copy link
Contributor

paulbramsen commented Jun 17, 2016

@jlowin hey @aoen and I think this is missing a dependency on zope. When I try to run even the most basic command (e.g. airflow version) it fails due to an ImportError because of from zope.deprecation import deprecated as _deprecated on airflow/hooks/__init__.py", line 66. I tried installing the most recent version of zope and that didn't fix it. I also noticed that zope is not mentioned anywhere in setup.py

@jlowin jlowin deleted the standard-imports branch December 20, 2016 13:18
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

8 participants