There's no notebook in https://github.com/jupyterhub/jupyterhub/blob/master/requirements.txt.

@dotlambda Hmm it is in dev-requirements.txt

I did find jupyterhub/jupyterhub@0414a0be which removed ipython[notebook] from requirements.txt, not sure if that's related

Ah yeah, they mention it in the readme:

If you plan to run notebook servers locally, you will need to install the Jupyter notebook package

So it's only an optional dependency for local servers

Why it doesn't work by adding notebook to the same nix-shell environment as I did in #58540?

@jluttine Because it gets wrapped with a PYTHON_PATH that contains exactly the packages that were declared in the build.

Ok. So one should write an overlay to modify the build inputs if running the notebook servers locally? That sounds like something that should perhaps be made a bit simpler because it's probably a typical usecase. Not sure how though.

@jluttine Well this PR fixes it, so if this gets merged you don't have to do anything to get it running.

If you want to get it running without this PR, you can use the following overlay, which looks totally ridiculous, but actually is the "proper" way to override stuff like that:

self: super: {
  python3 = super.python3.override (old: {
    packageOverrides = super.lib.composeExtensions (old.packageOverrides or (pself: psuper: {}))
      (pself: psuper: {
        jupyterhub = psuper.jupyterhub.overrideAttrs (old: {
          propagatedBuildInputs = old.propagatedBuildInputs or [] ++ [
            pself.notebook
          ];
        });
      });
  });
}

Thanks! That's what I thought that proper overriding is probably a bit tedious for the typical usecase. But in a way this pull request isn't the "correct" fix either because notebook is just an optional dependency, not required by JupyterHub except in some cases. I have no idea what would be the correct fix..

Alright, seems sensible. Could we add a comment on why notebook is there so that it isn't removed in the future?

But in a way this pull request isn't the "correct" fix either because notebook is just an optional dependency, not required by JupyterHub except in some cases.

We usually include optional dependencies if it's requested.
Alternatively, we could add a top-level attribute using toPythonApplication and override the jupyterhub derivation used. Then, people won't have the optional dependency of they only use it as a library.

To me, adding notebook to build inputs seems like the best practical solution, yes. Probably not much harm to include it there even if it's not always strictly necessary.

I tested and it indeed fixes the issue and now Jupyter Hub is running fine.

About the alternative, if I understand correctly, the dependency of notebook seems orthogonal to the question of whether Hub is used as an application or a library. People could use it as an application without running the notebook servers locally.

I've sometimes seen some optional arguments to derivations, something like: useCoolThing ? false. Not sure if that would be useful here. For instance, includeNotebook ? true. Then, I guess, it could be relatively easily disabled (although I'm not familiar how those arguments are overwritten).

@jluttine These arguments would be overwritten with jupyterhub.override { includeNotebook = true; }, but I don't think we need to add such an argument if nobody minds having it included by default. The only disadvantage is a 17% increase in closure size and the advantage is that it just works by default. I think the advantage far outweighs the disadvantage.

Sounds good.

Ping @FRidh and @costrouc

About the alternative, if I understand correctly, the dependency of notebook seems orthogonal to the question of whether Hub is used as an application or a library.

It isn't since when used as a library, withPackages can be used.

@dotlambda Ah, true! Thanks for clarifying!

I would merge this either as it is now, or add withNotebook ? true optional argument so that it is explicit that notebook isn't required but is just for convenience added by default. But as said, probably nobody ever cares so much about the slightly larger closure size that they'd set it to false..

Backported to 19.03 in a05b0cf

A bit related issue I now encountered: #58662