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

Switch jsmol to jupyter-jsmol #30315

Closed
mkoeppe opened this issue Aug 7, 2020 · 46 comments
Closed

Switch jsmol to jupyter-jsmol #30315

mkoeppe opened this issue Aug 7, 2020 · 46 comments

Comments

@mkoeppe
Copy link
Contributor

mkoeppe commented Aug 7, 2020

Currently, jsmol is installed by the jmol spkg and then symlinked into the jupyter notebook directory.

In this ticket, we switch to https://pypi.org/project/jupyter-jsmol/ (https://github.com/fekad/jupyter-jsmol), which can be installed by standard Python tools.

Discussion: https://groups.google.com/g/sage-devel/c/qKqTmLzHAbg/m/6YxQ_qWUBQAJ

See also:

Depends on #31002

CC: @kcrisman @novoselt @fchapoton @antonio-rojas @kiwifb @slel @nbruin @paulmasson @egourgoulhon @kwankyu @enriqueartal @jcamp0x2a

Component: packages: standard

Keywords: sd111

Author: Matthias Koeppe

Branch/Commit: 80a3db1

Reviewer: Joshua Campbell

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

@mkoeppe mkoeppe added this to the sage-9.2 milestone Aug 7, 2020
@mkoeppe

This comment has been minimized.

@mkoeppe
Copy link
Contributor Author

mkoeppe commented Aug 8, 2020

comment:2

(deleted)

@mkoeppe
Copy link
Contributor Author

mkoeppe commented Aug 8, 2020

comment:3

./sage -pip install jupyter-jsmol installs proper nbextensions:

$ jupyter nbextension list
Known nbextensions:
  config dir: /Users/mkoeppe/s/sage/sage-rebasing/local/etc/jupyter/nbconfig
    notebook section
      jupyter_jsmol/extension  enabled 
      - Validating: OK
      jupyter-js-widgets/extension  enabled 
      - Validating: OK

The installed files can be seen in $SAGE_LOCAL/share/jupyter/nbextensions/jupyter_jsmol/

@mkoeppe mkoeppe modified the milestones: sage-9.2, sage-9.3 Aug 13, 2020
@mkoeppe
Copy link
Contributor Author

mkoeppe commented Nov 29, 2020

@mkoeppe
Copy link
Contributor Author

mkoeppe commented Nov 29, 2020

Commit: ac1ab7c

@mkoeppe
Copy link
Contributor Author

mkoeppe commented Nov 29, 2020

New commits:

6617f87build/pkgs/jupyter_jsmol: New
cbaf661build/pkgs/jmol/spkg-install.in: No longer install jsmol
2ca5328sage.repl.display.jsmol_iframe: Adjust path to jsmol
ac1ab7csage.repl.ipython_kernel.install: No longer symlink jsmol

@mkoeppe
Copy link
Contributor Author

mkoeppe commented Nov 29, 2020

Author: Matthias Koeppe

@sagetrac-git
Copy link
Mannequin

sagetrac-git mannequin commented Nov 29, 2020

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

e6ebaf7build/pkgs/jupyter_jsmol: Add dependencies, install-requires.txt

@sagetrac-git
Copy link
Mannequin

sagetrac-git mannequin commented Nov 29, 2020

Changed commit from ac1ab7c to e6ebaf7

@mkoeppe
Copy link
Contributor Author

mkoeppe commented Nov 29, 2020

comment:9

sagetex declares jmol as a dependency - what is it used for?

@kcrisman
Copy link
Member

comment:10

sagetex declares jmol as a dependency - what is it used for?

I think for generating a 3d image in its documentation, compilation of which is part of its test suite.

@mkoeppe
Copy link
Contributor Author

mkoeppe commented Dec 6, 2020

comment:11

Hoping we can make progress on this ticket this week - https://wiki.sagemath.org/days111

@mkoeppe
Copy link
Contributor Author

mkoeppe commented Dec 6, 2020

Changed keywords from none to sd111

@mkoeppe
Copy link
Contributor Author

mkoeppe commented Dec 7, 2020

Dependencies: #31020

@sagetrac-git
Copy link
Mannequin

sagetrac-git mannequin commented Dec 7, 2020

Changed commit from e6ebaf7 to 6a549d9

@sagetrac-git
Copy link
Mannequin

sagetrac-git mannequin commented Dec 7, 2020

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

