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-6428] Fix import path for airflow.utils.dates.days_ago in Example DAGs #7007

Merged
merged 1 commit into from
Jan 3, 2020

Conversation

kaxil
Copy link
Member

@kaxil kaxil commented Jan 2, 2020

Import the days_ago function directly from airflow.utils.dates in all our Example DAGs.

Currently, without the entry in init.py, IDEs show that it could not find the reference to dates in most of our example_dags as they contain airflow.utils.dates.days_ago(1) and hence if you try to find the reference for days_ago function or dates modules it can't find it.

image
image


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.

@kaxil kaxil requested a review from potiuk January 2, 2020 15:40
@codecov-io
Copy link

codecov-io commented Jan 2, 2020

Codecov Report

Merging #7007 into master will decrease coverage by 0.28%.
The diff coverage is 100%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master    #7007      +/-   ##
==========================================
- Coverage   84.85%   84.56%   -0.29%     
==========================================
  Files         679      680       +1     
  Lines       38542    38543       +1     
==========================================
- Hits        32703    32594     -109     
- Misses       5839     5949     +110
Impacted Files Coverage Δ
airflow/utils/__init__.py 100% <100%> (ø)
airflow/kubernetes/volume_mount.py 44.44% <0%> (-55.56%) ⬇️
airflow/kubernetes/volume.py 52.94% <0%> (-47.06%) ⬇️
airflow/kubernetes/pod_launcher.py 45.25% <0%> (-46.72%) ⬇️
airflow/kubernetes/refresh_config.py 50.98% <0%> (-23.53%) ⬇️
...rflow/contrib/operators/kubernetes_pod_operator.py 78.75% <0%> (-20%) ⬇️

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 aa90753...267995a. Read the comment docs.

@potiuk
Copy link
Member

potiuk commented Jan 2, 2020

Why don't we do it "differently" ...

I am not sure why we are using those imports in this form? Do we have something that holds us back from changing all "days_ago" imports to the form that is much more pythonic (IMHO)?

Adding anything to __init__.py inside the application creates unnecessary dependencies. Anybody using "airflow.utils.somethingelse" will add an implicit dependency to "airflow.utils.dates" if we add dates to __init__.py - even if it is not used directly. This adds unnecessary dependencies (and leads to circular dependencies).

I think most of our __init__.py should be empty (or removed if we go to implicit python3 packages). I believe adding anything to __init__.py makes only sense if we provide a reusable library with one package - when if you import it, you should have access to all exposed functions.

I am happy to discuss it though, as we might have different understanding - and maybe we should expose all "exposable" classes from unit in this way as part of the "official airflow interface" (but I still think import should be from airflow.utils.xxx import yyy anyway).

Just to summary - we have two options:

  1. Import airflow and then rely on the init packages
import airflow

and then using

airflow.utils.dates.days_ago(2)
  1. Import the function directly (much better IMHO).
from airflow.utils.dates import days_ago

Option 1 (with importing the whole 'airflow').

Screenshot 2020-01-02 at 23 16 23

Option 2: (wiht importing only the function we need)

Screenshot 2020-01-02 at 23 09 14

WDYT @kaxil?

Copy link
Member

@potiuk potiuk left a comment

Choose a reason for hiding this comment

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

Just wanted to discuss general import approach in this case.

@kaxil kaxil changed the title [AIRFLOW-6428] Add dates module to airflow/utils/__init__.py [AIRFLOW-6428] Fix import path for airflow.utils.dates.days_ago in Example DAGs Jan 3, 2020
@kaxil
Copy link
Member Author

kaxil commented Jan 3, 2020

Just wanted to discuss general import approach in this case.

I agree the 2nd option sounds lot better, I have changed the PR and updated all the example DAGs by importing just the function from module instead of init.py.

@potiuk
Copy link
Member

potiuk commented Jan 3, 2020

👍 👍 👍 👍 👍 👍 👍 👍 👍 👍 👍 👍 👍 👍 👍 👍 👍

@potiuk
Copy link
Member

potiuk commented Jan 3, 2020

Needs rebase @kaxil :(.

BTW. I think we need a "Needs rebase" probot.

@potiuk
Copy link
Member

potiuk commented Jan 3, 2020

Hmm https://github.com/tibdex/autorebase - but it seems a bit abandoned in favour of autosquash.

@kaxil kaxil merged commit 18e8cea into apache:master Jan 3, 2020
@kaxil kaxil deleted the add-utils-init-dates branch January 3, 2020 13:42
kaxil added a commit that referenced this pull request Jan 26, 2020
kaxil added a commit that referenced this pull request Feb 3, 2020
galuszkak pushed a commit to FlyrInc/apache-airflow that referenced this pull request Mar 5, 2020
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.

4 participants