Opened 3 years ago

Closed 3 years ago

#25328 closed enhancement (duplicate)

Respect jupyter path priority

Reported by: gh-timokau Owned by:
Priority: major Milestone: sage-duplicate/invalid/wontfix
Component: distribution Keywords:
Cc: Merged in:
Authors: Timo Kaufmann Reviewers:
Report Upstream: N/A Work issues:
Branch: u/gh-timokau/respect-jupyter-path (Commits, GitHub, GitLab) Commit: 029123097d79f2c67bca2d725c470ef667a88ca2
Dependencies: Stopgaps:

Status badges

Description

Jupyter provides a jupyter_path function that gives back a list of paths in the right priority. Sage currently uses the apparently undocumented ENV_JUPYTER_PATH which is taken into consideration in the `jupyter_path` function.

ENV_JUPYTER_PATH is always [os.path.join(sys.prefix, 'share', 'jupyter')] and not overridable by the user. jupyter_path prefers the JUPYTER_PATH environment variable when it is set. When it is not set, it first checks jupyter_data_dir, which apparently defaults to a user-dir (~/.local) and then falls back to the current behaviour (ENV_JUPYTER_PATH).

So in summary, this patch changes the default behaviour from using a system directory to using a user directory. It allows the user to override that directory.

Change History (30)

comment:1 Changed 3 years ago by gh-timokau

  • Status changed from new to needs_review

comment:2 in reply to: ↑ description ; follow-up: Changed 3 years ago by jdemeyer

Replying to gh-timokau:

this patch changes the default behaviour from using a system directory to using a user directory

This is not acceptable. What is wrong with the current Sage behaviour? In other words, which bug is this supposed to fix?

comment:3 Changed 3 years ago by jdemeyer

  • Status changed from needs_review to needs_work

comment:4 in reply to: ↑ 2 ; follow-up: Changed 3 years ago by gh-timokau

Replying to jdemeyer:

This is not acceptable.

I don't have any experience with jupyter, but wouldn't it be the correct and expected behaviour to use the user's jupyter instead of the system one?

What is wrong with the current Sage behaviour? In other words, which bug is this supposed to fix?

For one relying on the undocumented ENV_JUPYTER_PATH could be regarded as a problem in itself. Specifically what I'm trying to fix is that in nixos jupyter is *not* in sys.path/share/jupyter.

comment:5 in reply to: ↑ 4 ; follow-up: Changed 3 years ago by jdemeyer

Replying to gh-timokau:

I don't have any experience with jupyter, but wouldn't it be the correct and expected behaviour to use the user's jupyter instead of the system one?

No. Installing a Jupyter kernel is not much different from installing a Python package: Sage doesn't install Python packages in the user's directory either.

For one relying on the undocumented ENV_JUPYTER_PATH could be regarded as a problem in itself.

Fair enough. I don't mind changing that.

Specifically what I'm trying to fix is that in nixos jupyter is *not* in sys.path/share/jupyter.

Do you mean sys.prefix instead of sys.path? So where is Jupyter installed then?

Does nixos even have a concept like sys.prefix in the first place? Probably I don't know nixos well enough to understand the problem.

I should add that this problem is not Sage-specific. Many Jupyter packages install files in sys.prefix/share/jupyter. So I wonder how nixos deals with those.

comment:6 in reply to: ↑ 5 ; follow-up: Changed 3 years ago by gh-timokau

Replying to jdemeyer:

Replying to gh-timokau:

I don't have any experience with jupyter, but wouldn't it be the correct and expected behaviour to use the user's jupyter instead of the system one?

No. Installing a Jupyter kernel is not much different from installing a Python package: Sage doesn't install Python packages in the user's directory either.

Okay I guess jupyter_path() is meant to be used to *discover* jupyter files, not install them. That's why it gives back a list and why the user directory has highest precedence (which is correct for discovery).

