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

[Do Not Merge [yet]] Update jupyterlab #67423

Closed
wants to merge 5 commits into from

Conversation

infinisil
Copy link
Member

@infinisil infinisil commented Aug 25, 2019

Motivation for this change

Do not merge yet, this is dependent on #67422

Closes #65625

This introduces jsonschema3 in parallel to jsonschema (version 2), because apparently a lot of python packages probably still depend on v2

Ping @FRidh @JonathanReeve

Things done
  • Tested that the new version actually works. I'd like @JonathanReeve to do that because I'm not a jupyterlab user
  • Built using sandboxing
  • 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.

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;
        };
      };
    };
  };
@FRidh
Copy link
Member

FRidh commented Aug 25, 2019 via email

@infinisil
Copy link
Member Author

@FRidh Why that? We need v3 for jupyterlab which is pretty popular. Would you prefer for v2 to be removed and replaced by v3?

@mmahut
Copy link
Member

mmahut commented Aug 25, 2019

@GrahamcOfBorg build python37Packages.jupyterlab

@infinisil
Copy link
Member Author

Ah whoops, I only built jupyterlab_server previously, now jupyterlab itself also works

@mmahut
Copy link
Member

mmahut commented Aug 25, 2019

@GrahamcOfBorg build python37Packages.jupyterlab

@infinisil infinisil changed the title Update jupyterlab [Do Not Merge [yet]] Update jupyterlab Aug 25, 2019
@rehno-lindeque
Copy link
Contributor

rehno-lindeque commented Sep 2, 2019

I seem to be having some initial success trying this approach suggested by @costrouc.

Quick/rough experiment looked like this:

let
  packageOverrides = selfPythonPackages: pythonPackages: {
    # v1.3.0 not yet in nixpkgs
    nixpkgs-pytools =
      selfPythonPackages.callPackage (
        { lib
        , buildPythonPackage
        , fetchPypi
        , jinja2
        , setuptools
        , isPy27
        }:

        buildPythonPackage rec {
          pname = "nixpkgs-pytools";
          version = "1.3.0";
          disabled = isPy27;

          src = fetchPypi {
            inherit pname version;
            sha256 = "11skcbi1lf9qcv9j5ikifb4pakhbbygqpcmv3390j7gxsa85cn19";
          };

          propagatedBuildInputs = [
            jinja2
            setuptools
            selfPythonPackages.rope
          ];

          # tests require network ..
          doCheck = false;
        }) {};

    jsonschema_3_0_2 = pythonPackages.buildPythonPackage rec {
      pname = "jsonschema";
      version = "3.0.2";

      src = pythonPackages.fetchPypi {
        inherit pname version;
        sha256 = "03anb4ljl624lixrsaxfi7i1iwavw39sd8cfkhcy0dr2dixjnjld";
      };

      nativeBuildInputs = with selfPythonPackages; [ setuptools_scm nixpkgs-pytools ];
      checkInputs = with selfPythonPackages; [ nose mock vcversioner twisted perf ];
      propagatedBuildInputs = with selfPythonPackages; [ functools32 attrs pyrsistent ];

      # Rewrite imports, alter setup.cfg, replace pkgutil.get in _util.py
      postPatch = with selfPythonPackages; ''
        substituteInPlace jsonschema/tests/test_jsonschema_test_suite.py \
          --replace "python" "${pkgs.python3.pythonForBuild.interpreter}"

        substituteInPlace setup.cfg \
          --replace 'name = jsonschema' 'name = jsonschema_${src.outputHash}' \
          --replace 'jsonschema = schemas/*.json' 'jsonschema_${src.outputHash} = schemas/*.json'

        # Unsure if these are necessary
        # --replace 'jsonschema = jsonschema.cli:main' 'jsonschema_${src.outputHash} = jsonschema_${src.outputHash}.cli:main'
        # --replace 'jsonschema/__init__.py' 'jsonschema_${src.outputHash}/__init__.py'
        # --replace 'jsonschema/_reflect.py' 'jsonschema_${src.outputHash}/_reflect.py'

        substituteInPlace jsonschema/_utils.py \
          --replace 'pkgutil.get_data("jsonschema",' 'pkgutil.get_data("jsonschema_${jsonschema_3_0_2.src.outputHash}",'

        python-rewrite-imports \
          --path . \
          --replace jsonschema jsonschema_${jsonschema_3_0_2.src.outputHash}

      '';

      checkPhase = ''
        nosetests
      '';

      doCheck = false;
    };

    json5 = pythonPackages.buildPythonPackage rec {
      pname = "json5";
      version = "0.8.5";
      src = pythonPackages.fetchPypi {
        inherit pname version;
        sha256 = "1c3k5blbhq7g2lnbap26a846ag5x19ivisd3wfzz6bzdl46hyjqj";
      };
      doCheck = false;
    };

    jupyterlab_server = pythonPackages.buildPythonPackage rec {
      pname = "jupyterlab_server";
      version = "1.0.6";
      src = pythonPackages.fetchPypi {
        inherit pname version;
        sha256 = "1bax8iqwcc5p02h5ysdc48zvx7ll5jfzfsybhb3lfvyfpwkpb5yh";
      };
      nativeBuildInputs = with selfPythonPackages; [ nixpkgs-pytools ];
      propagatedBuildInputs = with selfPythonPackages; [
        notebook
        json5
        jsonschema_3_0_2
      ];
      doCheck = false;

      # Substitute the requirement for jsonschema in setup.py  and rewrite all imports.
      postPatch = with selfPythonPackages; ''
        substituteInPlace setup.py --replace "jsonschema>=3.0.1" "jsonschema_${jsonschema_3_0_2.src.outputHash}>=${jsonschema_3_0_2.version}"
        python-rewrite-imports \
          --path . \
          --replace jsonschema jsonschema_${jsonschema_3_0_2.src.outputHash}
      '';

    };

Copy link
Member

@FRidh FRidh left a comment

Choose a reason for hiding this comment

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

As we've discussed, no overrideScope' within the package set.

And move it to pkgs.jupyterlab_server
And move it to pkgs.jupyterlab
@infinisil
Copy link
Member Author

infinisil commented Sep 4, 2019

@FRidh I've updated this PR to put jupyterlab and jupyterlab_server in the top-level pkgs.* set, while replacing the ones in pythonPackages.* with a throw mentioning they've been moved.

I opted to still use overrideScope' in all-packages.nix however, because it's just less boilerplate. This is what I'm using to override the package set:

python3.pkgs.overrideScope' (self: super: {
  jsonschema = self.jsonschema3;
})

