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

pythonPackages.apache-airflow: init at 1.10.3 #65026

Closed
wants to merge 21 commits into from

Conversation

costrouc
Copy link
Member

@costrouc costrouc commented Jul 18, 2019

Motivation for this change

Packaging apache airflow. This PR closes this PR #44028

Things done
  • Tested using sandboxing (nix.useSandbox on NixOS, or option sandbox in nix.conf on non-NixOS)
  • Built on platform(s)
    • 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"
  • 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.

@costrouc costrouc requested a review from FRidh as a code owner July 18, 2019 13:22
@costrouc costrouc force-pushed the python-airflow-init branch from 99dce52 to 4f587fb Compare July 19, 2019 01:03
@costrouc costrouc changed the title [WIP] pythonPackages.apache-airflow: init at 1.10.3 pythonPackages.apache-airflow: init at 1.10.3 Jul 19, 2019
@costrouc
Copy link
Member Author

So... I was able to build on both python2 and python3 and run airflow version without errors! Would appreciate if others would test. I have made every effort to have the tests running for each new package added. Which are quite a few.

@costrouc
Copy link
Member Author

@GrahamcOfBorg build python2Packages.airflow python3Packages.airflow

@costrouc
Copy link
Member Author

costrouc commented Jul 19, 2019

The package is exposed as pythonPackages.apache-airflow and apache-airflow. Thoughts? Should it be airflow (this is not the official pypi name)

@costrouc
Copy link
Member Author

@GrahamcOfBorg build python2Packages.apache-airflow python3Packages.apache-airflow

@ofborg ofborg bot added 8.has: package (new) This PR adds a new package 11.by: package-maintainer This PR was created by the maintainer of the package it changes 10.rebuild-darwin: 11-100 10.rebuild-linux: 11-100 labels Jul 19, 2019
Copy link
Contributor

@jonringer jonringer left a comment

Choose a reason for hiding this comment

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

nix-review passes on NixOS
diff LGTM
binary at least displays usage (not familiar enough with application to test further)

would like to see some unit tests ran, just to make sure that dependency import paths don't change from what airflow assumes there to be.

Not related to this pr:
Someone should upstream the dependency bounds of the dependencies, some of the the code bases at work do this much pinning and it makes my soul hurt (I understand the pain of being broken downstream constantly, but some of these are overly restrictive)

   substituteInPlace setup.py \
     --replace "flask-caching>=1.3.3, <1.4.0" "flask-caching" \
     --replace "flask-appbuilder==1.12.3" "flask-appbuilder" \
     --replace "pendulum==1.4.4" "pendulum" \
     --replace "configparser>=3.5.0, <3.6.0" "configparser" \
     --replace "jinja2>=2.7.3, <=2.10.0" "jinja2" \
     --replace "funcsigs==1.0.0" "funcsigs" \
     --replace "flask-swagger==0.2.13" "flask-swagger" \
     --replace "python-daemon>=2.1.1, <2.2" "python-daemon" \
     --replace "alembic>=0.9, <1.0" "alembic" \
     --replace "markdown>=2.5.2, <3.0" "markdown" \
     --replace "future>=0.16.0, <0.17" "future" \
     --replace "tenacity==4.12.0" "tenacity" \
     --replace "werkzeug>=0.14.1, <0.15.0" "werkzeug"

@costrouc
Copy link
Member Author

costrouc commented Jul 19, 2019

Result of nix-review pr 65026 1

