#30915 closed enhancement (fixed)

Three.js: Upgrade to r122

Reported by: paulmasson Owned by:
Priority: major Milestone: sage-9.3
Component: graphics Keywords: threejs
Cc: gh-jcamp0x2a, fbissey, mkoeppe, charpent, enriqueartal Merged in:
Authors: Paul Masson, Joshua Campbell Reviewers: Joshua Campbell, Paul Masson, Matthias Koeppe
Report Upstream: N/A Work issues:
Branch: c097439 (Commits, GitHub, GitLab) Commit: c0974392afc80a51693d40896a21c8e9902a7f27
Dependencies: Stopgaps:

Status badges

Description


Change History (60)

comment:1 Changed 21 months ago by paulmasson

  • Branch set to u/paulmasson/threejs-r122

comment:2 Changed 21 months ago by paulmasson

  • Commit set to 9ad4bdc82faf192a8bcf86eeb7ab3a18150efc97
  • Status changed from new to needs_review

Joshua, should only need to do make build to test this. Downloading the file should be automatic.


New commits:

9ad4bdcUpgrade to r122

comment:3 Changed 21 months ago by paulmasson

  • Status changed from needs_review to needs_work

comment:4 Changed 21 months ago by fbissey

  • Cc fbissey added

comment:5 follow-up: Changed 21 months ago by gh-jcamp0x2a

make build appears to have downloaded and extracted the files successfully. All the right stuff (OrbitControls and the "fat" line classes) seem to appear in the three.min.js file extracted into local/share/threejs/build and running a diff on it against the script I built from your repo produces no output, so it looks good.

Of course, the HTML plots produced are still looking for OrbitControls.js and/or directing to mrdoob's repo on the CDN, but that's going to be a separate ticket?

comment:6 in reply to: ↑ 5 ; follow-up: Changed 21 months ago by paulmasson

Replying to gh-jcamp0x2a:

Of course, the HTML plots produced are still looking for OrbitControls.js and/or directing to mrdoob's repo on the CDN, but that's going to be a separate ticket?

No, I overlooked that before meeting a friend for dinner. Will fix tomorrow, but my friend pointed out another development: Mr.doob reversed himself and won’t be deprecating examples/js this year. I think I’m still in favor of this custom build rather than assembling a tarball because the entire process is smoother and the transition will happen at some point anyway. Feedback is welcome.

comment:7 in reply to: ↑ 6 Changed 21 months ago by gh-jcamp0x2a

Replying to paulmasson:

I think I’m still in favor of this custom build rather than assembling a tarball because the entire process is smoother and the transition will happen at some point anyway. Feedback is welcome.

I would agree. It also allows everything to be bundled up into a single javascript file instead of having to include OrbitControls and now all the line stuff separately. So I think it's a positive change even if examples/js were never deprecated.

comment:8 Changed 21 months ago by git

  • Commit changed from 9ad4bdc82faf192a8bcf86eeb7ab3a18150efc97 to 63530d2029ef7dd07f5437220ec02f27ded02457

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

63530d2Update script link generation

comment:9 Changed 21 months ago by paulmasson

  • Cc mkoeppe added
  • Status changed from needs_work to needs_review

comment:10 Changed 21 months ago by paulmasson

The new commit updates the link generated internally, both for online and offline use. I changed the name of the internal function to recognize this. Doctests pass on the two modified files.

Matthias, part of the reason I wanted the repository moved to the SageMath organization is that the online links must point to my personal repository on the CDN. If someone creates notebooks with these changes in place for online use, they may fail to work in the future when the repository is moved. If you're fine with that possibility, then we can procede with the upgrade.

Joshua, before testing this ticket again let's hear back from Matthais.

comment:11 follow-up: Changed 21 months ago by mkoeppe

Makes sense. Am I understanding correctly that jsdelivr.net refers to specific tags here, so when you update the repository, the old versions stick around and can continue to be referred to by plots that were generated earlier?

comment:12 in reply to: ↑ 11 Changed 21 months ago by paulmasson

Replying to mkoeppe:

Makes sense. Am I understanding correctly that jsdelivr.net refers to specific tags here, so when you update the repository, the old versions stick around and can continue to be referred to by plots that were generated earlier?

