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

Update some dependencies #9684

Merged
merged 1 commit into from
Jul 6, 2020
Merged

Update some dependencies #9684

merged 1 commit into from
Jul 6, 2020

Conversation

Hartorn
Copy link
Contributor

@Hartorn Hartorn commented Jul 6, 2020


I had some trouble to install this along some latest version of packages, with Poetry as a dependency manager.
So here is what I needed to change :

  • latest version of flask-swagger
  • latest version of gunicorn
  • latest version of jinja2

Not sure if you guys are concerned with this, but there is some great tools to help manage dependency in Github, such as Renovate.

Else, if I want to try and contribute a bit, where should I start ? :)

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

  • Description above provides context of the change
  • Unit tests coverage for changes (not needed for documentation changes)
  • ~~ Target Github ISSUE in description if exists~~
  • Commits follow "How to write a good git commit message"
  • Relevant documentation is updated including usage instructions.
  • [ x I will engage committers as explained in Contribution Workflow Example.

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.

@boring-cyborg
Copy link

boring-cyborg bot commented Jul 6, 2020

Congratulations on your first Pull Request and welcome to the Apache Airflow community! If you have any issues or are unsure about any anything please check our Contribution Guide (https://github.com/apache/airflow/blob/master/CONTRIBUTING.rst)
Here are some useful points:

  • Pay attention to the quality of your code (flake8, pylint and type annotations). Our pre-commits will help you with that.
  • In case of a new feature add useful documentation (in docstrings or in docs/ directory). Adding a new operator? Check this short guide Consider adding an example DAG that shows how users should use it.
  • Consider using Breeze environment for testing locally, it’s a heavy docker but it ships with a working Airflow and a lot of integrations.
  • Be patient and persistent. It might take some time to get a review or get the final approval from Committers.
  • Please follow ASF Code of Conduct for all communication including (but not limited to) comments on Pull Requests, Mailing list and Slack.
  • Be sure to read the Airflow Coding style.
    Apache Airflow is a community-driven project and together we are making it better 🚀.
    In case of doubts contact the developers at:
    Mailing List: [email protected]
    Slack: https://apache-airflow-slack.herokuapp.com/

@potiuk
Copy link
Member

potiuk commented Jul 6, 2020

You will need to regenerate requirements if you change those. See https://github.com/apache/airflow/blob/master/CONTRIBUTING.rst#generating-requirement-files (also for python 3.8)

@Hartorn
Copy link
Contributor Author

Hartorn commented Jul 6, 2020

@potiuk I made the update.

Not sure why I had to do this though, isn't is possible to declare depencies in a pyproject.toml with python version ^3.6 ?
Wouldn'it do the same ?

@potiuk
Copy link
Member

potiuk commented Jul 6, 2020

@potiuk I made the update.

Not sure why I had to do this though, isn't is possible to declare depencies in a pyproject.toml with python version ^3.6 ?
Wouldn'it do the same ?

It is explained few paragraphs above: https://github.com/apache/airflow/blob/master/CONTRIBUTING.rst#airflow-dependencies

Airflow is not a standard project - it's both an "application" (for people installing it) and "library" (for people writing DAGs). Usually applications have pinned dependencies and libraries have dependencies as opened as possible. This article is one of many that explains why: http://blog.chrisgorgolewski.org/2017/12/to-pin-or-not-to-pin-dependencies.html . Since we are a bit of both, we had to come up with a clever handling of deps:

  • In setup.py we have as open deps as possible so that you can upgrade to newer version of depended library when it is released so that you can develop DAGs using latest compatible versions of libraries
  • In "requirements.txt" we keep the pinned requirement file that you can use as --constraint when installing airflow as "application" (via pip install) - see INSTALL for recommended way of installing airflow and some more options in CONTIBUTING.rst where you can get the constraint files directly from GitHub. This way you can be sure that you can have working airflow installed no matter if someone released a new version of their libraries that breaks it (happened many times in the past)

The "Genereate requirements" process is our way to keep the pinned dependencies updated regularly and our test suite makes sure that when you generated a new dependencies, those are used during tests and we know that those new dependencies did not break airflow (if we find that they do, we either fix it or limit versions in setup.py - it happened many time in the past that we experienced this problem).

There are subtle differences between different python versions for those - you can compare those generated deps. - that's why have separate constraint file for each version of python.

I hope that explains why our process is a bit complex than with a regular library or application (yes, I'd love to use poetry for that but poetry does not support this application <> library dualism we have).

@potiuk potiuk merged commit fd62b1c into apache:master Jul 6, 2020
@boring-cyborg
Copy link

boring-cyborg bot commented Jul 6, 2020

Awesome work, congrats on your first merged pull request!

@Hartorn
Copy link
Contributor Author

Hartorn commented Jul 6, 2020

@potiuk Thks for the great answer, I will check in depth both the links and the diff

Cheers

@Hartorn Hartorn deleted the patch-1 branch July 6, 2020 11:48
potiuk added a commit to PolideaInternal/airflow that referenced this pull request Jul 6, 2020
@potiuk
Copy link
Member

potiuk commented Jul 6, 2020

FYI @Hartorn -> we have to revert this one temporarily. We did not handle the case where tests were triggered after only the "setup.py" changed (#9690 ) so this one broke the master (via upgrade to one of azure dependencies) - for precisely the reason I described in my comment above :).

@potiuk
Copy link
Member

potiuk commented Jul 6, 2020

Usually changes in deps were coupled with some code changes - that's why we have not seen this problem before.

potiuk added a commit that referenced this pull request Jul 6, 2020
@Hartorn
Copy link
Contributor Author

Hartorn commented Jul 6, 2020

No worries, I will manage with a fork for now.

@Hartorn Hartorn restored the patch-1 branch July 6, 2020 14:00
potiuk pushed a commit that referenced this pull request Jul 22, 2020
(cherry picked from commit fd62b1c)
@potiuk potiuk added this to the Airflow 1.10.12 milestone Jul 22, 2020
potiuk added a commit that referenced this pull request Jul 22, 2020
kaxil pushed a commit that referenced this pull request Aug 11, 2020
(cherry picked from commit fd62b1c)
cfei18 pushed a commit to cfei18/incubator-airflow that referenced this pull request Mar 5, 2021
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.

2 participants