It looks like jupyter doesn't provide a standard way to find the correct directory to install to. Since ENV_JUPYTER_PATH is hardcoded as sys.prefix/share/jupyter (yes I meant prefix), what do you think about adding a configuration variable to env.py that defaults to sys.prefix/share/jupyter? Maybe JUPYTER_TARGET_DIR?

Specifically what I'm trying to fix is that in nixos jupyter is *not* in sys.path/share/jupyter.

Do you mean sys.prefix instead of sys.path? So where is Jupyter installed then?

Does nixos even have a concept like sys.prefix in the first place? Probably I don't know nixos well enough to understand the problem.

The basic idea of nix is that every package gets installed under /nix/store/<unique-package-hash>-pkg-name (allowing mulitple versions of a package to be installed side-by-side, guaranteeing immutability etc.).

The current jupyter version for example is in /nix/store/siicvck75m8qw0rvs303m8svgdrp3rz1-python2.7-jupyter_core-4.4.0. So no, there is no sys.prefix.

I should add that this problem is not Sage-specific. Many Jupyter packages install files in sys.prefix/share/jupyter. So I wonder how nixos deals with those.

I don't know about other software with that problem. In fact jupyter support for nix is currently worked on[1] so there is not much jupyter software around yet.

[1] https://github.com/NixOS/nixpkgs/pull/33673#issuecomment-394610280

comment:7 in reply to: ↑ 6 ; follow-up: Changed 3 years ago by jdemeyer

Replying to gh-timokau:

It looks like jupyter doesn't provide a standard way to find the correct directory to install to.

I would say that this is really a distutils issue rather than a Jupyter issue because it's distutils which should figure out installation directories.

comment:8 in reply to: ↑ 7 ; follow-up: Changed 3 years ago by gh-timokau

Replying to jdemeyer:

Replying to gh-timokau:

It looks like jupyter doesn't provide a standard way to find the correct directory to install to.

I would say that this is really a distutils issue rather than a Jupyter issue because it's distutils which should figure out installation directories.

But this is jupyter specific, not a standard python package.

Whatever is at fault, what do you think about the JUPYTER_TARGET_DIR approach? Should I implement that?

comment:9 Changed 3 years ago by fbissey

From the point of view of regular distribution which build sage with the intent of installing things system wide this patch would be awful. It would result in the kernel being installed in a place not considered for installation. We definitely want a sys.prefix location. Using ENV_JUPYTER_PATH is admittedly problematic, I am not sure about a JUPYTER_TARGET_DIR approach.

comment:10 in reply to: ↑ 8 Changed 3 years ago by jdemeyer

Replying to gh-timokau:

Whatever is at fault, what do you think about the JUPYTER_TARGET_DIR approach? Should I implement that?

As I said, this has nothing to do with Jupyter, so you might as well call it PYTHON_TARGET_PREFIX or whatever.

comment:11 Changed 3 years ago by jdemeyer

Does nixos patch distutils to install stuff in non-standard locations?

comment:12 follow-up: Changed 3 years ago by jdemeyer

I asked the question about patching distutils because most Jupyter kernels simply use setup.py to install those files.

Sage has hand-written code for that (I guess mainly because some files are generated, not copied). The default install location (using distutils) is sys.prefix/share/jupyter so Sage assumes that. Of course, if you patch distutils, then this might no longer be correct.

So I think the best solution might be to install the Jupyter kernel of Sage using a setup.py script. Maybe we should even make it a separate package.

(Note that when I said "distutils" it applies to setuptools and pip too because those just extensions of distutils)

comment:13 in reply to: ↑ 12 ; follow-up: Changed 3 years ago by gh-timokau

Replying to fbissey:

From the point of view of regular distribution which build sage with the intent of installing things system wide this patch would be awful. It would result in the kernel being installed in a place not considered for installation. We definitely want a sys.prefix location. Using ENV_JUPYTER_PATH is admittedly problematic, I am not sure about a JUPYTER_TARGET_DIR approach.