Once a file is in the CDN it is always available for future use and is indeed referenced by tag. According to the documentation the files remain even when a repository is deleted, so presumably it can handle movement of the repository between organizations. If you want us to proceed with the current setup then we can do that without too much worry after all.

comment:13 follow-up: Changed 21 months ago by mkoeppe

Thanks. And for the offline mode - does it make sense to make several versions available in SAGE_LOCAL?

comment:14 follow-up: Changed 21 months ago by mkoeppe

Also, does the offline mode depend at all on the symbolic link threejs that is installed in $SAGE_LOCAL/share/jupyter/nbextensions?

(My guess is that it does not and we can remove this symbolic link. At least that's what I gather from a comment in #30476 - it seems that threejs offline graphics also works with system jupyter notebook, which does not have that symbolic link.)

comment:15 in reply to: ↑ 13 ; follow-up: Changed 21 months ago by paulmasson

Replying to mkoeppe:

Thanks. And for the offline mode - does it make sense to make several versions available in SAGE_LOCAL?

No, it does not. The Three.js template is specifically tailored to the release currently in use. Mr. Doob sometimes breaks things. Backwards compatibility is not assured.

comment:16 in reply to: ↑ 14 ; follow-up: Changed 21 months ago by paulmasson

Replying to mkoeppe:

Also, does the offline mode depend at all on the symbolic link threejs that is installed in $SAGE_LOCAL/share/jupyter/nbextensions?

The system Jupyter notebook needs to find the local files for our Three.js viewer to work. As far as I know the symbolic link is how that happens. If you have a better idea in mind for letting the notebook know where the files are, then go for it. I'm still not clear on this question.

comment:17 in reply to: ↑ 16 ; follow-up: Changed 21 months ago by mkoeppe

  • Cc charpent enriqueartal added

Replying to paulmasson:

Replying to mkoeppe:

Also, does the offline mode depend at all on the symbolic link threejs that is installed in $SAGE_LOCAL/share/jupyter/nbextensions?

The system Jupyter notebook needs to find the local files for our Three.js viewer to work. As far as I know the symbolic link is how that happens. If you have a better idea in mind for letting the notebook know where the files are, then go for it. I'm still not clear on this question.

This seems to contradict what @charpent reported in #30476 regarding offline threejs in the system jupyter notebook. @charpent, @enriqueartal, could you help clarify this?

comment:18 in reply to: ↑ 15 ; follow-ups: Changed 21 months ago by gh-jcamp0x2a

Replying to paulmasson:

Replying to mkoeppe:

Thanks. And for the offline mode - does it make sense to make several versions available in SAGE_LOCAL?

No, it does not. The Three.js template is specifically tailored to the release currently in use. Mr. Doob sometimes breaks things. Backwards compatibility is not assured.

Hmm... perhaps it would be better to link the offline plots to a specific version.

For example, suppose I generate an offline plot using the template that works for r117. The plot links to /home/joshua/sage/local/share/threejs/build/three.min.js. Some time in the future I upgrade SageMath in-place. (Do normal users do that?) That old plot is now linking to three.js r122 despite the template it was generated from only being guaranteed to work with r117. Suddenly the plot may no longer work.

That's not an issue for the Jupyter notebook since it's a "live" environment and you can always just re-run the cell to get an up-to-date plot. It's also not an issue for the CDN scripts since those are tied to specific versions.

Although even if we fixed this, it'd still be fragile in that moving/deleting SageMath would also break old plots. To make a fully self-contained and isolated offline plot would probably require including the content of three.min.js inline instead of linking, but I imagine that might run afoul of the license.

comment:19 follow-up: Changed 21 months ago by mkoeppe

In any case, so that we can make progress here, I have forked the repo at https://github.com/sagemath/threejs-sage

comment:20 in reply to: ↑ 18 ; follow-up: Changed 21 months ago by mkoeppe

Replying to gh-jcamp0x2a:

Replying to paulmasson:

Replying to mkoeppe:

Thanks. And for the offline mode - does it make sense to make several versions available in SAGE_LOCAL?

No, it does not. The Three.js template is specifically tailored to the release currently in use. Mr. Doob sometimes breaks things. Backwards compatibility is not assured.

Hmm... perhaps it would be better to link the offline plots to a specific version.

For example, suppose I generate an offline plot using the template that works for r117. The plot links to /home/joshua/sage/local/share/threejs/build/three.min.js. Some time in the future I upgrade SageMath in-place. (Do normal users do that?)

Yes, that's a typical operation.

That old plot is now linking to three.js r122 despite the template it was generated from only being guaranteed to work with r117. Suddenly the plot may no longer work.

Right. So let's please do a versioned installation scheme to address this issue. Basically, upgrades should not break operation of older things.

That's not an issue for the Jupyter notebook since it's a "live" environment and you can always just re-run the cell to get an up-to-date plot.

Well, but isn't it an important part of the point of the notebook format that the outputs are also stored?

It's also not an issue for the CDN scripts since those are tied to specific versions.

Although even if we fixed this, it'd still be fragile in that moving/deleting SageMath would also break old plots.

(Moving the Sage tree is not supported.)

comment:21 in reply to: ↑ 20 ; follow-up: Changed 21 months ago by mkoeppe

Replying to mkoeppe:

That old plot is now linking to three.js r122 despite the template it was generated from only being guaranteed to work with r117. Suddenly the plot may no longer work.

Right. So let's please do a versioned installation scheme to address this issue. Basically, upgrades should not break operation of older things.

Again the key use case to consider is to have one ("system") installation of the Jupyter notebook or JupyterLab?, which the user uses to access multiple installations of different versions of the Sage kernel.

comment:22 in reply to: ↑ 18 ; follow-up: Changed 21 months ago by mkoeppe

Replying to gh-jcamp0x2a:

To make a fully self-contained and isolated offline plot would probably require including the content of three.min.js inline instead of linking,

Sounds like this would be a useful option.

but I imagine that might run afoul of the license.

Does a more restrictive license than MIT apply? https://github.com/sagemath/threejs-sage/blob/main/LICENSE

comment:23 in reply to: ↑ 22 Changed 21 months ago by gh-jcamp0x2a

Replying to mkoeppe:

Replying to gh-jcamp0x2a:

To make a fully self-contained and isolated offline plot would probably require including the content of three.min.js inline instead of linking,

Sounds like this would be a useful option.

but I imagine that might run afoul of the license.

Does a more restrictive license than MIT apply? https://github.com/sagemath/threejs-sage/blob/main/LICENSE

No, I don't believe so. I was thinking it was LGPL for some reason and how all that static vs. dynamic linking stuff would work with javascript. Sorry :)

comment:24 in reply to: ↑ 19 ; follow-up: Changed 21 months ago by paulmasson

Replying to mkoeppe:

In any case, so that we can make progress here, I have forked the repo at https://github.com/sagemath/threejs-sage

Matthias, to avoid any confusion with the repository, please delete your fork and I will initiate the transfer to the SageMath organization.

comment:25 in reply to: ↑ 17 ; follow-up: Changed 21 months ago by enriqueartal

I just checked again. With online=False, it uses the local files of threejs. What I did is to link the threejs folder of the sage installation in /usr/local/sage/local/share/jupyter/nbextensions/

The threejs version is r-117

Replying to mkoeppe:

Replying to paulmasson:

Replying to mkoeppe:

Also, does the offline mode depend at all on the symbolic link threejs that is installed in $SAGE_LOCAL/share/jupyter/nbextensions?

The system Jupyter notebook needs to find the local files for our Three.js viewer to work. As far as I know the symbolic link is how that happens. If you have a better idea in mind for letting the notebook know where the files are, then go for it. I'm still not clear on this question.

This seems to contradict what @charpent reported in #30476 regarding offline threejs in the system jupyter notebook. @charpent, @enriqueartal, could you help clarify this?

Last edited 21 months ago by enriqueartal (previous) (diff)

comment:26 in reply to: ↑ 24 ; follow-up: Changed 21 months ago by mkoeppe

Replying to paulmasson:

Replying to mkoeppe:

In any case, so that we can make progress here, I have forked the repo at https://github.com/sagemath/threejs-sage

Matthias, to avoid any confusion with the repository, please delete your fork and I will initiate the transfer to the SageMath organization.

It seems the confusion has already happened. I created this fork for you because you requested a repository in the sagemath github organization. If you find that this is not suitable for what you have in mind, try explaining.

comment:27 in reply to: ↑ 25 ; follow-up: Changed 21 months ago by mkoeppe

Replying to enriqueartal:

I just checked again. With online=False, it uses the local files of threejs. What I did is to link the threejs folder of the sage installation in /usr/local/sage/local/share/jupyter/nbextensions/

Can you please check whether it works without this symlink?

comment:28 in reply to: ↑ 26 Changed 21 months ago by paulmasson

Replying to mkoeppe:

Replying to paulmasson:

Replying to mkoeppe:

In any case, so that we can make progress here, I have forked the repo at https://github.com/sagemath/threejs-sage

Matthias, to avoid any confusion with the repository, please delete your fork and I will initiate the transfer to the SageMath organization.

It seems the confusion has already happened. I created this fork for you because you requested a repository in the sagemath github organization. If you find that this is not suitable for what you have in mind, try explaining.

Sorry! When I requested the repository, I was planning to do all the work there from the start. Then I went ahead and made my own to get things going. At this point, I could work in your fork and delete mine and GitHub should then treat yours as the new master repository. Rather than rely on that happening, I think it will be cleaner for me to transfer mine to you, so that there is no possibility of a mix up. I need to initiate the transfer and was waiting for you to be ready for it.

comment:29 follow-up: Changed 21 months ago by mkoeppe

In git, there is no notion of a master repository. The repository I created for you is indistinguishable from yours: It has the same branches and tags.

comment:30 in reply to: ↑ 18 Changed 21 months ago by paulmasson

Replying to gh-jcamp0x2a:

Although even if we fixed this, it'd still be fragile in that moving/deleting SageMath would also break old plots. To make a fully self-contained and isolated offline plot would probably require including the content of three.min.js inline instead of linking, but I imagine that might run afoul of the license.

Joshua, I always envisioned the viewer as being used to share graphics on the web, since that's how I work and that's the future. I have yet to hear of anyone having encountered this problem. While I appreciate your concerns, I think we can let this lie until someone actually complains about it.

And you could deal with the license by just copying it into the body of the document along with the library's code.

comment:31 in reply to: ↑ 29 ; follow-up: Changed 21 months ago by paulmasson

Replying to mkoeppe:

In git, there is no notion of a master repository. The repository I created for you is indistinguishable from yours: It has the same branches and tags.

It does for the moment, but it won't update itself automatically. Someone would need to do that. And I used the wrong term: GitHub uses parent. Please read the section on deleting public repositories.

comment:32 in reply to: ↑ 31 ; follow-up: Changed 21 months ago by mkoeppe

Replying to paulmasson:

Replying to mkoeppe:

In git, there is no notion of a master repository. The repository I created for you is indistinguishable from yours: It has the same branches and tags.

It does for the moment, but it won't update itself automatically.

Yes, obviously you would need to start using the repository.

But fine, I can redo it. Give me admin access to your repo and I'll transfer it.

comment:33 in reply to: ↑ 21 ; follow-up: Changed 21 months ago by paulmasson

Replying to mkoeppe:

Replying to mkoeppe:

That old plot is now linking to three.js r122 despite the template it was generated from only being guaranteed to work with r117. Suddenly the plot may no longer work.

Right. So let's please do a versioned installation scheme to address this issue. Basically, upgrades should not break operation of older things.

Again the key use case to consider is to have one ("system") installation of the Jupyter notebook or JupyterLab, which the user uses to access multiple installations of different versions of the Sage kernel.

Matthias, the viewer was designed from the beginning for use in Sage only, so for me it's part of the kernel. Are you thinking of having us split it off completely? Otherwise the version of Three.js will track with the version of Sage.

comment:34 in reply to: ↑ 33 ; follow-up: Changed 21 months ago by mkoeppe

Replying to paulmasson:

Replying to mkoeppe:

Again the key use case to consider is to have one ("system") installation of the Jupyter notebook or JupyterLab, which the user uses to access multiple installations of different versions of the Sage kernel.

Matthias, the viewer was designed from the beginning for use in Sage only, so for me it's part of the kernel. Are you thinking of having us split it off completely? Otherwise the version of Three.js will track with the version of Sage.

The use case I am describing uses one system Jupyter installation ("frontend") that the user uses to access several kernels, including several Sage kernels installed in different directories. The frontend may not come from Sage at all, but we need to find a way so that each of the Sage kernels fully works, and that includes the 3d graphics.

comment:35 Changed 21 months ago by mkoeppe

(In #30476 we are putting together documentation on how to configure such a setup.)

comment:36 in reply to: ↑ 32 ; follow-up: Changed 21 months ago by mkoeppe

Replying to mkoeppe:

Give me admin access to your repo and I'll transfer it.

I received and accepted the repo invite but will need additional permissions to make the transfer.

comment:37 in reply to: ↑ 36 ; follow-up: Changed 21 months ago by paulmasson

Replying to mkoeppe:

Replying to mkoeppe:

Give me admin access to your repo and I'll transfer it.

I received and accepted the repo invite but will need additional permissions to make the transfer.

I don't see any options other than just inviting you. Do you not have access to Settings/Options at this point?

comment:38 in reply to: ↑ 37 ; follow-up: Changed 21 months ago by mkoeppe

Replying to paulmasson:

Do you not have access to Settings/Options at this point?

No

comment:39 in reply to: ↑ 38 Changed 21 months ago by paulmasson

Replying to mkoeppe:

Replying to paulmasson:

Do you not have access to Settings/Options at this point?

No

Then we'll go with Plan B: since the fork is still there, I'll work in it and delete mine at some point. Good to go. Thanks!

comment:40 in reply to: ↑ 27 ; follow-up: Changed 21 months ago by enriqueartal

Replying to mkoeppe:

Replying to enriqueartal:

I just checked again. With online=False, it uses the local files of threejs. What I did is to link the threejs folder of the sage installation in /usr/local/sage/local/share/jupyter/nbextensions/

Can you please check whether it works without this symlink?

I tried this time inspecting. Without the symlink and with online=False, it works since it looks for the threejs files locally and fails, and then it tries online successfully. I rechecked with the symlink and it finds the local files.

BTW, with jupyterhub, it uses internet pages, not local ones even with the symlink

Last edited 21 months ago by enriqueartal (previous) (diff)

comment:41 in reply to: ↑ 40 Changed 21 months ago by mkoeppe

Replying to enriqueartal:

Replying to mkoeppe:

Replying to enriqueartal:

I just checked again. With online=False, it uses the local files of threejs. What I did is to link the threejs folder of the sage installation in /usr/local/sage/local/share/jupyter/nbextensions/

Can you please check whether it works without this symlink?

I tried this time inspecting.

Thanks a lot for investigating!

Without the symlink and with online=False, it works since it looks for the threejs files locally

By "locally", do you mean through the notebook webserver?

and fails, and then it tries online successfully. I rechecked with the symlink and it finds the local files.

Thanks.

BTW, with jupyterhub, it uses internet pages, not local ones even with the symlink

Sounds like we would need to install files using jupyter labextension to support this.

Last edited 21 months ago by mkoeppe (previous) (diff)

comment:42 Changed 21 months ago by git

  • Commit changed from 63530d2029ef7dd07f5437220ec02f27ded02457 to 82b8888f142d193ada736c32d1f8e358a6380878

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

82b8888Update online link

comment:43 follow-up: Changed 21 months ago by paulmasson

Joshua, ready for another review. Thanks!

comment:44 in reply to: ↑ 34 ; follow-up: Changed 21 months ago by mkoeppe

Replying to mkoeppe:

Replying to paulmasson:

Matthias, the viewer was designed from the beginning for use in Sage only, so for me it's part of the kernel. Are you thinking of having us split it off completely? Otherwise the version of Three.js will track with the version of Sage.

The use case I am describing uses one system Jupyter installation ("frontend") that the user uses to access several kernels, including several Sage kernels installed in different directories. The frontend may not come from Sage at all, but we need to find a way so that each of the Sage kernels fully works, and that includes the 3d graphics.

Paul, can we try to make progress on this question please? Is what I am describing here clear now? I'll be happy to explain more if necessary.

comment:45 Changed 21 months ago by git

  • Commit changed from 82b8888f142d193ada736c32d1f8e358a6380878 to 55449d83b20c85a9e80db210e5e2b2325987db0b

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

55449d8Update upstream URL

comment:46 in reply to: ↑ 44 ; follow-up: Changed 21 months ago by paulmasson

Replying to mkoeppe:

Replying to mkoeppe:

Replying to paulmasson:

Matthias, the viewer was designed from the beginning for use in Sage only, so for me it's part of the kernel. Are you thinking of having us split it off completely? Otherwise the version of Three.js will track with the version of Sage.

The use case I am describing uses one system Jupyter installation ("frontend") that the user uses to access several kernels, including several Sage kernels installed in different directories. The frontend may not come from Sage at all, but we need to find a way so that each of the Sage kernels fully works, and that includes the 3d graphics.

Paul, can we try to make progress on this question please? Is what I am describing here clear now? I'll be happy to explain more if necessary.

Unfortunately I am not clear as to what you expect of me regarding this. The Sage files have to be somewhere and the notebook needs to now how to find them. If that is already necessary, why won't the notebook know how to find the Three.js file? Are you planning on eliminating src/sage/ext_data?

Maybe this should be a separate ticket. This one is really just to upgrade the version with the existing structure already in place.

comment:47 in reply to: ↑ 46 ; follow-ups: Changed 21 months ago by mkoeppe

Replying to paulmasson:

Are you planning on eliminating src/sage/ext_data?

No. The issue is about share/jupyter/nbextensions/threejs and the fact that there will be several unrelated copies of that.

Here is an example.

  • Let's say system jupyter is installed in /usr, so the notebook server is accessing /usr/share/jupyter/nbextensions.
  • Sage-9.x may be installed in /home/x/sage-9.x/local/ and may need threejs r122.
  • Sage-9.y may be installed in /home/x/sage-9.y/local/ and may need threejs r155.
  • Let's say r122 and r155 are mutually incompatible.

As @enriqueartal has reported above, our offline threejs graphics needs /usr/share/jupyter/nbextensions/threejs/. But the system does not provide it -- it must come from Sage. If we create a symlink /usr/share/jupyter/nbextensions/threejs -> /home/x/sage-9.x/local/share/jupyter/nbextensions/threejs, then Sage 9.x will work with the system jupyter notebook; but Sage 9.y will not.

Do you agree?

So this means that we need a versioned installation scheme. The details of this installation scheme is what I am trying to discuss with you, in particular because it is certainly related to that new repository.

Last edited 21 months ago by mkoeppe (previous) (diff)

comment:48 in reply to: ↑ 43 ; follow-up: Changed 21 months ago by gh-jcamp0x2a

  • Authors changed from Paul Masson to Paul Masson, Joshua Campbell
  • Branch changed from u/paulmasson/threejs-r122 to u/gh-jcamp0x2a/threejs-r122
  • Commit changed from 55449d83b20c85a9e80db210e5e2b2325987db0b to eed14d3dc1b9f69cada9a54bb377f94ad145ec3f

Replying to paulmasson:

Joshua, ready for another review. Thanks!

Hi Paul, I found a failing doctest in display_manager.py due to the CDN URL change. I hope you don't mind, but I took the liberty of pushing a fix for it. I also went ahead and modified it to read from that new version file instead of relying on the REVISION= stuff, and I also renamed a few more occurrences of "scripts" to "script" as you had done in your commit.

If you are okay with the additional changes, I think you or I could mark it as positive review.

comment:49 in reply to: ↑ 47 Changed 21 months ago by gh-jcamp0x2a

Replying to mkoeppe:

So this means that we need a versioned installation scheme. The details of this installation scheme is what I am trying to discuss with you, in particular because it is certainly related to that new repository.

Including the three.min.js inline for offline use (whether jupyter or command line) seems like it would clear up this headache for three.js at least. No more local file paths.

I'd like to get this ticket merged as-is so I can make some headway on my "fat" lines branch. So perhaps a follow-up ticket?

comment:50 in reply to: ↑ 47 Changed 21 months ago by paulmasson

Replying to mkoeppe:

Replying to paulmasson:

Are you planning on eliminating src/sage/ext_data?

No. The issue is about share/jupyter/nbextensions/threejs and the fact that there will be several unrelated copies of that.

Here is an example.

  • Let's say system jupyter is installed in /usr, so the notebook server is accessing /usr/share/jupyter/nbextensions.
  • Sage-9.x may be installed in /home/x/sage-9.x/local/ and may need threejs r122.
  • Sage-9.y may be installed in /home/x/sage-9.y/local/ and may need threejs r155.
  • Let's say r122 and r155 are mutually incompatible.

As @enriqueartal has reported above, our offline threejs graphics needs /usr/share/jupyter/nbextensions/threejs/. But the system does not provide it -- it must come from Sage. If we create a symlink /usr/share/jupyter/nbextensions/threejs -> /home/x/sage-9.x/local/share/jupyter/nbextensions/threejs, then Sage 9.x will work with the system jupyter notebook; but Sage 9.y will not.

Do you agree?

So this means that we need a versioned installation scheme. The details of this installation scheme is what I am trying to discuss with you, in particular because it is certainly related to that new repository.

Moving discussion to #30972

comment:51 in reply to: ↑ 48 Changed 21 months ago by paulmasson

  • Reviewers set to Joshua Campbell, Paul Masson
  • Status changed from needs_review to positive_review

Replying to gh-jcamp0x2a:

Replying to paulmasson:

Joshua, ready for another review. Thanks!

Hi Paul, I found a failing doctest in display_manager.py due to the CDN URL change. I hope you don't mind, but I took the liberty of pushing a fix for it. I also went ahead and modified it to read from that new version file instead of relying on the REVISION= stuff, and I also renamed a few more occurrences of "scripts" to "script" as you had done in your commit.

If you are okay with the additional changes, I think you or I could mark it as positive review.

LGTM

comment:52 Changed 21 months ago by mkoeppe

  • Status changed from positive_review to needs_work

Public methods such as threejs_offline_scripts should not be renamed without deprecation

comment:53 follow-up: Changed 21 months ago by paulmasson

Joshua, would you just revert the method name? It's only used internally and I no longer care if the script name doesn't match the output.

comment:54 Changed 21 months ago by git

  • Commit changed from eed14d3dc1b9f69cada9a54bb377f94ad145ec3f to c0974392afc80a51693d40896a21c8e9902a7f27

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

c097439Revert threejs_scripts method name changes

comment:55 in reply to: ↑ 53 Changed 21 months ago by gh-jcamp0x2a

  • Status changed from needs_work to needs_review

Replying to paulmasson:

Joshua, would you just revert the method name? It's only used internally and I no longer care if the script name doesn't match the output.

Done. Did a quick sanity check and it still runs without issue in all four combinations of command-line/jupyter and online/offline.

comment:56 Changed 21 months ago by mkoeppe

  • Reviewers changed from Joshua Campbell, Paul Masson to Joshua Campbell, Paul Masson, Matthias Koeppe
  • Status changed from needs_review to positive_review

comment:57 follow-up: Changed 21 months ago by mkoeppe

Joshua, by the way, have you tested if the offline mode works with JupyterLab too?

comment:58 in reply to: ↑ 57 ; follow-up: Changed 21 months ago by gh-jcamp0x2a

Replying to mkoeppe:

Joshua, by the way, have you tested if the offline mode works with JupyterLab too?

Yes, sorry, that's actually what I meant by "jupyter". I've been using it in preference to the traditional notebook lately. I've gone back and tested with the traditional notebook and it works fine as well.

Oh, and I'm going to try to spend some time later today looking at your #30972.

comment:59 in reply to: ↑ 58 Changed 21 months ago by mkoeppe

Replying to gh-jcamp0x2a:

Replying to mkoeppe:

Joshua, by the way, have you tested if the offline mode works with JupyterLab too?

Yes, sorry, that's actually what I meant by "jupyter". I've been using it in preference to the traditional notebook lately. I've gone back and tested with the traditional notebook and it works fine as well.

Great, thanks, I was not sure whether the JupyterLab server also serves the /nbextensions path.

comment:60 Changed 21 months ago by vbraun

  • Branch changed from u/gh-jcamp0x2a/threejs-r122 to c0974392afc80a51693d40896a21c8e9902a7f27
  • Resolution set to fixed
  • Status changed from positive_review to closed
Note: See TracTickets for help on using tickets.