35 package were build:
  • apache-airflow (python37Packages.apache-airflow)
  • python27Packages.apache-airflow (python37Packages.apache-airflow)
  • python27Packages.apispec (python37Packages.apache-airflow)
  • python27Packages.croniter (python37Packages.apache-airflow)
  • python27Packages.flask-admin (python37Packages.apache-airflow)
  • python27Packages.flask-appbuilder (python37Packages.apache-airflow)
  • python27Packages.flask-babelex (python37Packages.apache-airflow)
  • python27Packages.flask-mongoengine (python37Packages.apache-airflow)
  • python27Packages.flask-openid (python37Packages.apache-airflow)
  • python27Packages.json-merge-patch (python37Packages.apache-airflow)
  • python27Packages.marshmallow-enum (python37Packages.apache-airflow)
  • python27Packages.mongoengine (python37Packages.apache-airflow)
  • python27Packages.openapi-spec-validator (python37Packages.apache-airflow)
  • python27Packages.prance (python37Packages.apache-airflow)
  • python27Packages.prison (python37Packages.apache-airflow)
  • python27Packages.python-openid (python37Packages.apache-airflow)
  • python27Packages.sqlalchemy-citext (python37Packages.apache-airflow)
  • python27Packages.sqlalchemy-utils (python37Packages.apache-airflow)
  • python27Packages.wtf-peewee (python37Packages.apache-airflow)
  • python37Packages.apispec (python37Packages.apache-airflow)
  • python37Packages.croniter (python37Packages.apache-airflow)
  • python37Packages.flask-admin (python37Packages.apache-airflow)
  • python37Packages.flask-appbuilder (python37Packages.apache-airflow)
  • python37Packages.flask-babelex (python37Packages.apache-airflow)
  • python37Packages.flask-mongoengine (python37Packages.apache-airflow)
  • python37Packages.flask-openid (python37Packages.apache-airflow)
  • python37Packages.json-merge-patch (python37Packages.apache-airflow)
  • python37Packages.marshmallow-enum (python37Packages.apache-airflow)
  • python37Packages.mongoengine (python37Packages.apache-airflow)
  • python37Packages.openapi-spec-validator (python37Packages.apache-airflow)
  • python37Packages.prance (python37Packages.apache-airflow)
  • python37Packages.prison (python37Packages.apache-airflow)
  • python37Packages.sqlalchemy-citext (python37Packages.apache-airflow)
  • python37Packages.sqlalchemy-utils (python37Packages.apache-airflow)
  • python37Packages.wtf-peewee (python37Packages.apache-airflow)

@jonringer I agree it would be nice to get some unit tests working with apache-airflow. That way we have a better guarentee than airflow version worked. Looks like I can manually select some unit tests for example https://github.com/apache/airflow/blob/16d93c9e45e14179c7822fed248743f0c3fd935c/run_unit_tests.sh#L71. Do you have much experience with nose tests?

@jonringer
Copy link
Contributor

I do not. In all honesty, I prefer not to touch python code. I'm forced to because of work ;(

# remove mockldap requirement
rm flask_appbuilder/tests/_test_ldapsearch.py
nosetests flask_appbuilder.api flask_appbuilder.tests.test_api
'';
Copy link
Contributor

@risicle risicle Jul 19, 2019

Choose a reason for hiding this comment

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

Is it worth leaving these in when they don't actually solve the problem?

@risicle
Copy link
Contributor

risicle commented Jul 19, 2019

35 packages built on macos 10.13, py27 and py37 versions' executables both run. Excellent work here.

@costrouc costrouc force-pushed the python-airflow-init branch from d654f91 to fd7eef6 Compare July 22, 2019 13:41
@costrouc
Copy link
Member Author

@GrahamcOfBorg build python2Packages.airflow python3Packages.airflow

@costrouc
Copy link
Member Author

Ready for merge. Test pass and builds on osx and linux

@costrouc
Copy link
Member Author

Ping. Ready for merge

}:

