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

python3Packages.apache-airflow init 1.10.5 #73074

Merged
merged 20 commits into from Nov 18, 2019

Conversation

ingenieroariel
Copy link
Member

Motivation for this change

Apache Airflow is a widely used workflow tool. https://github.com/apache/airflow

Two PRs precede this one:

This PR builds on #65026 but avoids the py27 question completely by making it py3k only and patching airflow to be able to use the version of pendulum already in nixpkgs.

Things done
  • [ x ] Tested using sandboxing (nix.useSandbox on NixOS, or option sandbox in nix.conf on non-NixOS linux)
  • Built on platform(s)
    • [ x ] NixOS
    • macOS
    • other Linux distributions
  • Tested via one or more NixOS test(s) if existing and applicable for the change (look inside nixos/tests)
  • Tested compilation of all pkgs that depend on this change using nix-shell -p nix-review --run "nix-review wip"
  • [ x ] Tested execution of all binary files (usually in ./result/bin/)
  • Determined the impact on package closure size (by running nix path-info -S before and after)
  • Ensured that relevant documentation is up to date
  • Fits CONTRIBUTING.md.
Notify maintainers

cc @

Copy link
Contributor

@flokli flokli left a comment

Choose a reason for hiding this comment

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

Thanks a lot, must have been a lot of work! Some suggestions.

@ingenieroariel
Copy link
Member Author

Thanks a lot, must have been a lot of work! Some suggestions.

All the credit for the approach and hard work goes to @costrouc

I'll unify the approach on skipping tests / fetching patches to upstream from PRs created to their repo. Thanks a lot for the quick feedback @flokli!

@ingenieroariel
Copy link
Member Author

@flokli I think I covered all the comments/suggestions with the new commit. If I missed anything please let me know. Thanks in advance!

Copy link
Contributor

@flokli flokli left a comment

Choose a reason for hiding this comment

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

This already looks quite good! Thanks a lot. @FRidh, could you take a look as well?

@flokli
Copy link
Contributor

flokli commented Nov 12, 2019

Not something that should stall this PR, but @ingenieroariel do you have a NixOS module in the pipeline aswell, or some quick notes on how to get it running?

It might be beneficial to have some NixOS tests that spins up airflow, and does some curl request to see it was able to do the initialization.

@ingenieroariel
Copy link
Member Author

Thanks @flokli I have not yet thought about the module but it makes a lot of sense to get started on that after this gets merged.

Copying people from previous PRs who expressed interest in this being merged:
cc @offlinehacker @costrouc @cawilliamson @volth @risicle @costrouc @Ericson2314

@ingenieroariel
Copy link
Member Author

@jonringer He created the package as part of #65026 and that addition is on a commit made by him that this PR respects.

I could add myself too if that is best practice (I did as much in the main airflow package) but it felt rude to just retroactively claim ownership.

Still want me to add myself? No problem doing so as I want to keep airflow working on unstable.

@jonringer
Copy link
Contributor

I guess I must have overlooked that this was based upon a previous PR, keeping him there is fine, but feel free to add yourself

@jonringer
Copy link
Contributor

I resolved the issues dealing with maintainership

@jonringer
Copy link
Contributor

Generally when adding python-modules, it's roughly one commit = one package addition. @FRidh how strictly do you want to adhere to that role, I think there's 18 or 19 packages being added

@ingenieroariel
Copy link
Member Author

Generally when adding python-modules, it's roughly one commit = one package addition. @FRidh how strictly do you want to adhere to that role, I think there's 18 or 19 packages being added

I squashed the commits to make porting to latest master easier, each package folder is fine but the top-level/python-packages.nix file is problematic when a new package in master falls in the middle of two packages from this branch. If it is strictly needed I can re-create from scratch on top of master but commits may loose correct ownership.

@offlinehacker
Copy link
Contributor

I am not sure with a way this package is packaged, since you override all version requirements, how can you be sure things will work. I did a pull request a while back that used pypi2nix, it generated a large dependency file, but at least I was sure dependencies are right.

@ingenieroariel
Copy link
Member Author

I am not sure with a way this package is packaged, since you override all version requirements, how can you be sure things will work. I did a pull request a while back that used pypi2nix, it generated a large dependency file, but at least I was sure dependencies are right.

This is a good point, I commented below the actual version we ended up choosing (based on what is available in NixOS):

