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

jupyter.lib: init #278315

Open
wants to merge 5 commits into
base: master
Choose a base branch
from
Open

jupyter.lib: init #278315

wants to merge 5 commits into from

Conversation

teto
Copy link
Member

@teto teto commented Jan 2, 2024

Description of changes

This is the first PR of hopefully others trying to tackle issues I mentioned in #269331, i.e., improve nixpkgs API regarding jupyter usage and document its usage.

This doesn't do everything I have in mind because I dont want to do N breaking changes before realizing jupyter maintainers disagree with the approach.

My idea is to merge small PRs like this iteratively so we can discuss the changes gradually since we have time until the next release.

For instance the documentation doesn't explain how to create jupyter-notebooks envs but this will be fixed by next release.

Things done

  • Built on platform(s)
    • x86_64-linux
    • aarch64-linux
    • x86_64-darwin
    • aarch64-darwin
  • For non-Linux: Is sandboxing enabled in nix.conf? (See Nix manual)
    • sandbox = relaxed
    • sandbox = true
  • Tested, as applicable:
  • Tested compilation of all packages that depend on this change using nix-shell -p nixpkgs-review --run "nixpkgs-review rev HEAD". Note: all changes have to be committed, also see nixpkgs-review usage
  • Tested basic functionality of all binary files (usually in ./result/bin/)
  • 24.05 Release Notes (or backporting 23.05 and 23.11 Release notes)
    • (Package updates) Added a release notes entry if the change is major or breaking
    • (Module updates) Added a release notes entry if the change is significant
    • (Module addition) Added a release notes entry if adding a new NixOS module
  • Fits CONTRIBUTING.md.

Add a 👍 reaction to pull requests you find important.

Introduce 2 functions to help with creating a jupyter environment:
- mkKernelFromHaskellEnv
- mkKernelFromPythonEnv
jupyter-all is not really something usable out of the box, mostly
something used as a test. This PR moves it to jupyter.tests to reflect
that.
... though I've let the reference accessible (for now) in
jupyter-console. I hope to remove `jupyter-console.mkConsole` in a next PR after more
discussions, possibly along with `withSingleKernel` ?
we have many kernels available and nixpkgs should be a good way to
configure software as complex as jupyter.
Right now our API is not straightforward and thus needs documenting.
This is not the final doc, it's an initial PR to bootstrap the effort
and refine the nixpkgs API for jupyter.

future work: document how to wrap jupyter-notebook
Copy link
Contributor

@thomasjm thomasjm left a comment

Choose a reason for hiding this comment

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

Thanks for getting started on this!

General impression: I'd be happier with this if the API looked more like the one we discussed in #269331. Not sure how you're planning to evolve this though. At the moment, it seems to have roughly the same warts as the current system, where you have to make a definitions object and pass it to a Jupyter function. Not as ergonomic as the proposed API IMO.

I think what we want to do here is maintain backwards compatibility with the old way of doing things, i.e. jupyter.override { definitions = ...; }. But now we add the new API, jupyter.withKernels (kernels: [...]).

A few other random comments:

  • As regards IHaskell specifically, I came up with nice flake support for GHC 9.0 through 9.8. The maintainer of IHaskell didn't want to keep them all in the main repo, but they're available here. I think it might make sense to pull these in to Nixpkgs with builtins.getFlake. We can even put kernelspecs over there and expose them in the flake, and then they can get tested in the other repo's CI. And they will have the logo images and stuff.
  • I'm not sure about the change to rename jupyter-all--it seems like an unrelated change to discuss in its own PR. I actually think jupyter-all is still useful to users as a top-level attribute, while jupyterLib is actually not (assuming we move towards the new API like I'm suggesting).

Type:
mkKernelFromHaskellEnv :: Derivation -> AttrSet
*/
mkKernelFromHaskellEnv = ghcEnv:
Copy link
Contributor

Choose a reason for hiding this comment

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

I'd expect to have 1 file per kernel. And perhaps not constructing raw Jupyter kernelspecs like this, because then we have to worry about making sure they're all valid. IIRC there is currently some machinery in Nixpkgs for constructing these with some type safety.

Copy link
Member Author

@teto teto Jan 5, 2024

Choose a reason for hiding this comment

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

I'd expect to have 1 file per kernel

ok

And perhaps not constructing raw Jupyter kernelspecs like this, because then we have to worry about making sure they're all valid. IIRC there is currently some machinery in Nixpkgs for constructing these with some type safety.

there is in nixos/modules/services/development/jupyter/./kernel-options.nix. It's possible to use those outside of the module but it's not trivial IMO. It can be added transparently later.

@thomasjm
Copy link
Contributor

thomasjm commented Jan 3, 2024

Let me try to emphasize a point about the abstraction I was hoping we can arrive at. The idea is to make a jupyter-kernels object with keys like python3 and ihaskell. Then you can build things like