vs. what you'd need to write to achieve the same with .override:

(python3.override (old: {
  packageOverrides = lib.composeExtensions
    (old.packageOverrides or (self: super: {}))
    (self: super: {
      jsonschema = self.jsonschema3;
    });
})).pkgs

(if I leave out the composeExtension part, the previous packageOverrides of python3 would be ignored)

So I think it's a good idea to merge #67422 because it simplifies this kind of thing a lot.

@costrouc
Copy link
Member

costrouc commented Sep 5, 2019

@rheno-lindeque and @infinisil the import rewrite method that I've demonstrated will work without overrides. I think it is worth thinking what will happen if we allow incompatible versions of packages into a PYTHONPATH. I could provide a PR that demonstrates how this could be done without overrides?

@infinisil
Copy link
Member Author

@costrouc I'm not sure, but it seems to me that jupyterlab isn't meant to be used as a python package at all. So this PR now just moves it out of the python package set to pkgs.jupyterlab. I guess there's still the problem of the implicit python path thing when you set python packages in buildInputs (this is what you mean right?), but I think the fact that buildInputs can be used for python packages is bad design anyways, and I'd prefer people use pythonPath = ... or python.withPackages instead, then the PYTHONPATH problem shouldn't occur either (because you'd use buildInputs = [ jupyterlab (python.withPackages ...) ] which shouldn't conflate different versions).

I'm no python expert though, let me know if I got something mixed up. And feel free to demonstrate your approach if you feel like it fits in nixpkgs, I'm not quite sure myself what this entails.

@rehno-lindeque
Copy link
Contributor

I think the link I posted may have gotten lost in the noise. @costrouc posted a detailed description over here:

As a proof point, I did manage to get jupyterlab 1.1.1 working using python-rewrite-imports, though I think it's a little unfortunate you have to use it in both the package as well as packages that depend on it to make this work... (I ended up abandoning my attempt because of difficulties with extensions in jupyterlab > 1.0.0, it worked otherwise)

