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

Python: Fixed-point combinator #17428

Closed
wants to merge 1 commit into from
Closed

Python: Fixed-point combinator #17428

wants to merge 1 commit into from

Conversation

FRidh
Copy link
Member

@FRidh FRidh commented Aug 1, 2016

Motivation for this change

Use fixed-point combinator for python-packages.nix.

Follow-up of #16784.

  • use combinator
  • fix Python 3 buildEnv
  • document changes, this will break current overriding
  • pythonFull

@FRidh FRidh self-assigned this Aug 1, 2016
@mention-bot
Copy link

@FRidh, thanks for your PR! By analyzing the annotation information on this pull request, we identified @bennofs, @domenkozar and @vcunat to be potential reviewers

@FRidh FRidh changed the title Fixed-point combinator Python: Fixed-point combinator Aug 1, 2016
@FRidh
Copy link
Member Author

FRidh commented Aug 1, 2016

I have a strange problem. The following works

$ nix-shell -E 'with import <nixpkgs> {}; (pkgs.pythonNew.cpython27.withPackages (ps: [ps.py])).env' -I nixpkgs=~/Code/libraries/nixpkgs

but when I call with any Python 3.x interpreter I get

$ nix-shell -E 'with import <nixpkgs> {}; (pkgs.pythonNew.cpython35.withPackages (ps: [ps.py])).env' -I nixpkgs=~/Code/libraries/nixpkgs
these derivations will be built:
  /nix/store/mwshhi78jabxskfxbw51lid90maz9ls0-python3-3.5.2-env.drv
building path(s) ‘/nix/store/x2skcfchcin4ndiyhivvcqvzlpbl1hsz-python3-3.5.2-env’
collision between `/nix/store/06af6d830iawfqbla1vjzckpzmyqswvi-python3-3.5.2/bin/easy_install-3.5' and `/nix/store/f0spcxz7h00mncf3gn8x81ng46vhdzl2-python3.5m-setuptools-19.4/bin/easy_install-3.5'
builder for ‘/nix/store/mwshhi78jabxskfxbw51lid90maz9ls0-python3-3.5.2-env.drv’ failed with exit code 25
error: build of ‘/nix/store/mwshhi78jabxskfxbw51lid90maz9ls0-python3-3.5.2-env.drv’ failed
/run/current-system/sw/bin/nix-shell: failed to build all dependencies

It does work correctly with pypy27.

if isPy34 then "python34" else
if isPy35 then "python35" else
if isPy36 then "python36" else
if isPyPy then "pypy" else "";
Copy link
Member

@lsix lsix Aug 1, 2016

Choose a reason for hiding this comment

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

Maybe something like

else (assert false; "");

I do not see a situation where none of the previous cases evaluates to true.

Copy link
Contributor

Choose a reason for hiding this comment

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

throw "<some descriptive error message>" would be better in this case. But could we perhaps just use python.name (which should be something like "python-2.6" etc)? The problem with hardcoding pythonName here is that it is difficult for people to add a new interpreter locally (say they have packaged python3.7 and want to test this out), because they have to edit nixpkgs to add a new interpreter.

Copy link
Member Author

Choose a reason for hiding this comment

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

Note that this is what we have currently in python-packages.nix.

A git grep showed we actually don't use pythonName anywhere so I will remove it.

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 its gone 2988112

@lsix
Copy link
Member

lsix commented Aug 1, 2016

pkgs/development/python-modules/default.nix line 4 and 22 have commented lines. Should those be removed?

pythonPackages = self:
let
python = interpreter;
in import ./support.nix { inherit pkgs python; } self;
Copy link
Contributor

Choose a reason for hiding this comment

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

I think the following would be a better way to write this:

let
...
pythonPackages = import ./support.nix {
  inherit pkgs; 
  python = interpreter;
};

Avoids the let, so it is more clear that the python is only used to pass to support.nix.

@FRidh
Copy link
Member Author

FRidh commented Aug 2, 2016

Thanks for the feedback. What do you think about the splitting up of the packages (python-packages.nix) and support.nix? It's not really necessary in my opinion but its nice for clarity to have it separated I think.

@bennofs
Copy link
Contributor

bennofs commented Aug 2, 2016

I don't have a strong opinion either way.

@FRidh
Copy link
Member Author

FRidh commented Aug 2, 2016

@bennofs do you have any idea what is causing #17428 (comment)?

@@ -5878,44 +5878,25 @@ in
};
purePackages = recurseIntoAttrs (callPackage ./pure-packages.nix {});

pythonNew = callPackage ./python-new-packages.nix {};
Copy link
Member Author

@FRidh FRidh Aug 2, 2016

Choose a reason for hiding this comment

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

unless python wouldn't point to the interpreter anymore, I think a better name is needed here. Any suggestions? Another possibility is to directly include the contents of python-new-packages.nix into all-packages.nix. That way you have pkgs.cpython27.pkgs, pkgs.pypy27.buildEnv, etc.

Copy link
Contributor

Choose a reason for hiding this comment

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

One advantage of this name is that it is consistent with haskell: for Haskell, haskell also does not point to the compiler but there is haskell.compiler.ghc7103 for example. Of course, with Haskell, it is more clear since the compiler and the language have different names.

@FRidh FRidh force-pushed the fixpoint branch 2 times, most recently from 07bcd2f to 9e98d32 Compare August 2, 2016 09:43
FRidh added a commit that referenced this pull request Aug 2, 2016
Both python3 and setuptools come with easy-install. For some magic
reason this hasn't caused any collisions yet, but it does with #17428.
We hereby prioritize the version that comes with setuptools.
@FRidh
Copy link
Member Author

FRidh commented Aug 2, 2016

#17428 (comment) is solved with 8fad3e8

@FRidh FRidh added the 9.needs: reporter feedback This issue needs the person who filed it to respond label Aug 16, 2016
@FRidh FRidh force-pushed the fixpoint branch 3 times, most recently from f2c74eb to 7be184f Compare August 17, 2016 12:25
@FRidh
Copy link
Member Author

FRidh commented Aug 17, 2016

I've introduced an extra function for building Python packages that do not come with setup.py.

@layus
Copy link
Member

layus commented Aug 17, 2016

I have tried this PR, and I am hitting some chicken and egg problem. Documenting pythonNew may seem useless because test results may trigger changes, but testing without documentation is... hard :-).

Anyway, this PR breaks pythonPackages.lti. This package is interesting because it needs to be built in a different scope, where pytest is pytest_27 instead of the default pytest_29.
Apparently the new self has no override attribute anymore. How can I override self from within pkgs/top-level/python-packages.nix ?

@FRidh
Copy link
Member Author

FRidh commented Aug 17, 2016

@layus thanks for checking it out.

Calling pkgs/development/interpreters/python/interpreter.nix returns a set (I need a name for this set!) that includes a package set. You can give interpreter.nix overrides.

I will add to the set which has no name yet a small function that allows you to easily build a new set for that interpreter. I think that would be very convenient.

I'm a bit short in time atm but will get back to you!

@layus
Copy link
Member

layus commented Aug 17, 2016

I need a name for this set!
Why not interpreter ?

I had a simple hack implemented in https://gist.github.com/layus/311fb6cb9d6c83dadb1e1522c6caa9ee. I know it only allows to override interpreter.pkgs and not interpreter itself, but it is also quite simple, and does not require naming/exposing the aforementioned set.

@FRidh
Copy link
Member Author

FRidh commented Aug 17, 2016

Why not interpreter ?

Well, right now we have pythonNew.cpython.python.interpreter. Do you know what each attribute is? 😄
Many derivations still use python, the actual interpreter, which is in this case pythonNew.cpython.python. I hope that eventually we have python = pythonNew.cpython, which is then a interpreter. When you need the actual interpreter it would then be python.python.

I had a simple hack implemented in https://gist.github.com/layus/311fb6cb9d6c83dadb1e1522c6caa9ee. I know it only allows to override interpreter.pkgs and not interpreter itself, but it is also quite simple, and does not require naming/exposing the aforementioned set.

