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

Requirements now depend on python version #7841

Conversation

potiuk
Copy link
Member

@potiuk potiuk commented Mar 23, 2020

Generate a separate requirements.txt file for each Python Version we support.


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


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.

@potiuk
Copy link
Member Author

potiuk commented Mar 23, 2020

IT turned out that requirements are slightly different for different versions of python (especially in 1.10) so I had to add different version of requirements. At the same time I removed -pinned version. It's not really needed we should be able to easily install airflow now with one command:

  pip install apache-airflow[gcp]==1.10.10 \
      --constraint https://raw.githubusercontent.com/apache/airflow/1.10.10/requirements/requirements-python3.7.txt

@potiuk potiuk force-pushed the different-requirements-for-different-python branch 2 times, most recently from f834138 to 8800dc3 Compare March 23, 2020 20:14
.travis.yml Outdated Show resolved Hide resolved
@potiuk
Copy link
Member Author

potiuk commented Mar 24, 2020

BTW. I am going to re-write some parts of it. I came to the conclusion that generating requirements is too big of a hassle for pre-commits (especially that we need them in different versions) so I will do a separate breeze command (and script for those not using Breeze) so that whenever you modify setup.py and new requirements are needed you will be notified via CI rather than pre-commit. I think it's much less impact on the development workflow.

Bear with me :).

@potiuk potiuk force-pushed the different-requirements-for-different-python branch from 8800dc3 to 422bdaf Compare March 24, 2020 18:43
@potiuk
Copy link
Member Author

potiuk commented Mar 24, 2020

@kaxil @ashb @turbaszek @mik-laj @dimberman -> We have much nicer version now. Requirement generation is now checked only as travis build (separately for each python version) rather than pre-commit and you can esily manually generate the requirements with breeze (examples in doc). Also when you modify setup.py in the way that it will require regeneration of the reuqirements, the build in CI will fail, show you with nice, coloured diff what is added and will ask you to run breeze generate-requirements --python x.y as needed.

Pls review. That's the last thing to cherry-pick and test prod image in airflow 1.10.10

@potiuk potiuk force-pushed the different-requirements-for-different-python branch from 422bdaf to c1660bd Compare March 24, 2020 19:53
@codecov-io
Copy link

codecov-io commented Mar 24, 2020

Codecov Report

Merging #7841 into master will decrease coverage by 22.37%.
The diff coverage is n/a.

Impacted file tree graph

@@             Coverage Diff             @@
##           master    #7841       +/-   ##
===========================================
- Coverage    87.1%   64.72%   -22.38%     
===========================================
  Files         928      927        -1     
  Lines       45025    44966       -59     
===========================================
- Hits        39217    29104    -10113     
- Misses       5808    15862    +10054
Impacted Files Coverage Δ
...low/contrib/operators/wasb_delete_blob_operator.py 0% <0%> (-100%) ⬇️
airflow/contrib/hooks/vertica_hook.py 0% <0%> (-100%) ⬇️
airflow/contrib/sensors/__init__.py 0% <0%> (-100%) ⬇️
airflow/hooks/mssql_hook.py 0% <0%> (-100%) ⬇️
...viders/docker/example_dags/example_docker_swarm.py 0% <0%> (-100%) ⬇️
airflow/hooks/webhdfs_hook.py 0% <0%> (-100%) ⬇️
airflow/contrib/sensors/emr_base_sensor.py 0% <0%> (-100%) ⬇️
...irflow/contrib/operators/slack_webhook_operator.py 0% <0%> (-100%) ⬇️
...providers/google/cloud/example_dags/example_dlp.py 0% <0%> (-100%) ⬇️
...cncf/kubernetes/example_dags/example_kubernetes.py 0% <0%> (-100%) ⬇️
... and 501 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 2a98a61...776f33c. Read the comment docs.

CONTRIBUTING.rst Outdated Show resolved Hide resolved
CONTRIBUTING.rst Outdated Show resolved Hide resolved
CONTRIBUTING.rst Outdated Show resolved Hide resolved
CONTRIBUTING.rst Outdated Show resolved Hide resolved
CONTRIBUTING.rst Outdated Show resolved Hide resolved
CONTRIBUTING.rst Outdated Show resolved Hide resolved
CONTRIBUTING.rst Outdated Show resolved Hide resolved
CONTRIBUTING.rst Outdated Show resolved Hide resolved
CONTRIBUTING.rst Outdated Show resolved Hide resolved
CONTRIBUTING.rst Outdated Show resolved Hide resolved
CONTRIBUTING.rst Outdated Show resolved Hide resolved
scripts/ci/_utils.sh Outdated Show resolved Hide resolved
@potiuk potiuk force-pushed the different-requirements-for-different-python branch 2 times, most recently from 9b02bbf to a64eb17 Compare March 25, 2020 04:49
@potiuk potiuk force-pushed the different-requirements-for-different-python branch 8 times, most recently from 9d3a5eb to 0f16fe0 Compare March 25, 2020 05:39
.travis.yml Show resolved Hide resolved
Dockerfile.ci Outdated Show resolved Hide resolved
@potiuk potiuk force-pushed the different-requirements-for-different-python branch 6 times, most recently from 776f33c to 7a1a25f Compare March 27, 2020 11:10
@potiuk potiuk force-pushed the different-requirements-for-different-python branch from 7a1a25f to 5245398 Compare March 27, 2020 12:06
@potiuk
Copy link
Member Author

potiuk commented Mar 27, 2020

OK. fixed separate requirements for different python versions is green in CI @kaxil @mik-laj -> do you want to take a look before I merge (@turbaszek ) ?

@potiuk potiuk merged commit 3fb5f15 into apache:master Mar 27, 2020
potiuk added a commit that referenced this pull request Mar 28, 2020
kaxil pushed a commit to astronomer/airflow that referenced this pull request Mar 30, 2020
kaxil pushed a commit that referenced this pull request Mar 30, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants