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: Switch to lib.makeScope for `overrideScope` support #67422

Closed
wants to merge 1 commit into from

Conversation

@Infinisil
Copy link
Member

@Infinisil Infinisil commented Aug 25, 2019

Motivation for this change

This change allows previously ~final package sets to be overridden with
overlays , e.g. you can do pkgs.python.pkgs.overrideScope' (self: super: { ... })
whereas previously only pkgs.python.override { packageOverrides = (self: super: { ... }) was possible.

This therefore also allows packages definitions to transitively override
dependencies. So e.g. if jupyterlab_server requires jsonschema >= 3, but
the rest of the packages should still use jsonschema == 2, you can
define it as such:

jupyterlab_server = (self.overrideScope' (self: super: {
  jsonschema = self.jsonschema3;
})).callPackage ../development/python-modules/jupyterlab_server { };

This applies the overlay to the whole dependency closure, whereas

jupyterlab_server = callPackage ../development/python-modules/jupyterlab_server {
  jsonschema = self.jsonschema3;
};

would only make it apply to jupyterlab_server itself, but not its
dependencies, resulting in build-time errors. To get the desired effect without
this change, this would be needed for this specific case:

jupyterlab_server = callPackage ../development/python-modules/jupyterlab_server {
  jsonschema = self.jsonschema3;
  notebook = self.notebook.override {
    nbformat = self.nbformat.override {
      jsonschema = self.jsonschema3;
    };
    nbconvert = self.nbconvert.override {
      nbformat = self.nbformat.override {
        jsonschema = self.jsonschema3;
      };
    };
  };
};

Ping @FRidh @JonathanReeve @jonringer @veprbl

Things done
  • Tested that all python override strategies still work
@Infinisil Infinisil requested a review from FRidh as a code owner Aug 25, 2019
@ofborg ofborg bot added the 6.topic: python label Aug 25, 2019
@Infinisil Infinisil mentioned this pull request Aug 25, 2019
1 of 7 tasks complete
@veprbl
Copy link
Member

@veprbl veprbl commented Aug 25, 2019

This provides convenient and consistent dependency overrides. However, the main problem seems to be that there will be a conflict between python packages if the jsonschema and jupyterlab (which brings jsonschema3) will appear in the same closure.

@Infinisil
Copy link
Member Author

@Infinisil Infinisil commented Aug 25, 2019

@veprbl Yeah that is a problem (not a main one), and this PR doesn't solve this (if this can be solved at all).

The biggest reason I made this PR is to make it much more convenient to do transitive overrides, which is possible for a lot of other package sets already because of their use of makeScope.

@jonringer
Copy link
Contributor

@jonringer jonringer commented Aug 25, 2019

What about the case where I need two packages, one needs jsonschema==2.* and the other needs jsonschema>=3. Won't the buildEnv still create a PYTHONPATH that has both versions on it?

Example of a shell.nix:

...
buildInputs = [
  jupyter_server # needs jsonschema_3
  jsonschema_2
];
...
@Infinisil
Copy link
Member Author

@Infinisil Infinisil commented Aug 25, 2019

@jonringer Yeah I think that would create such a conflicting env because of how buildInputs treats python packages, but if you do this it should work:

{
  buildInputs = [
    jupyterlab_server
    (python3.withPackages (p: [ p.jsonschema ]))
  ];
}
@FRidh
Copy link
Member

@FRidh FRidh commented Aug 26, 2019

@Infinisil
Copy link
Member Author

@Infinisil Infinisil commented Aug 26, 2019

@FRidh For jupyterlab to build at all. See the PR description for what would be needed to make it build without this change.

@Infinisil
Copy link
Member Author

@Infinisil Infinisil commented Aug 26, 2019

I guess this is mostly useful for getting end-user applications to build correctly

@jonringer
Copy link
Contributor

@jonringer jonringer commented Aug 26, 2019