21259b2build/pkgs/jupyter_jsmol: New
b5f5f43build/pkgs/jmol/spkg-install.in: No longer install jsmol
a0fe543sage.repl.display.jsmol_iframe: Adjust path to jsmol
6e24bc0sage.repl.ipython_kernel.install: No longer symlink jsmol
52803b4build/pkgs/jupyter_jsmol: Add dependencies, install-requires.txt
7194459build/pkgs/jmol/type: Make it optional
0ed256cbuild/make/Makefile.in: Compute SAGE_CHECK_... earlier, before evaluating dependencies
1f714c1build/pkgs/sagetex/dependencies: Conditionalize order-only deps on SAGE_CHECK_sagetex
6a549d9Merge branch 't/31020/build_make_makefile_in__fix_sage_check_logic__conditionalize_sagetex_dependencies_on_sage_check' into t/30315/switch_jsmol_to_jupyter_jsmol__make_jmol_optional

@mkoeppe
Copy link
Contributor Author

mkoeppe commented Dec 7, 2020

comment:14

Replying to @kcrisman:

sagetex declares jmol as a dependency - what is it used for?

I think for generating a 3d image in its documentation, compilation of which is part of its test suite.

Thanks! I have adjusted the dependencies in #31020 (now a dependency) so that jmol is only treated as a dependency when SAGE_CHECK is active.

@sagetrac-git
Copy link
Mannequin

sagetrac-git mannequin commented Dec 7, 2020

Changed commit from 6a549d9 to ae970c2

@sagetrac-git
Copy link
Mannequin

sagetrac-git mannequin commented Dec 7, 2020

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

