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

Pinning max pandas version to 2.0 (lesser than) to allow pandas 1.0. #7954

Merged
merged 2 commits into from
Mar 30, 2020
Merged

Pinning max pandas version to 2.0 (lesser than) to allow pandas 1.0. #7954

merged 2 commits into from
Mar 30, 2020

Conversation

JPFrancoia
Copy link
Contributor

@JPFrancoia JPFrancoia commented Mar 29, 2020

Issue link: #7905

Currently on master, pandas is pinned to < 1.0.0 in Airflow's dependencies. Version 1.0 was released in October 31, 2019. Lots of projects are starting to migrate to pandas 1.0 and it will gradually become more and more difficult to solve conflicts between their dependencies and Airflow's dependencies.

This PR increases the max version of pandas to 2.0, which will allow installing pandas 1.0. The minimum pandas version stays unchanged so all existing projects with pandas version pinned to 1.0 shouldn't be impacted.

My commit passed the pre-commits. I installed (or tried to) installed as much dependencies as possible in the all extra (in a local virtualenv), and ran as much unit tests as possible before upgrading pandas. This was my baseline. I then upgraded pandas to 1.0.3 and ran the same tests again and the number of failed tests didn't decrease.

I'm sorry I can't give a clearer answer on whether pandas' version will break something, but testing has proven to be difficult: the integration tests are tangled with the unit tests and it's unclear to me which tests should pass or not. Help is appreciated.

@boring-cyborg
Copy link

boring-cyborg bot commented Mar 29, 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.
  • 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 Mar 29, 2020

Please rebase to latest master to fix a problem that I've fixed yesterday. Also just yesterday we've merged a change that will help with managing requirements. It's best if you use Breeze to run breeze generate-requirements --python 3.6 and breeze generate-requirements --python 3.7 to regenerate fixed set of requirements in "requirements" folder.

See a brand new chapter (and the following ones) in the Contributing guide about it. https://github.com/apache/airflow/blob/master/CONTRIBUTING.rst#airflow-dependencies

BTW. After rebasing to the latest master you will get a failing "generate requirements" build in case you do not run it and those instructions will be printed in the log of failing build.

@JPFrancoia
Copy link
Contributor Author

JPFrancoia commented Mar 29, 2020

I forked from master this morning (a couple hours ago), I think I'm up to date.

Regarding the generation of the requirements files: everything seems to run fine, I get that for example:

Successfully installed apache-airflow
You are using pip version 19.0.2, however version 20.0.2 is available.
You should consider upgrading via the 'pip install --upgrade pip' command.

Copying requirements /opt/airflow/requirements/requirements-python3.6.txt -> /tmp/requirements-python3.6.txt


Freezing requirements to /opt/airflow/requirements/requirements-python3.6.txt


Requirements generated in /opt/airflow/requirements/requirements-python3.6.txt

But I can't find the requirement file, neither in /opt/airflow/requirements/requirements-python3.6.txt or in /tmp/requirements-python3.6.txt. Is it expected?

@codecov-io
Copy link

Codecov Report

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

Impacted file tree graph

@@            Coverage Diff             @@
##           master    #7954      +/-   ##
==========================================
- Coverage   87.16%   86.90%   -0.26%     
==========================================
  Files         931      932       +1     
  Lines       45173    45187      +14     
==========================================
- Hits        39375    39270     -105     
- Misses       5798     5917     +119     
Impacted Files Coverage Δ
airflow/kubernetes/volume_mount.py 44.44% <0.00%> (-55.56%) ⬇️
airflow/kubernetes/volume.py 52.94% <0.00%> (-47.06%) ⬇️
airflow/kubernetes/pod_launcher.py 47.18% <0.00%> (-45.08%) ⬇️
...viders/cncf/kubernetes/operators/kubernetes_pod.py 69.69% <0.00%> (-25.26%) ⬇️
airflow/kubernetes/refresh_config.py 50.98% <0.00%> (-23.53%) ⬇️
...ud/example_dags/example_kubernetes_engine_setup.py 100.00% <0.00%> (ø)

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 daad60b...39134cc. Read the comment docs.

@potiuk
Copy link
Member

potiuk commented Mar 30, 2020

Hello @JPFrancoia -> it looks good but I would like to wait until we merge #7980 . Then I will ask you to rebase it on top of the latest master. What happened in the current case (if you look closely) was that even if you added <2.0 exclusion, Pandas were not upgraded. This is exactly what #7980 is addressing. When you rebase, you will have to run generate-requirements again (this time panda should get bumped to 2.0 for both python versions) and you should re-push the change. Then we will actually test if Pandas 1.0 is OK.

@potiuk
Copy link
Member

potiuk commented Mar 30, 2020

Hello @JPFrancoia -> it's merged, so you can rebase now.

@JPFrancoia
Copy link
Contributor Author

Ah, I understand now. Ok, I rebased and regenerated the requirements files. Let's see what happens.

@JPFrancoia
Copy link
Contributor Author

Nothing broke \o/

@potiuk potiuk merged commit 1428c0f into apache:master Mar 30, 2020
@boring-cyborg
Copy link

boring-cyborg bot commented Mar 30, 2020

Awesome work, congrats on your first merged pull request!

@potiuk
Copy link
Member

potiuk commented Mar 30, 2020

Cool!

@JPFrancoia
Copy link
Contributor Author

Thanks for your guidance @potiuk .

@josh-gree
Copy link

Hey thanks for this PR great to be able to have pandas > 1.0! Can I ask when this will make its way into a release?

@kaxil
Copy link
Member

kaxil commented Jun 15, 2020

Hey thanks for this PR great to be able to have pandas > 1.0! Can I ask when this will make its way into a release?

1.10.11 will contain this PR

@kaxil kaxil added this to the Airflow 1.10.11 milestone Jun 15, 2020
kaxil pushed a commit that referenced this pull request Jun 22, 2020
potiuk pushed a commit that referenced this pull request Jun 29, 2020
@kaxil kaxil added the type:improvement Changelog: Improvements label Jul 1, 2020
kaxil pushed a commit that referenced this pull request Jul 1, 2020
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
type:improvement Changelog: Improvements
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants