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

Add an "addPackages" utility to python buildEnv #97467

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

trevorcook
Copy link

Motivation for this change

This change adds the ability to add packages to an existing python env. That is, if s1 and s2 are some functions that select python packages, and py1 = python.withPackages s1 is a python environment, this change allows py2 = py1.addPackages s2. The resulting environment is the same as python.withPackages s1-and-s2, where s1-and-s2 is the function that selects the packages from s1 and s2.

This feature would help provide more modular python environments. I'm developing a nix lib for modular and portable shell environments, env-th. A change such as the one proposed here would allow multiple such shell environments to be merged together.

Things done

I've tested this in the nix repl. Below shows a session where I defined a python environment with numpy and pyzmq in two ways. The first way, py2a, uses a withPackages. The second way, py2b, uses the new utility. I show in the output below that they return the same derivation.

trevorcook:~/nixpkgs$ nix repl ./default.nix
Welcome to Nix version 2.3.2. Type :? for help.

Loading 'default.nix'...
Added 12433 variables.
nix-repl> s1 = pkgs : [pkgs.numpy]

nix-repl> s2 = pkgs : [pkgs.pyzmq]

nix-repl> s1-and-s2 = pkgs: [pkgs.numpy pkgs.pyzmq]

nix-repl> py1 = python37.withPackages s1

nix-repl> py2a = python37.withPackages s1-and-s2

nix-repl> py2a
«derivation /nix/store/fpn8lnk8mqx966q6lhzf1a1ry1fgp3qf-python3-3.7.9-env.drv»

nix-repl> py2b = py1.addPackages s2

nix-repl> py2b
«derivation /nix/store/fpn8lnk8mqx966q6lhzf1a1ry1fgp3qf-python3-3.7.9-env.drv»

nix-repl> py2a == py2b
true

nix-repl> :b py2b

this derivation produced the following outputs:
  out -> /nix/store/4qcf02ycgxzlawb2ms7mgqv5szc5mz0v-python3-3.7.9-env

This is a new feature, so there shouldn't be a problem with breaking existing packages. However, it is a change to a very significant feature, so I do expect a lot of scrutiny. For one, there is already a shortcoming in my approach in that it will only
work with environments made with withPackages. Changing the buildEnv attribute would need to be targeted otherwise and I haven't found a clean way to do that yet.

  • Tested using sandboxing (nix.useSandbox on NixOS, or option sandbox in nix.conf on non-NixOS linux)
  • 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 nixpkgs-review --run "nixpkgs-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.

@FRidh
Copy link
Member

FRidh commented Sep 9, 2020

I am against adding this to withPackages because this can already be done with buildEnv. withPackages is meant as just a light interface to be typically used. If it does not suffices, one should use buildEnv directly.

with import (fetchTarball "channel:nixos-20.03") {};

with python3.pkgs;

let
  #env = python3.buildEnv.override {
  #  extraLibs = [ pytest ];
  #};
  env = python3.withPackages(ps: with ps; [ pytest ]);  # Equivalent to the commented `buildEnv.override`
  env2 = env.override (old: {
    extraLibs = old.extraLibs ++ [ numpy ];
  });
in env2

@trevorcook
Copy link
Author

Thanks FRidh for reviewing this and for the alternative. I've tried for a few hours to provide a low code-impact replacement based on your suggestion, but what I am coming up with seems to need to something like the following added to python/default.nix:

passthruFun = ... 
  envBuild =add-AddPackages (callPackage ./wrapper.nix { python = self; inherit (pythonPackages) requiredPythonModules; };
  withPackages = add-addPackages (import ./with-packages.nix { inherit buildEnv pythonPackages;};)

I've not got it working, and I need to get on to other things, so I wanted to ask if it is worth pursuing. You said you are against adding this to withPackages. Does this mean you have some issues just with the implementation i proposed, or is it more along the lines that you think an addPackages type extension is the wrong move in the first place? If its worth pursuing, could you give some guidance on what would be preferrable? Integrated into buildEnvs wrapper.nix? A separate add-packages.nix that would modify the passthruFun.buildEnv and passthruFun.withPackages?

@stale
Copy link

stale bot commented Mar 16, 2021

I marked this as stale due to inactivity. → More info

@stale stale bot added the 2.status: stale https://github.com/NixOS/nixpkgs/blob/master/.github/STALE-BOT.md label Mar 16, 2021
@thomasjm
Copy link
Contributor

thomasjm commented May 5, 2023

FWIW, I really think chained .withPackages calls should be additive and it's currently a bug that they are not. It's surprising behavior when (python.withPackages f).withPackages g only contains packages from g.

IMO the addition of a separate addPackages isn't the solution and we should just change .withPackages.

I was able to make it work using the override of extraLibs as suggested in #97467 (comment), but this isn't very ergonomic.

@stale stale bot removed the 2.status: stale https://github.com/NixOS/nixpkgs/blob/master/.github/STALE-BOT.md label May 5, 2023
@nixos-discourse
Copy link

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

https://discourse.nixos.org/t/override-function-no-longer-accepts-parameter-in-python-env/40964/1

@wegank wegank added the 2.status: stale https://github.com/NixOS/nixpkgs/blob/master/.github/STALE-BOT.md label Mar 19, 2024
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

6 participants