Yeah the paths.jupyter_path()[0] solution isn't viable. The JUPYTER_TARGET_DIR approach would remain the default unchanged but make it configurable.

Replying to jdemeyer:

Replying to gh-timokau:

Whatever is at fault, what do you think about the JUPYTER_TARGET_DIR approach? Should I implement that?

As I said, this has nothing to do with Jupyter, so you might as well call it PYTHON_TARGET_PREFIX or whatever.

Why doesn't it have anything to do with Jupyter? It is the installation location for a Jupyter package after all. Although I don't really have any strong opionion about the variable name.

Replying to jdemeyer:

Does nixos patch distutils to install stuff in non-standard locations?

I don't think it does, but I'd have to ask our python maintainer/expert. Grepping through nixpkgs, it seems like the standard python install is basically:

` pip install *.whl --no-index --prefix=$out --no-cache ${toString installFlags} --build tmpbuild `

So --prefix has that purpose. For nixos the jupyter kernel should be installed in

/nix/store/<hash>-sage-8.2/share/jupyter

or even into a sage-jupyter directory. But sage tries to instead install it to

/nix/store/<hash>-python-2.7.14/share/jupyter

Replying to jdemeyer:

So I think the best solution might be to install the Jupyter kernel of Sage using a setup.py script. Maybe we should even make it a separate package.