I suppose a detailed discussion probably wants to go to discourse or an rfc though. I feel as though both approaches have merit, but I'm not a regular contributor.

@cdyson37
Copy link
Contributor

cdyson37 commented Oct 4, 2019

Possibly stupid question: with jupyterlab as a standalone package, how does it "see" other Python modules? With python37.withPackages (p: with p; [jupyterlab fancy_module]) I'd expect fancy_module to be available within the jupyterlab session. But if it's standalone presumably I need to tell it where to find a suitable kernel?

FWIW jsonschema is only really used in one place within jupyterlab_server as far as I can see, and is probably easily manually renamed with a bit of sed-magic; that might be the path of least resistance?

@MMesch
Copy link
Contributor

MMesch commented Nov 1, 2019

@infinisil , thank you very much for this helpful PR. Do you still work on it? Is it blocked on something specific?

@infinisil
Copy link
Member Author

For one, this PR still depends on #67442, which @FRidh wants docs for, but I really hate writing xml. I could change this PR to not depend on it though by rewriting it in the verbose style described in #67423 (comment).

Also I'm not using jupyterlab, and #67423 (comment) mentions that apparently you need to be able to add python modules inside it. I think this PR might break the workflow described there because it can lead to package conflicts for jsonschema. Maybe that's unproblematic though.

@tbenst
Copy link
Contributor

tbenst commented Nov 4, 2019

as a heads up, jupyterlab is currently broken on master nix-shell -p 'python3.buildEnv.override { extraLibs = [ python3Packages.jupyterlab_server ]; }' look forward to this pull request!

Copy link
Contributor

@tbenst tbenst left a comment

Choose a reason for hiding this comment

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

I think this can be simplified greatly now that we have jsonschema-3.1 in nixpkgs. I would suggest retaining this as a python-module, and getting rid of the overlays & various changes to top-level & python-modules/packages that are no longer necessary. Thanks for the work updating this!

@@ -0,0 +1,42 @@
{ stdenv, buildPythonPackage, fetchPypi, python
Copy link
Contributor

Choose a reason for hiding this comment

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

This file is no longer needed as jsonschema has been updated to 3.1 already

@@ -2495,9 +2496,9 @@ in {

jupyterlab_launcher = callPackage ../development/python-modules/jupyterlab_launcher { };

jupyterlab_server = callPackage ../development/python-modules/jupyterlab_server { };
jupyterlab_server = throw "pkgs.python3.pkgs.jupyterlab_server has moved to pkgs.jupyterlab_server";
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 we want to keep as a python module for reasons explained in #67423 (comment)

@@ -3113,6 +3114,8 @@ in {

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

jsonschema3 = callPackage ../development/python-modules/jsonschema3 { };
Copy link
Contributor

Choose a reason for hiding this comment

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

no longer needed

@@ -4117,6 +4117,16 @@ in

jupyter-kernel = callPackage ../applications/editors/jupyter/kernel.nix { };

inherit (
let pythonPkgs = python3.pkgs.overrideScope' (self: super: {
Copy link
Contributor

Choose a reason for hiding this comment

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

This is no longer necessary now that nixpkgs is on jsonschema-3.1

@cdepillabout
Copy link
Member

At work we are developing an extension for JupyterLab. I'd really like to be able to write a nix expression to be able to create a development environment for JupyterLab extensions.

We are developing against a post-1.0 version of JupyterLab, so ideally I'd like to use this PR along with jupyterWith.

@infinisil Is there anything you need help with to get this PR in a state where it can be merged in?

@evanjs
Copy link
Member

evanjs commented Nov 12, 2019

Likewise wondering if there's anything that can be done to help.
Would love to swap Jupyter out for Jupyterlab (mmm, native dark theme)

@FRidh
Copy link
Member

FRidh commented Nov 12, 2019

jupyterlab works fine with 4eed2a5

@FRidh FRidh closed this Nov 12, 2019
@infinisil infinisil deleted the update-jupyterlab branch November 12, 2019 16:26
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.

Jupyterlab 1.0.4 fails to build, citing missing tornado version which exists
10 participants