It should definitely still be possible to do such overrides within the package set, not being able to is a bug! (There are still plenty issues with this.) If you do need to override the interpreter you will inevitably get a new package set in which case you can use what I wrote.

@layus
Copy link
Member

layus commented Aug 19, 2016

Well, right now we have pythonNew.cpython.python.interpreter. Do you know what each attribute is? 😄

I would go with the following attributes hierarchy (inspired by haskell):

pythonNew.interpreters.{cpythonXY,pypy,...}.{buildPythonPackage,mkDerivation,wrapPython,...}
pythonNew.packages.{cpythonXY,pypy,...}.{django_1_9,ipython,...}

pythonXXPackages -> pythonNew.packages.cpythonXX
pythonPackages -> python27Packages

pythonXX -> pythonNew.interpreters.cpythonXX
python -> python27

For completeness, you could even add cross-references:

pythonNew.interpreters.{cpythonXY,pypy,...}.pkgs == pythonNew.packages.{cpythonXY,pypy,...}
pythonNew.packages.{cpythonXY,pypy,...}.interpreter == pythonNew.interpreters.{cpythonXY,pypy,...}

Of course, this means that python.pkgs.interpreter.pkgs.interpreter is valid, but not necessarily a problem.

Finally, pythonNew.interpreters.cpython27 could be both an interpreter set (buildPythonPackage, mkDerivation, wrapPython, etc.) and a derivation. This is possible in nix, and would avoid the extra python field in pythonNew.interpreters.cpython27.python.

@FRidh: Is there any way I can help you with this PR ?

@FRidh
Copy link
Member Author

FRidh commented Aug 19, 2016

@FRidh: Is there any way I can help you with this PR ?

Thanks for the offer! The biggest issue at the moment is deciding on exactly how this should look like, before moving on.

I would go with the following attributes hierarchy (inspired by haskell):

I considered that first (I just copied it as a starting point) but whereas Haskell has multiple compilers and multiple package sets, with Python we have multiple interpreters and only one package set. Therefore I dropped

pythonNew.interpreters.
pythonNew.packages.

and went with

pythonNew.<interpreter>.pkgs

instead.

Finally, pythonNew.interpreters.cpython27 could be both an interpreter set (buildPythonPackage, mkDerivation, wrapPython, etc.) and a derivation. This is possible in nix, and would avoid the extra python field in pythonNew.interpreters.cpython27.python

Indeed, and that is basically what we do currently with the interpreters and buildEnv / withPackages added to the passthru. I like that, however, it doesn't make it possible to build an interpreter without a package set. I wonder whether that really would be a problem.

  • pythonNew.cpython27.pkgs package set
  • pythonNew.cpython35.withPackages to build an env with packages
  • pythonNew.pypy.buildEnv to build an env
  • pythonNew.pypy27.executable name of Python executable

Also, what do you think of actually putting the interpreters directly in top-level, that is, merging the two sets so we get pkgs.cpython27.pkgs

@layus
Copy link
Member

layus commented Aug 19, 2016

pythonNew.<interpreter>.pkgs makes total sense.


I like that, however, it doesn't make it possible to build an interpreter without a package set. I wonder whether that really would be a problem.

Who cares? Nix is lazy enough avoid building the package set if you don't need it. It is only a potential package set.


  • pythonNew.cpython27.pkgs package set
  • pythonNew.cpython35.withPackages to build an env with packages
  • pythonNew.pypy.buildEnv to build an env

Okay

  • pythonNew.pypy27.executable name of Python executable

Huh ? Do you mean a string like "pypy27" ? In that case we already have pythonNew.pypy27.name as pythonNew.pypy27 is a proper derivation.


Also, what do you think of actually putting the interpreters directly in top-level, that is, merging the two sets so we get pkgs.cpython27.pkgs

Not sure what you mean here. I have considered putting packages directly under pythonNew.cpython27, as in pythonNew.cpython27.django. I like it, but it seems rather bold, and could easily conflict with some attributes of the pythonNew.cpython27 derivation.

