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

Install Jupyter kernel when --enable-editable is in use #33855

Closed
mkoeppe opened this issue May 15, 2022 · 35 comments
Closed

Install Jupyter kernel when --enable-editable is in use #33855

mkoeppe opened this issue May 15, 2022 · 35 comments

Comments

@mkoeppe
Copy link
Contributor

mkoeppe commented May 15, 2022

When --enable-editable is in use, the Jupyter kernel sagemath is not being installed.

Workaround for users:

  • Either install without --enable-editable first
  • Or fix it using: ./sage -python -c 'from sage.repl.ipython_kernel.install import SageKernelSpec; SageKernelSpec.update()'

In this ticket, we fix it by adding a custom develop command.

(In follow up ticket #30298, the jupyter kernel (but without the symlink doc -- which cannot and should not be stored in a wheel!) will also be added as data_files - and thus be available in all types of installations - setup.py install, wheel-based, and editable)

CC: @tobiasdiez @dimpase @kiwifb @antonio-rojas @egourgoulhon

Component: build

Author: Matthias Koeppe

Branch/Commit: 4473375

Reviewer: Tobias Diez

Issue created by migration from https://trac.sagemath.org/ticket/33855

@mkoeppe mkoeppe added this to the sage-9.7 milestone May 15, 2022
@mkoeppe
Copy link
Contributor Author

mkoeppe commented May 15, 2022

comment:1

Some related tickets: #30298, #30124, #31156, #31157, #30306, #33651

@mkoeppe
Copy link
Contributor Author

mkoeppe commented May 15, 2022

Author: Matthias Koeppe

@mkoeppe
Copy link
Contributor Author

mkoeppe commented May 15, 2022

@mkoeppe
Copy link
Contributor Author

mkoeppe commented May 15, 2022

comment:4

Here's some preparation, split out from #32927.


New commits:

99038d1src/sage_setup/command/sage_install.py: Split sage_install_and_clean into several classes

@mkoeppe
Copy link
Contributor Author

mkoeppe commented May 15, 2022

Commit: 99038d1

@mkoeppe
Copy link
Contributor Author

mkoeppe commented May 15, 2022

comment:5

The kernel might also be missing in the wheels. Best to redo the kernelspec generation/installation similar to https://github.com/ipython/ipykernel/blob/main/setup.py#L108

@mkoeppe
Copy link
Contributor Author

mkoeppe commented May 15, 2022

Changed author from Matthias Koeppe to Matthias Koeppe, ...

@mkoeppe
Copy link
Contributor Author

mkoeppe commented May 16, 2022

comment:6

When building a wheel, the symlink doc must be omitted but the other files should be listed as data_files.

@sagetrac-git
Copy link
Mannequin

sagetrac-git mannequin commented May 16, 2022

Branch pushed to git repo; I updated commit sha1. This was a forced push. New commits:

6c5903dsrc/sage_setup/command/sage_install.py: Split sage_install_and_clean into several classes
40b44a1build/pkgs/sagelib/spkg-install: Do installation of sagemath kernelspec here, not in sage_setup.command.sage_install

@sagetrac-git
Copy link
Mannequin

sagetrac-git mannequin commented May 16, 2022

Changed commit from 99038d1 to 40b44a1

@mkoeppe
Copy link
Contributor Author

mkoeppe commented May 16, 2022

comment:8

Replying to @mkoeppe:

When building a wheel, the symlink doc must be omitted but the other files should be listed as data_files.

I'll do this in #30298 as a follow-up.

Here's a simple solution that will suffice for #33739

@sagetrac-git
Copy link
Mannequin

sagetrac-git mannequin commented May 16, 2022

Changed commit from 40b44a1 to 1c1dbc5

@sagetrac-git
Copy link
Mannequin

sagetrac-git mannequin commented May 16, 2022

Branch pushed to git repo; I updated commit sha1. New commits:

1c1dbc5src/setup.py: No need to use custom sage_install

@sagetrac-git
Copy link
Mannequin

sagetrac-git mannequin commented May 16, 2022

Changed commit from 1c1dbc5 to 6862287

@sagetrac-git
Copy link
Mannequin

sagetrac-git mannequin commented May 16, 2022

Branch pushed to git repo; I updated commit sha1. New commits:

6862287build/pkgs/sagelib/spkg-install: Un-poison SAGE_DOC for installing the kernelspec

@mkoeppe
Copy link
Contributor Author

mkoeppe commented May 16, 2022

Changed author from Matthias Koeppe, ... to Matthias Koeppe

@mkoeppe

This comment has been minimized.

@slel
Copy link
Member

slel commented May 17, 2022

comment:13

Possibly related question:

@mkoeppe
Copy link
Contributor Author

mkoeppe commented May 17, 2022

comment:14

Yes, that's the same issue

@tobiasdiez
Copy link
Contributor

comment:16

As you observe in #33739, this doesn't work if one simply uses pip to editable install sagelib. To also support this, I propose to run the kernelspec install as a custom develop cmdclass, similar to how it is currently done for install (extracting the kernelspec things to a mixin that can be reused). This seams to be a fairly standard way of doing it: https://github.com/search?q=kernelspec+develop++filename%3Asetup.py&type=code

@mkoeppe
Copy link
Contributor Author

mkoeppe commented May 17, 2022

comment:17

Yes, this sounds like a good approach. Do you want to push to the ticket? I'm working on something else right now

@tobiasdiez
Copy link
Contributor

comment:18

No, I sadly don't have time for this atm.

@mkoeppe

This comment has been minimized.

@mkoeppe

This comment has been minimized.

@sagetrac-git
Copy link
Mannequin

sagetrac-git mannequin commented May 18, 2022

Branch pushed to git repo; I updated commit sha1. This was a forced push. New commits:

6c5903dsrc/sage_setup/command/sage_install.py: Split sage_install_and_clean into several classes
4473375src/sage_setup/command/sage_install.py: Add sage_develop, use it in src/setup.py, pkgs/sagemath-standard/setup.py

@sagetrac-git
Copy link
Mannequin

sagetrac-git mannequin commented May 18, 2022

Changed commit from 6862287 to 4473375

@mkoeppe
Copy link
Contributor Author

mkoeppe commented May 18, 2022

comment:24

Replying to @tobiasdiez:

I propose to run the kernelspec install as a custom develop cmdclass

Re-done as you suggested, works nicely.

@tobiasdiez
Copy link
Contributor

Reviewer: Tobias Diez

@tobiasdiez
Copy link
Contributor

comment:25

Thanks. Code looks good to me and it seems to work (tested using #33739).

@mkoeppe
Copy link
Contributor Author

mkoeppe commented May 18, 2022

comment:26

Thanks!

@kiwifb
Copy link
Member

kiwifb commented May 18, 2022

comment:27

This ticket may conflict with a number of others touching the same files either in the same location or nearby locations. So some dependencies/merge order will have to be established. #32927 is already mentioned so I guess it is alright but #33838 may also clash.

@mkoeppe
Copy link
Contributor Author

mkoeppe commented May 18, 2022

comment:28

Test merge with #33838 succeeds without conflict

@kiwifb
Copy link
Member

kiwifb commented May 18, 2022

comment:29

That's great, are you aware of any other tickets that touches setup.pys?

@mkoeppe
Copy link
Contributor Author

mkoeppe commented May 18, 2022

comment:30

You are asking for tickets to review next? #28925

@vbraun
Copy link
Member

vbraun commented May 20, 2022

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

No branches or pull requests

5 participants