Skip to content

Python: support installing wheels#15010

Merged
zimbatm merged 2 commits intoNixOS:masterfrom
FRidh:format
May 9, 2016
Merged

Python: support installing wheels#15010
zimbatm merged 2 commits intoNixOS:masterfrom
FRidh:format

Conversation

@FRidh
Copy link
Member

@FRidh FRidh commented Apr 26, 2016

This PR adds support for installing wheels. The parameter format can be used to specify the source is a wheel.

While I imagine it would be possible to determine that it is a wheel and adjust the build process accordingly, the reason behind having an explicit option like this is because I would like to support Flit packages, and therefore a third option would be added, format="flit" (see #14945).

cc @grahamc @domenkozar

PS: here is already a first limitation of #15001, we cannot link wheels.

@FRidh FRidh added 0.kind: enhancement Add something new or improve an existing system. 6.topic: python Python is a high-level, general-purpose programming language. 9.needs: reporter feedback This issue needs the person who filed it to respond labels Apr 26, 2016
@mention-bot
Copy link

By analyzing the blame information on this pull request, we identified @domenkozar, @chaoflow and @edolstra to be potential reviewers

Copy link
Contributor

Choose a reason for hiding this comment

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

Looks like this comment is misplaced

@adnelson
Copy link
Contributor

To be honest I'm not sure that that one package entrypoint warrants an overhaul of the buildPythonPackage function. In general the whole raison d'être of wheels is obviated by nix anyway, so I can't imagine many packages will need them (the fact that we haven't needed them yet is telling). In the case of this one package, simply fetching from git, and/or using mkDerivation would be fairly straightforward.

@domenkozar
Copy link
Member

I'll review this shortly, trying to catch a deadline now :)

@FRidh
Copy link
Member Author

FRidh commented Apr 28, 2016

As I explained in the opening post, my intention is to add also support for Flit packages (#14945).
These are simple Python-only packages with a declarative specification of its dependencies. So far all Flit packages that I've needed were published on PyPI as wheels.

Therefore, we could get away with adding support for installing wheels, but not Flit packages. In that case, it is indeed possible to detect whether src is a wheel as @adnelson suggested.

Because wheels-only packages are still so rare, I'm fine with just adjusting the expressions for the specific packages. We can always consider this again later.

@zimbatm
Copy link
Member

zimbatm commented May 5, 2016

Just as an anecdote, recently I was trying to package a wheel-only package (bash_kernel which depends on flit actually) and it was a nightmare.

@FRidh
Copy link
Member Author

FRidh commented May 6, 2016

The wheels that were created with flit that I've encountered so far were
all packaged by the author of flit, which is also the case in your example.

On Thu, May 5, 2016 at 11:00 PM, zimbatm notifications@github.com wrote:

Just as an anecdote, recently I was trying to package a wheel-only package
(bash_kernel which depends on flit actually) and it was a nightmare.


You are receiving this because you authored the thread.
Reply to this email directly or view it on GitHub
#15010 (comment)

@FRidh FRidh mentioned this pull request May 6, 2016
7 tasks
@zimbatm
Copy link
Member

zimbatm commented May 6, 2016

@FRidh I've fixed it all over here, let me know what you think: zimbatm@a5c2777

@FRidh
Copy link
Member Author

FRidh commented May 6, 2016

@zimbatm 's branch is https://github.com/zimbatm/nixpkgs/commits/python-wheels
I'm glad you've improved it even further.

What do you think about detecting the format and choosing wheel/setuptools based on that? I like that approach although, thinking a bit further about when we would like to have e.g. generated lists of sources/hashes, it would be nice to be able to choose what to pick - wheel or setuptools source.

@zimbatm
Copy link
Member

zimbatm commented May 6, 2016

Thanks, I'm quite happy with the current solution for now and much prefer explicit things than heuristics.

If I were to improve this further I think splitting the derivation in two, a pythonSetuptoolsPackage and a pythonWheelsPackage might make more sense. They don't really share a lot of things anyways. Then on top of that we could have a heuristic that generates one of either based on the url file extension.

@FRidh
Copy link
Member Author

FRidh commented May 6, 2016

@zimbatm actually, I think it would make sense then to have a functions to build wheels (flit, setuptools) and a function to install a wheel. Anyway, I can push your version to this PR so we can discuss it here further, or you open a PR. Both are fine with me.

@zimbatm
Copy link
Member

zimbatm commented May 6, 2016

Yeah feel free to amend your PR with my changes. Good idea on keeping the discussion here.

FRidh added 2 commits May 9, 2016 13:17
By passing `format="wheel"` it is now possible to install a wheel.
It's not recommended, but it can be useful.
@FRidh
Copy link
Member Author

FRidh commented May 9, 2016

I just pushed https://github.com/zimbatm/nixpkgs/commits/python-wheels by @zimbatm in here and added to the docs the new option.
A Hydra is computing all Python packages: https://headcounter.org/hydra/jobset/iElectric/python-wip#tabs-evaluations.

I think it is good to merge like this.

@zimbatm zimbatm merged commit 428db78 into NixOS:master May 9, 2016
# a common idiom in Python
#
# For backwards compatibility, let's use an alias
doInstallCheck = attrs.doCheck or false;
Copy link
Member Author

Choose a reason for hiding this comment

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

If I am correct this should actually be attrs.doCheck or true, because like this we disable the default testing.

@colemickens
Copy link
Member

Can someone point me at a simple example of a python package that leverages this work and installs from a wheel? I'm anxious to fix #15041 so I can finally update my system to the latest channels/nixos-unstable.

@FRidh
Copy link
Member Author

FRidh commented May 9, 2016

@colemickens check 303e976 which is part of this PR.
Note that I've only tested Python-only wheels. I don't know how well it works when you use compiled code in wheels.

@FRidh
Copy link
Member Author

FRidh commented May 9, 2016

@colemickens obviously I picked the wrong commit. This is the right one 3c0dc7a
It works exactly as other packages, all you need to add is format = "wheel".

@FRidh FRidh mentioned this pull request May 10, 2016
7 tasks
@FRidh FRidh deleted the format branch November 4, 2016 12:26
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

0.kind: enhancement Add something new or improve an existing system. 6.topic: python Python is a high-level, general-purpose programming language. 9.needs: reporter feedback This issue needs the person who filed it to respond

Projects

None yet

Development

Successfully merging this pull request may close these issues.

6 participants