--replace "flask>=1.1.0, <2.0" "flask" \  # got 1.1.1
--replace "flask-caching>=1.3.3, <1.4.0" "flask-caching" \ # got 1.7.2
--replace "flask-appbuilder>=1.12.5, <2.0.0" "flask-appbuilder" \ # got 2.1.6
--replace "pendulum==1.4.4" "pendulum" \ # got 2.0.5
--replace "cached_property~=1.5" "cached_property" \ # got 1.5.1
--replace "dill>=0.2.2, <0.3" "dill" \ # go 0.3.1.1
--replace "configparser>=3.5.0, <3.6.0" "configparser" \ # got 4.0.2
--replace "jinja2>=2.7.3, <=2.10.0" "jinja2" \ # got 2.10.3
--replace "funcsigs==1.0.0" "funcsigs" \ # got 1.0.2
--replace "flask-swagger==0.2.13" "flask-swagger" \ # got 0.2.14
--replace "python-daemon>=2.1.1, <2.2" "python-daemon" \ # got 2.2.3
--replace "alembic>=0.9, <1.0" "alembic" \ # got 1.2.1
--replace "markdown>=2.5.2, <3.0" "markdown" \ # 3.1.1
--replace "future>=0.16.0, <0.17" "future" \ # got 0.18.1
--replace "tenacity==4.12.0" "tenacity" \ # # got 5.1.1
--replace "text-unidecode==1.2" "text-unidecode" \ # got 1.3
--replace "tzlocal>=1.4,<2.0.0" "tzlocal" \ # got 2.0.0
--replace "sqlalchemy~=1.3" "sqlalchemy" \ # got 1.3.10
--replace "werkzeug>=0.14.1, <0.15.0" "werkzeug" # got 0.16.0

I do not expect markdown, sqalchemy, werkzeug or jinja2 to make wild changes on minor updates, but for those apps that made incompatible changes (like pendulum), patches to airflow have been provided. setup.py may look strict but from experience it is always a judgement call on the last person who created a release: in our case, the nixos package maintainer. After that, we only have the test suite to protect us and hopefully some module tests in the near future for integration testing.

@jonringer
Copy link
Contributor

If the versions of the dependencies do need diverge from whats on nixpkgs, then you can move this out of python-modules and create an application. At least there you can wrap the program with it's own pythonpath.

Having more than 1 version in python-modules isn't acceptable due to how environments are created with python packages (both will be present, only 1 will get used)

@ingenieroariel
Copy link
Member Author

If the versions of the dependencies do need diverge from whats on nixpkgs, then you can move this out of python-modules and create an application. At least there you can wrap the program with it's own pythonpath.

Having more than 1 version in python-modules isn't acceptable due to how environments are created with python packages (both will be present, only 1 will get used)

This PR does not add more than one version of any packages, it attempts to reuse what is there. Both a top level application and a python-module are included here. For my project I do need a python-module on NixOS.

@ingenieroariel
Copy link
Member Author

Thanks all for the comments and reviews. Should I do a one package per commit and force push the branch or is this okay as is?

@jonringer
Copy link
Contributor

not really sure, I was hoping @FRidh would make a more opinionated stance

@ingenieroariel
Copy link
Member Author

@jonringer I went ahead and rebased one commit per package onto latest master, preserving original autorship.

@ingenieroariel ingenieroariel changed the title python3Packages.apache-airflow 1.10.5 python3Packages.apache-airflow init 1.10.5 Nov 15, 2019
@jonringer
Copy link
Contributor

jonringer commented Nov 16, 2019

do you mind editing your commit to be:

apache-airflow: 1.10.3 -> 1.10.5, enable python

also, your last commit, touches a lot of files

Includes misc fixes suggested during the PR process.
@ingenieroariel
Copy link
Member Author

apache-airflow: 1.10.3 -> 1.10.5, enable python

Done.

@flokli flokli merged commit 93f5c85 into NixOS:master Nov 18, 2019
@jonringer
Copy link
Contributor

The last commit, 7af3a95 touched around 10 different files, it should have been broken into ~9 commits on what packages it's bump, removing, or altering

@flokli
Copy link
Contributor

flokli commented Nov 18, 2019

@jonringer Urgh, you're right. Should we revert and do a new PR?

@jonringer
Copy link
Contributor

not entirely sure, but if there's a need to cherrypick or remove a package change, then we will have to revert that commit

@jonringer
Copy link
Contributor

reverting, then applying the 9 commits would probably be the most proper course of action

@flokli
Copy link
Contributor

flokli commented Nov 25, 2019

Sorry, lost track about this. I think reverting it causes even more noise. We probably won't backport things from here to 19.09, so it'll hopefully be fine…

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.

None yet

5 participants