@Infinisil in applications, there's a lot more freedom to change python versions, because there's a good guarantee that that the python packages will be wrapped and isolated from other python packages. A common example is the aws-sam-cli package.
However, I think your scope example is a lot more concise than the aws-sam-cli/override example :) 👍

@jonringer jonringer mentioned this pull request Aug 26, 2019
3 of 10 tasks complete
@FRidh
Copy link
Member

@FRidh FRidh commented Aug 27, 2019

@Infinisil
Copy link
Member Author

@Infinisil Infinisil commented Aug 31, 2019

Hm I'm not sure I'm getting what you're saying @FRidh, but maybe it's that if I define jupyterlab in top-level/all-packages.nix, I can apply an overlay to the python package set before using callPackage, therefore achieving the same deep override as shown above, but without having to change anything in the python package set. This would mean pkgs.pythonPackages.jupyterlab would be removed in favor of pkgs.jupyterlab with custom python overrides. Is that what you mean?

I still feel like this functionality could be useful though, and it's already built into the makeScope function which is commonly used for package sets. It essentially allows adding overlays to already constructed package sets, which can make it more ergonomic in some cases, e.g. when you only receive a python package set in a function but need to add an overlay to it and you don't know where it came from.

@FRidh
Copy link
Member

@FRidh FRidh commented Aug 31, 2019

This would mean pkgs.pythonPackages.jupyterlab would be removed in favor of pkgs.jupyterlab with custom python overrides. Is that what you mean?

Correct.

@Infinisil
Copy link
Member Author

@Infinisil Infinisil commented Aug 31, 2019

@FRidh Alright, then let's use that to get jupyterlab to build. You onboard with merging this just for the sake of uniformity of package sets in nixpkgs?

@FRidh
Copy link
Member

@FRidh FRidh commented Aug 31, 2019

In general I am of the opinion that we need an RFC first to detail how the various package sets should look. I do not see any benefit feature-wise of this PR, although uniformity is absolutely desirable. It cannot cause any regressions either so that's good. However, the use case you had in mind cannot be dealt with in this way as discussed. By merging this change I expect more PR's with overrideScope inside the Python package set. That is something I would prefer to avoid.

Given that there's a lot of other ways to do bad things that's not a good enough reason, so I'd say go ahead with it. Just update the commit message to change the self.overrideScope' to e.g. a python3Packages.overrideScope'. We should also document that it should not be used within the Python package set.

This change allows previously ~final package sets to be overridden with
overlays , e.g. you can do `pkgs.python.pkgs.overrideScope' (self: super: { ... })`
whereas previously only `pkgs.python.override { packageOverrides =
(self: super: { ... })` was possible.

This therefore also allows packages definitions to transitively override
dependencies. So e.g. if jupyterlab_server requires jsonschema >= 3, but
the rest of the packages should still use jsonschema == 2, you can
define it as such:

  jupyterlab_server = (python.pkgs.overrideScope' (self: super: {
    jsonschema = self.jsonschema3;
  })).callPackage ../development/python-modules/jupyterlab_server { };

This applies the overlay to the whole dependency closure, whereas

  jupyterlab_server = python.pkgs.callPackage ../development/python-modules/jupyterlab_server {
    jsonschema = self.jsonschema3;
  };

would only make it apply to jupyterlab_server itself, but not its
dependencies, resulting in build-time errors. To get the desired effect without
this change, this would be needed for this specific case:

  jupyterlab_server = python.pkgs.callPackage ../development/python-modules/jupyterlab_server {
    jsonschema = self.jsonschema3;
    notebook = self.notebook.override {
      nbformat = self.nbformat.override {
        jsonschema = self.jsonschema3;
      };
      nbconvert = self.nbconvert.override {
        nbformat = self.nbformat.override {
          jsonschema = self.jsonschema3;
        };
      };
    };
  };
@Infinisil Infinisil force-pushed the Infinisil:python-makeScope branch from c5f7278 to 97a7887 Sep 5, 2019
@Infinisil
Copy link
Member Author