let # import error "from pendulum import Pendulum" due to 2.x
pendulum1 = pendulum.overrideAttrs (super: rec {
Copy link
Member

Choose a reason for hiding this comment

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

We cannot have multiple versions in python-packages.nix

Copy link
Member Author

Choose a reason for hiding this comment

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

Having the restriction that only 1 version is allowed. Should I create a patch that has support for the newer versions?

Copy link
Member

Choose a reason for hiding this comment

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

Yes, to get this in it needs to support the pendulum version we have in the package set.

Copy link
Member Author

Choose a reason for hiding this comment

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

To get around this restriction could I treat airflow as an application rather than a library? I think this could possibly be a hard patch to maintain

Copy link
Member Author

Choose a reason for hiding this comment

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

And not include it in pythonPackages

Copy link
Contributor

@bhipple bhipple Aug 26, 2019

Choose a reason for hiding this comment

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

Treating airflow itself as an application rather than a library sounds like a good way to go, with all of the dependencies aside from pendulum being regular python packages in the distribution.

I'd really like to be able to use this from NixPkg and appreciate the work the two of you are doing, btw!

@FRidh FRidh added the 2.status: work-in-progress This PR isn't done label Jul 28, 2019
@costrouc
Copy link
Member Author

@bhipple I am happy to package this simply as an application. Does this work with you @FRidh?

I am sure this question will come up once it is packaged as an application. How would one include additional packages with airflow then?

@jonringer
Copy link
Contributor

@costrouc if it's very unlikely that another package will ever need to import this (E.g. it's an application), then I would package it as just an application, and may move it out of python modules (to discourage trying to import it). But I would still wait for @FRidh to input, as he probably has better insights into the correct actions.

@costrouc
Copy link
Member Author

On a similar note I have a devious plan for "multiple" python versions.

Python only allows one version of each package at a time (whatever is found in the python path first). What if a tool was written to rewrite import statements for a given package? For example import numpy -> import ab12abc45_numpy_1_16 for example. This would allow there to be many versions of numpy being used in the same environment.

This would allow multiple versions of a packages 😁. i would like to see what would happen with a tool like this.

@jonringer
Copy link
Contributor

On a similar note I have a devious plan for "multiple" python versions.

Python only allows one version of each package at a time (whatever is found in the python path first). What if a tool was written to rewrite import statements for a given package? For example import numpy -> import ab12abc45_numpy_1_16 for example. This would allow there to be many versions of numpy being used in the same environment.

This would allow multiple versions of a packages grin. i would like to see what would happen with a tool like this.

I'm assuming it would be a lot of complexity and hair-pulling in constructing a dependency house of cards.

However, if successful, i think it would be really convenient to use when there's mutually exclusive version bounds in the same closure.

@costrouc
Copy link
Member Author

Yes I agree that It could add to complexity and would need to be very explicit what is happening. Having something like this would be extremely helpful in these cases were we need to break the rules of multiple packages in nixpkgs. For example here I only want to change the verisons of two packages https://github.com/NixOS/nixpkgs/blob/fd7eef6dcf212e8e164f2201b7e58bb63247f290/pkgs/development/python-modules/apache-airflow/default.nix#L53-L73

@jonringer
Copy link
Contributor

@costrouc you may also want to take a look at #67422 then

@costrouc costrouc force-pushed the python-airflow-init branch from 3046b43 to 1f39697 Compare August 27, 2019 18:21
@ofborg ofborg bot removed the 2.status: merge conflict This PR has merge conflicts with the target branch label Aug 27, 2019
@costrouc
Copy link
Member Author

costrouc commented Aug 27, 2019

Okay so this PR is a working example of rewriting imports with https://github.com/nix-community/nixpkgs-pytools/#python-rewrite-imports. It allows multiple versions of python packages existing within the same PYTHONPATH.

How does it work? It uses rope https://github.com/python-rope/rope to refactor certain imports. Rope makes this approach quite robust. Here is an example of how it correctly refactors chaning the numpy import https://github.com/nix-community/nixpkgs-pytools/blob/master/tests/test_import_rewrite.py.

I have left the "tricks" done in rewriting the imports as explicit as possible in this PR. In hopes that if others think this is worth moving forward with we can create a wrapper function that will automatically apply these changes in a more user friendly way. This approach should have minimal impact on libraries that depend on it. Personally I think this is a feasable approach to different versions of python package coexisting. Would love to hear others thoughts.

In this PR I demonstrate that you can have packages that would normally break airflow in the propagated build inputs (pendulum, flask-appbuilder). Because of the import rewrites airflow does not use these packages and allows downstream packages to use the latest.

@FRidh what are your thoughts? Would this be an okay approach to having several versions of a python package available? (Obviously having 1 version is ideal and we should always strive for the latest).

@nixos-discourse
Copy link

This pull request has been mentioned on NixOS Discourse. There might be relevant details there:

https://discourse.nixos.org/t/scaling-the-python-package-set-in-nixpkgs/3749/18

@costrouc costrouc force-pushed the python-airflow-init branch from 1f39697 to 907fe9c Compare August 27, 2019 18:42
@costrouc
Copy link
Member Author

@GrahamcOfBorg build python2Packages.apache-airflow python3Packages.apache-airflow

@FRidh
Copy link
Member

FRidh commented Aug 28, 2019

@costrouc please open a separate ticket or PR as demo, don't do it in something you actually want to get merged. Also, make it a minimal working example. Furthermore, post it on ideas or packaging to get additional feedback.

@Ericson2314
Copy link
Member

Ericson2314 commented Oct 15, 2019

@FRidh Does the PR still have the controversial multiple versions? Or anything else you want changed? I might take this over so just tell me what to do :)

@rasendubi
Copy link
Member

#73074 adds apache-airflow-1.10.5 and has been merged, so this can be closed?

@jonringer jonringer closed this Nov 19, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
2.status: work-in-progress This PR isn't done 6.topic: python 8.has: package (new) This PR adds a new package 10.rebuild-darwin: 11-100 10.rebuild-linux: 11-100 11.by: package-maintainer This PR was created by the maintainer of the package it changes
Projects
None yet
Development

Successfully merging this pull request may close these issues.

8 participants