As for removing the pythonNew prefix, it looks like a great thing, but we may break many packages, right ?

@layus
Copy link
Member

layus commented Aug 19, 2016

Let's denote . the root of nixpkgs attributes hierarchy. We then have .pythonNew.python.

If you want something more flat, we could even go with .cpython35.django.
In this case, .python35 would be a package set augmented with utilities like withPackages and buildEnv. The real python interpreter would be located in .cpython35.python.

How badly would we break nixpkgs with this scheme ?

@FRidh
Copy link
Member Author

FRidh commented Aug 19, 2016

Who cares? Nix is lazy enough avoid building the package set if you don't need it. It is only a potential package set.

Its more that if you want to build just the interpreter you have to pass in something looking like a package set. Or, actually, we build the package set inside the derivation of the interpreter. That way you can easily override the interpreter without fighting to get the package set to use the updated interpreter.

Huh ? Do you mean a string like "pypy27" ? In that case we already have pythonNew.pypy27.name as pythonNew.pypy27 is a proper derivation.

I was referring to one of the attributes that is currently passed through.

As for removing the pythonNew prefix, it looks like a great thing, but we may break many packages, right ?

No, we won't, since we don't use the cpython prefix anywhere, and furthermore, if we would make pkgs an attribute of the interpreter not much really changes for other expressions.

If you want something more flat, we could even go with .cpython35.django.
In this case, .python35 would be a package set augmented with utilities like withPackages and buildEnv. The real python interpreter would be located in .cpython35.python.

How badly would we break nixpkgs with this scheme ?

Nothing really, if you pass .cpython27 instead of what is now .pythonPackages. But my preference would be to have a separate attribute pkgs since I find adding packages directly to the passthru of the interpreter rather messy.

@FRidh FRidh added the 2.status: merge conflict This PR has merge conflicts with the target branch label Sep 1, 2016
@FRidh FRidh force-pushed the fixpoint branch 7 times, most recently from 83dd12d to ebf0b8a Compare September 26, 2016 15:13
@FRidh
Copy link
Member Author

FRidh commented Sep 26, 2016

I've simplified the changes somewhat. It doesn't work entirely yet, but I think it is an improvement nevertheless.

The following example shows how to build scipy with an older version of numpy

python35.override{ pkgOverrides =  self: super: {numpy = self.numpy_1_10;} ;}).pkgs.scipy

and this derivation can be build with

$ nix-build -E 'with import <nixpkgs>{}; (pkgs.python35.override{ pkgOverrides =  self: super: {numpy = self.numpy_1_10;} ;}).pkgs.scipy

Furthermore, python.buildEnv is currently broken, no idea why.

@layus , lti doesn't work yet work. I guess I have to find the right spot to apply makeOverridable.

@FRidh
Copy link
Member Author

FRidh commented Oct 28, 2016

@layus in order to find a solution to lti I made a commit that uses the fixed-point combinator without any additional changes. Thinking about it, I don't think it is possible to override the package set like you did.

I had a look at the Haskell package set for a solution. Like in all-packages.nix each expression is a function with all dependencies as arguments (see hackage-packages.nix). That way, just overriding the function can give you a derivation with say a different version of a dependency. Furthermore, the derivations are given an attribute overrideScope, which allows you to evaluate all packages with a different version of a dependency.

This we should be able to get working with python-packages.nix as well. Adding a function definition to each expression is going to take a lot of time, although maybe we could use the ellipsis. I don't plan to work on this anymore anytime soon so I am closing this.

@FRidh FRidh closed this Oct 28, 2016
FRidh added a commit to FRidh/nixpkgs that referenced this pull request Dec 5, 2016
Use a fixed-point combinator for the Python package set to allow easier overriding of its contents.
Earlier implementations were proposed in NixOS#16784 and NixOS#17428. This commit is by comparison much smaller
and changes only what is needed.
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 2.status: merge conflict This PR has merge conflicts with the target branch 2.status: work-in-progress This PR isn't done 6.topic: python 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.

5 participants