jupyter-kernels.python3.withPackages (ps: [ps.scipy ps.matplotlib]) # ==> yields a kernel.json with this Python environment
jupyter-kernels.ihaskell.withPackages (ps: [ps.aeson]) # ==> yields a kernel.json with this Haskell environment

The idea is to make a common withPackages abstraction, and not to have a bunch of separate functions like mkKernelFromHaskellEnv, mkKernelFromPythonEnv, etc. These are fine as internal functions, but should not make up the public API.

These kernels will be a good building block, because then you can assemble a JUPYTER_PATH with them and point any Jupyter tool at it. It will make the implementation of jupyter.withKernels (kernels: [...]) easy.

(As well as jupyter-console.withKernel (kernels: ...). While we're at it I think we should add jupyter-notebook.withKernels, and maybe a separate alias jupyter-lab.withKernels as well.)

@teto
Copy link
Member Author

teto commented Jan 5, 2024

@thomasjm I've invited you to https://matrix.to/#/#jupyter:nixos.org , I think it will be easier to discuss the API details there. Let us know if that works with you.

I had in mind two ways to approach the API:

  1. you pass on definitions everywhere.
  2. you build a kernel derivation (jupyter-kernel) and pass it on to the API

This PR sticks to 1. because it's the current paradigm.

At the moment, it seems to have roughly the same warts as the current system, where you have to make a definitions object and pass it to a Jupyter function

I didn't want to change this without knowing first if "definition" was an official jupyter semantic ? If not we could change it, I have no idea.

I have questions about jupyter-kernels.python3.withPackages (ps: [ps.scipy ps.matplotlib]):

  • how do you change the other parameters of the kernel.json ? like the "displayName". Does it return a derivation or an attrset like this PR's mkKernelFromHaskellEnv ?
  • how do you select the python version ? like python310 instead of python311 ?
    scoping the kernel creation function by language instead of a function suffix might be easier to remember. haskell.mkKernel (in current PR) instead of mkKernelFromHaskellEnv

to provide jupyter frontends, we could reuse the traditional nixpkgs system:
wrapJupyter <frontend> <kernels>
such this would wrap the frontend bin/ executables with JUPYTER_PATH.
could be jupyter-console/jupyter-notebook.

I think it might make sense to pull these in to Nixpkgs with builtins.getFlake

builtins.getFlake is experimental so we can't use it but that's a detail. FYI: I have the merge rights on ihaskell (because I created the first flake IIRC, thanks for your latest update).

I'm not sure about the change to rename jupyter-all--it seems like an unrelated change to discuss in its own PR. I actually think jupyter-all is still useful to users as a top-level attribute

I disagree, it's a completely arbitrary confusing package. One would think it contains all kernels (which could make sense, but hard to maintain) but it just contain 2, and not the most popular if i may say so.
Like jupyter-console. One would think it's the python3Packages.jupyter-console but it's not, it's an attrset equivalent to a jupyter nix library (aka jupyterLib).
jupyter-kernels is also a jupyter library. What I like about "jupyterLib" is that it's clearer it is not a derivation but a set of nix functions. I am fine with sticking to jupyter-kernels though.

there should be a getResourceGroup function, with which you can use the manually created group without having to manage it with pulumi.
afaik there are 3 ways pulumi can use resources:

creates them, and so manages them (pulumi destroy deletes them)
one time import, but then pulumi manages them (pulumi destroy deletes them)
get* function, which just queries it's properties, and can be used as parameters/arguments creating other resources (pulumi destroy doesn't delete them)

I like jupyter.withKernels (kernels: [...]). We might need a more evolved wrapper to pass jupyter options too, like the ones in the nixos module: jupyter config, port to listen to, password, arbitrary config etc.

@thomasjm
Copy link
Contributor

thomasjm commented Jan 6, 2024

I didn't want to change this without knowing first if "definition" was an official jupyter semantic ?

It is not.

how do you change the other parameters of the kernel.json ? like the "displayName".

Those parameters would rarely change I think. I'd suggest designing it so it can be overridden, i.e.

(jupyter-kernels.python3.withPackages (ps: [...])).override { displayName = "foo"; }

and/or

(jupyter-kernels.python3.override { displayName = "foo"; }).withPackages (ps: [...])

how do you select the python version ? like python310 instead of python311 ?

I'd suggesting just providing jupyter-kernels.python310 and jupyter-kernels.python311 (and any other current Python versions in Nixpkgs) for users who want to pick the version. That way we can also run tests that these versions work with the kernel (which is not guaranteed in my experience).

there should be a getResourceGroup function, with which you can use the manually created group without having to manage it with pulumi.

You've lost me here. Are these comments meant for a different thread?

We might need a more evolved wrapper to pass jupyter options too, like the ones in the nixos module: jupyter config, port to listen to, password, arbitrary config etc.

Those sorts of config are properties of the specific Jupyter tool you're using. To me, configuring those is an orthogonal concern. However, I do think we should make it clear which tool you're using. Probably by having jupyter-lab.withKernels and jupyter-notebook.withKernels, not just "jupyter.withKernels".

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

2 participants