@Infinisil Infinisil commented Sep 5, 2019

I updated the commit message to change self -> python.pkgs (where applicable).

I wouldn't really know where to put docs for this however, and given that this functionality might be needed in some cases (maybe even in the python package set), I wouldn't want to tell people to not use it. Unless you insist on docs I'd like this to be merged so the jupyterlab update can be merged too.

@FRidh
Copy link
Member

@FRidh FRidh commented Sep 6, 2019

The section How to override a Python package? in the Nixpkgs manual could be extended with an example using this and a warning it should not be used from within the Python packages set.

@bb010g
Copy link
Member

@bb010g bb010g commented Oct 23, 2019

Is this just waiting on documentation?

@bb010g
Copy link
Member

@bb010g bb010g commented Oct 27, 2019

Also, for fun (wanting to experiment with mirroring Nixpkgs's structure in a NUR repo), you can actually do this on today's Nixpkgs with a bit of backing horror:

bb010g/nur-packages@2ba91b8

Uncommented, abridged, and slightly modified code taken from 2ba91b8:pkgs/default.nix#L47-87 using #67423 as an example:

let
  # pkgs = self;
  pkgs' = let
    inherit (pkgs) lib;
    pkgsScope = lib.makeScope pkgs.newScope (_: pkgs);
    pythonScopeOverrides = self: super: {
      pythonInterpreters = super.pythonInterpreters.override (o: {
        pkgs = o.pkgs // {
          callPackage = fn: args: o.pkgs.callPackage fn (let
            packageOverrides = pySelf: pySuper:
              pySuper.__extends__ or
                (let pyScope = lib.makeScope self.newScope (self: pySuper // {
                  inherit (self) callPackage;
                }); in if args ? packageOverrides then
                  pyScope.overrideScope' args.packageOverrides
                  else
                    pyScope);

              f = if lib.isFunction fn then fn else import fn;
              in if (lib.functionArgs f) ? packageOverrides then
                args // { inherit packageOverrides; }
              else
                args);
        };
      });
      inherit (self.pythonInterpreters) python37;
      python37Packages = self.python37.pkgs;
      python3 = self.python37;
      python3Packages = self.python3.pkgs;
    };
  in pkgsScope.overrideScope' pythonScopeOverrides;
in { inherit (pkgs'.python3Packages.overrideScope' (self: super: {
  jsonschema = self.jsonschema3;
  jupyterlab = self.callPackage ../applications/editors/jupyterlab { };
  jupyterlab_server = self.callPackage ../servers/jupyterlab { };
})) jupyterlab jupyterlab_server; }
@Infinisil
Copy link
Member Author

@Infinisil Infinisil commented Nov 2, 2019

Neat and hacky!

Yeah I think the only thing missing here is docs wanted by @FRidh, but I'm frightened by xml. Imo leaving out docs for this isn't too bad, because nobody would even know they could now use .overrideScope' without looking at the source code, so no need to point out to people to not use it for python-packages.nix either. We can add docs later still.

@FRidh
Copy link
Member

@FRidh FRidh commented Nov 2, 2019

If you don't want to point it out, why add it?

@Infinisil
Copy link
Member Author

@Infinisil Infinisil commented Nov 2, 2019

It's still useful when you need it (such as for jupyterlab and probably more cases as well), even without docs. Docs would be nice yeah, but I don't even use python or jupyterlab, so I don't really have much motivation for this.

@Infinisil
Copy link
Member Author

@Infinisil Infinisil commented Dec 12, 2019

If anybody else feels like doing this, feel free to use the patch in this PR

@Infinisil Infinisil closed this Dec 12, 2019
@jonringer
Copy link
Contributor

@jonringer jonringer commented Dec 12, 2019

Agreed, the azure-cli python package list would have been a good candidate

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Linked issues

Successfully merging this pull request may close these issues.

None yet

5 participants
You can’t perform that action at this time.