3a45d72build/pkgs/jupyter_jsmol/spkg-install.in: Do not rebuild the javascript file
a8e0364Specify bdist temp folder for WSL compatibility
a3077c7Use mktemp
c5a14a3build/bin/sage-dist-helpers (sdh_setup_bdist_wheel): New
fb10429build/pkgs/*/spkg-install.in: Use new function sdh_setup_bdist_wheel
32bf463Merge branch 't/31002/public/build/bdist_wsl' into t/30315/switch_jsmol_to_jupyter_jsmol__make_jmol_optional
ae970c2build/pkgs/jupyter_jsmol/spkg-install.in: Use new function sdh_setup_bdist_wheel

@mkoeppe
Copy link
Contributor Author

mkoeppe commented Dec 7, 2020

Changed dependencies from #31020 to #31002, #31020

@mkoeppe

This comment has been minimized.

@kcrisman
Copy link
Member

kcrisman commented Dec 7, 2020

comment:18

That is not a good idea. Because we want people to be able to build the documentation for SageTeX, which after all isn't "really" upstream. Instead, we should see if jsmol or some other tool suffices to build 3d pictures in SageTeX, which is a very useful use case for it. Untested = broken, and I don't think that we've ever had a dependency only in SAGE_CHECK before.

@kcrisman
Copy link
Member

kcrisman commented Dec 7, 2020

comment:19

and I don't think that we've ever had a dependency only in SAGE_CHECK before.

Though my info on the build system is pretty outdated :-)

@novoselt
Copy link
Member

novoselt commented Dec 7, 2020

comment:22

Replying to @kcrisman:

Interesting. I wonder whether jsmol might be a drop-in replacement for this specific use case though. Or even Tachyon ...

What's the status of generating 3D plots and saving them using threejs? I think for SageTeX it is irrelevant what is producing pictures, just show that it works well. Tachyon is not the best in this regard.

@mkoeppe
Copy link
Contributor Author

mkoeppe commented Dec 7, 2020

comment:23

A discussion has unfolded in https://groups.google.com/g/sage-devel/c/qKqTmLzHAbg/m/6YxQ_qWUBQAJ

@mkoeppe

This comment has been minimized.

@mkoeppe mkoeppe changed the title Switch jsmol to jupyter-jsmol, make jmol optional Switch jsmol to jupyter-jsmol Dec 8, 2020
@sagetrac-git
Copy link
Mannequin

sagetrac-git mannequin commented Dec 8, 2020

Changed commit from ae970c2 to bf3c1dc

@sagetrac-git
Copy link
Mannequin

sagetrac-git mannequin commented Dec 8, 2020

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

e57f927Merge branch 't/31002/public/build/bdist_wsl' into t/30315/switch_jsmol_to_jupyter_jsmol__make_jmol_optional
017b816build/pkgs/jupyter_jsmol: New
db1b38cbuild/pkgs/jmol/spkg-install.in: No longer install jsmol
95a7131sage.repl.display.jsmol_iframe: Adjust path to jsmol
98b83f5sage.repl.ipython_kernel.install: No longer symlink jsmol
40a8823build/pkgs/jupyter_jsmol: Add dependencies, install-requires.txt
bf3c1dcbuild/pkgs/jupyter_jsmol/spkg-install.in: Do not rebuild the javascript file

@mkoeppe
Copy link
Contributor Author

mkoeppe commented Dec 8, 2020

Changed dependencies from #31002, #31020 to #31002

@mkoeppe
Copy link
Contributor Author

mkoeppe commented Dec 8, 2020

comment:26

I have narrowed this ticket to only separating the jmol and jsmol installation.

@mkoeppe
Copy link
Contributor Author

mkoeppe commented Dec 8, 2020

comment:27

Making jmol optional, and the discussion about new technologies to create images from 3d graphics, is now on #31027.

@jcamp0x2a
Copy link
Mannequin

jcamp0x2a mannequin commented Dec 8, 2020

comment:28

I was able to verify that JSmol still works correctly in both Jupyter and JupyterLab. However, I did notice a test failure in src/sage/repl/ipython_kernel/install.py due to the new deprecation warning:

File "src/sage/repl/ipython_kernel/install.py", line 142, in sage.repl.ipython_kernel.install.SageKernelSpec.use_local_jsmol
Failed example:
    spec.use_local_jsmol()
Expected nothing
Got:
    doctest:warning
      File "/home/joshua/sage/src/bin/sage-runtests", line 182, in <module>
        err = DC.run()
    [...]
    :
    DeprecationWarning: Symlinking jsmol is no longer necessary
    See https://trac.sagemath.org/30315 for details.

Also, after removing the local/share/jsmol folder since it is no longer needed (and wouldn't be present on a clean install), I get a couple more test failures in the same file:

File "src/sage/repl/ipython_kernel/install.py", line 144, in sage.repl.ipython_kernel.install.SageKernelSpec.use_local_jsmol
Failed example:
    os.path.isdir(jsmol)
Expected:
    True
Got:
    False

and

File "src/sage/repl/ipython_kernel/install.py", line 146, in sage.repl.ipython_kernel.install.SageKernelSpec.use_local_jsmol
Failed example:
    os.path.isfile(os.path.join(jsmol, "JSmol.min.js"))
Expected:
    True
Got:
    False

@sagetrac-git
Copy link
Mannequin

sagetrac-git mannequin commented Dec 9, 2020

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

f2a02d9Get rid of JSMOL_DIR
7732e2ebuild/pkgs/jmol/SPKG.rst: Update

@sagetrac-git
Copy link
Mannequin

sagetrac-git mannequin commented Dec 9, 2020

Changed commit from bf3c1dc to 7732e2e

@mkoeppe
Copy link
Contributor Author

mkoeppe commented Dec 9, 2020

comment:30

Thanks. This function, after all, does not make sense any more, so there's no point in trying to go through deprecation. I have now removed it completely (alongside with the variable JSMOL_DIR).

@jcamp0x2a
Copy link
Mannequin

jcamp0x2a mannequin commented Dec 9, 2020

comment:31

Looks good. JSmol still works in Jupyter and JupyterLab.

@jcamp0x2a
Copy link
Mannequin

jcamp0x2a mannequin commented Dec 9, 2020

Reviewer: Joshua Campbell

@mkoeppe
Copy link
Contributor Author

mkoeppe commented Dec 9, 2020

comment:32

Thank you!

@sagetrac-git
Copy link
Mannequin

sagetrac-git mannequin commented Dec 10, 2020

Changed commit from 7732e2e to 80a3db1

@sagetrac-git
Copy link
Mannequin

sagetrac-git mannequin commented Dec 10, 2020

Branch pushed to git repo; I updated commit sha1 and set ticket back to needs_review. New commits:

80a3db1build/pkgs/jupyter_jsmol/spkg-install.in: Use sdh_setup_bdist_wheel

@mkoeppe
Copy link
Contributor Author

mkoeppe commented Dec 10, 2020

comment:34

Forgot to actually use #31002 - this change is necessary for correct installation on WSL

@jcamp0x2a
Copy link
Mannequin

jcamp0x2a mannequin commented Dec 10, 2020

comment:35

Replying to @mkoeppe:

Forgot to actually use #31002 - this change is necessary for correct installation on WSL

Ok. I tested this on Ubuntu WSL earlier without any installation issues. If I understand #31002 correctly, the problem was occurring due to mounting Windows folders, which I'm not doing, so that's probably why I didn't run into it.

I've gone ahead and re-verified that JSmol still works in Jupyter and JupyterLab.

@mkoeppe
Copy link
Contributor Author

mkoeppe commented Dec 10, 2020

comment:36

Thanks again!

@vbraun
Copy link
Member

vbraun commented Dec 13, 2020

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

4 participants