Seperating the jupyter kernel sounds like the best option to me. At least archlinux (didn't check other distros) seperates it already. I don't know enough about jupyter and sage to do that myself though.

comment:14 in reply to: ↑ 13 ; follow-up: Changed 3 years ago by jdemeyer

Replying to gh-timokau:

` pip install *.whl --no-index --prefix=$out --no-cache ${toString installFlags} --build tmpbuild `

And this is used for building Sage? I very doubt that Sage can be installed using pip.

comment:15 in reply to: ↑ 14 ; follow-up: Changed 3 years ago by gh-timokau

Replying to jdemeyer:

Replying to gh-timokau:

` pip install *.whl --no-index --prefix=$out --no-cache ${toString installFlags} --build tmpbuild `

And this is used for building Sage? I very doubt that Sage can be installed using pip.

No, that is the python default. For sage I overrode the buildPhase with python -u setup.py --no-user-cfg build.

comment:16 in reply to: ↑ 15 ; follow-up: Changed 3 years ago by jdemeyer

Replying to gh-timokau:

For sage I overrode the buildPhase with python -u setup.py --no-user-cfg build.

But then you're not setting the installation prefix, so it's not surprising that it fails.

comment:17 in reply to: ↑ 16 ; follow-up: Changed 3 years ago by gh-timokau

Replying to jdemeyer:

Replying to gh-timokau:

For sage I overrode the buildPhase with python -u setup.py --no-user-cfg build.

But then you're not setting the installation prefix, so it's not surprising that it fails.

I do set it in the installPhase:

python2 -u setup.py --no-user-cfg install --prefix=$out

I don't think it is possible to pass --prefix to build, right? At least it wouldn't make much sense. So apparently the kernel is installed in the build phase of setup.py?

comment:18 in reply to: ↑ 17 ; follow-up: Changed 3 years ago by jdemeyer

I wouldn't assume that Sage really has a clean separation between build and install. I think it's safer to run only install, not build.

comment:19 follow-up: Changed 3 years ago by jdemeyer

See also #21507

comment:20 in reply to: ↑ 18 ; follow-up: Changed 3 years ago by gh-timokau

Replying to jdemeyer:

I wouldn't assume that Sage really has a clean separation between build and install. I think it's safer to run only install, not build.

I didn't know that was possible, interesting. However it currently works except for this jupyter issue. Splitting the two has various advantages, for example the build can happen in a tmpfs.

Reading the setup.py does suggest that the kernel spec is installed in install though. The function is called in sage_install.

comment:21 in reply to: ↑ 19 Changed 3 years ago by gh-timokau

Replying to jdemeyer:

See also #21507

Thats very interesting. Apparently fbissey already mentioned pretty much this exact problem in https://trac.sagemath.org/ticket/21507#comment:31 and it was discussed a bit, but then apparently dropped.

comment:22 in reply to: ↑ 20 Changed 3 years ago by jdemeyer

Replying to gh-timokau:

Reading the setup.py does suggest that the kernel spec is installed in install though. The function is called in sage_install.

Yes, you are right.

I think the issue might be confusion between the "installation prefix" and sys.prefix which may not be the same thing.

comment:23 Changed 3 years ago by jdemeyer

I have an idea for solving this, hang on...

comment:24 Changed 3 years ago by jdemeyer

See #25546

comment:25 Changed 3 years ago by jdemeyer

So does #25546 fix your issue?

comment:26 Changed 3 years ago by gh-timokau

If I just replace my JUPYTER_PATH patch with #25546 I get a few errors:

Failed example:
    spec.use_local_mathjax()
Exception raised:
    Traceback (most recent call last):
      File "/nix/store/3s097s5b25czl1sz747h2dl6szf0ckz3-python-2.7.15-env/lib/python2.7/site-packages/sage/doctest/forker.py", line 551, in _run
        self.compile_and_execute(example, compiler, test.globs)
      File "/nix/store/3s097s5b25czl1sz747h2dl6szf0ckz3-python-2.7.15-env/lib/python2.7/site-packages/sage/doctest/forker.py", line 961, in compile_and_execute
        exec(compiled, globs)
      File "<doctest sage.repl.ipython_kernel.install.SageKernelSpec.use_local_mathjax[2]>", line 1, in <module>
        spec.use_local_mathjax()
      File "/nix/store/3s097s5b25czl1sz747h2dl6szf0ckz3-python-2.7.15-env/lib/python2.7/site-packages/sage/repl/ipython_kernel/install.py", line 123, in use_local_mathjax
        self.symlink(src, dst)
      File "/nix/store/3s097s5b25czl1sz747h2dl6szf0ckz3-python-2.7.15-env/lib/python2.7/site-packages/sage/repl/ipython_kernel/install.py", line 106, in symlink
        os.symlink(src, dst)
    OSError: [Errno 17] File exists
**********************************************************************
File "/nix/store/flwvb6s36sp7slwbrwsahg29jw0i3nha-sage-src-8.2/src/sage/repl/ipython_kernel/install.py", line 118, in sage.repl.ipython_kernel.install.SageKernelSpec.use_local_mathjax
Failed example:
    os.path.isdir(mathjax)
Expected:
    True
Got:
    False
**********************************************************************
File "/nix/store/flwvb6s36sp7slwbrwsahg29jw0i3nha-sage-src-8.2/src/sage/repl/ipython_kernel/install.py", line 133, in sage.repl.ipython_kernel.install.SageKernelSpec.use_local_jsmol
Failed example:
    spec.use_local_jsmol()
Exception raised:
    Traceback (most recent call last):
      File "/nix/store/3s097s5b25czl1sz747h2dl6szf0ckz3-python-2.7.15-env/lib/python2.7/site-packages/sage/doctest/forker.py", line 551, in _run
        self.compile_and_execute(example, compiler, test.globs)
      File "/nix/store/3s097s5b25czl1sz747h2dl6szf0ckz3-python-2.7.15-env/lib/python2.7/site-packages/sage/doctest/forker.py", line 961, in compile_and_execute
        exec(compiled, globs)
      File "<doctest sage.repl.ipython_kernel.install.SageKernelSpec.use_local_jsmol[2]>", line 1, in <module>
        spec.use_local_jsmol()
      File "/nix/store/3s097s5b25czl1sz747h2dl6szf0ckz3-python-2.7.15-env/lib/python2.7/site-packages/sage/repl/ipython_kernel/install.py", line 140, in use_local_jsmol
        self.symlink(src, dst)
      File "/nix/store/3s097s5b25czl1sz747h2dl6szf0ckz3-python-2.7.15-env/lib/python2.7/site-packages/sage/repl/ipython_kernel/install.py", line 106, in symlink
        os.symlink(src, dst)
    OSError: [Errno 17] File exists
**********************************************************************
File "/nix/store/flwvb6s36sp7slwbrwsahg29jw0i3nha-sage-src-8.2/src/sage/repl/ipython_kernel/install.py", line 135, in sage.repl.ipython_kernel.install.SageKernelSpec.use_local_jsmol
Failed example:
    os.path.isdir(jsmol)
Expected:
    True
Got:
    False
**********************************************************************
File "/nix/store/flwvb6s36sp7slwbrwsahg29jw0i3nha-sage-src-8.2/src/sage/repl/ipython_kernel/install.py", line 150, in sage.repl.ipython_kernel.install.SageKernelSpec.use_local_threejs
Failed example:
    spec.use_local_threejs()
Exception raised:
    Traceback (most recent call last):
      File "/nix/store/3s097s5b25czl1sz747h2dl6szf0ckz3-python-2.7.15-env/lib/python2.7/site-packages/sage/doctest/forker.py", line 551, in _run
        self.compile_and_execute(example, compiler, test.globs)
      File "/nix/store/3s097s5b25czl1sz747h2dl6szf0ckz3-python-2.7.15-env/lib/python2.7/site-packages/sage/doctest/forker.py", line 961, in compile_and_execute
        exec(compiled, globs)
      File "<doctest sage.repl.ipython_kernel.install.SageKernelSpec.use_local_threejs[2]>", line 1, in <module>
        spec.use_local_threejs()
      File "/nix/store/3s097s5b25czl1sz747h2dl6szf0ckz3-python-2.7.15-env/lib/python2.7/site-packages/sage/repl/ipython_kernel/install.py", line 157, in use_local_threejs
        self.symlink(src, dst)
      File "/nix/store/3s097s5b25czl1sz747h2dl6szf0ckz3-python-2.7.15-env/lib/python2.7/site-packages/sage/repl/ipython_kernel/install.py", line 106, in symlink
        os.symlink(src, dst)
    OSError: [Errno 17] File exists
**********************************************************************
File "/nix/store/flwvb6s36sp7slwbrwsahg29jw0i3nha-sage-src-8.2/src/sage/repl/ipython_kernel/install.py", line 152, in sage.repl.ipython_kernel.install.SageKernelSpec.use_local_threejs
Failed example:
    os.path.isdir(threejs)
Expected:
    True
Got:
    False

I don't have any time to investigate right now and I probably won't get to it until at least the 25.06.

comment:27 Changed 3 years ago by fbissey

For the record I move the kernel spec install to sage_setup which I neither install or doctest. Most the doctest in that files will fail on a system wide install because they try to touch files that don't belong to users. Those doctests would probably fail just the same for me.

comment:28 Changed 3 years ago by gh-timokau

@jdemeyer I opened a new ticket (#25722) to fix those issues by simply moving the test to a tmpdir. That in combination with your prefix-fix solves the problem for me (in a much better way than I did in this ticket). Thanks!

@fbissey I'm not having any issues with tests trying to write where they shouldn't (anymore). I've recently upstreamed a few changes like #25357. Maybe you wouldn't have any problems anymore either if you'd try again now (or after the next sage release).

comment:29 Changed 3 years ago by gh-timokau

  • Milestone changed from sage-8.3 to sage-duplicate/invalid/wontfix

comment:30 Changed 3 years ago by jdemeyer

  • Resolution set to duplicate
  • Status changed from needs_work to closed
Note: See TracTickets for help on using tickets.