Opened 2 years ago

Closed 4 months ago

#26434 closed enhancement (duplicate)

Do not require installed_packages() to work for three.js inclusion & show a warning if three.js cannot be found locally

Reported by: saraedum Owned by:
Priority: major Milestone: sage-duplicate/invalid/wontfix
Component: distribution Keywords: threejs, conda, installed_packages
Cc: gh-timokau, fbissey, arojas, egourgoulhon Merged in:
Authors: Julian Rüth Reviewers: Paul Masson
Report Upstream: N/A Work issues: needs testing - but is the general approach ok?
Branch: u/saraedum/26434 (Commits) Commit: be72dbe9745aae80e364912f0f9c9300de3b9a56
Dependencies: Stopgaps:

Description (last modified by saraedum)

On conda, we are seeing failing doctests because installed_packages() is used:

File "src/sage/plot/plot3d/base.pyx", line 361, in sage.plot.plot3d.base.Graphics3d._rich_repr_threejs
Failed example:
    sphere(online=True)._rich_repr_threejs()
Exception raised:
    Traceback (most recent call last):
      File "/builds/saraedum/conda-sagelib-tests/sagemath/local/lib/python2.7/site-packages/sage/doctest/forker.py", line 573, in _run
        self.compile_and_execute(example, compiler, test.globs)
      File "/builds/saraedum/conda-sagelib-tests/sagemath/local/lib/python2.7/site-packages/sage/doctest/forker.py", line 983, in compile_and_execute
        exec(compiled, globs)
      File "<doctest sage.plot.plot3d.base.Graphics3d._rich_repr_threejs[0]>", line 1, in <module>
        sphere(online=True)._rich_repr_threejs()
      File "sage/plot/plot3d/base.pyx", line 377, in sage.plot.plot3d.base.Graphics3d._rich_repr_threejs (build/cythonized/sage/plot/plot3d/base.c:9311)
        scripts = get_display_manager().threejs_scripts(options['online'])
      File "/builds/saraedum/conda-sagelib-tests/sagemath/local/lib/python2.7/site-packages/sage/repl/rich_output/display_manager.py", line 749, in threejs_scripts
        version = installed_packages()['threejs'].split('.')[0]
    KeyError: 'threejs'

This ticket aims to make installed_packages() here optional but also to simplify the threejs inclusion code that appears to have been duplicated and has since diverged.

If you think that this is fixing two separate issues and should be split into two separate tickets, feel free to make such a change.

Change History (48)

comment:1 Changed 2 years ago by saraedum

  • Branch set to u/saraedum/26434

comment:2 Changed 2 years ago by saraedum

  • Commit set to 9ea40f299351b4907da3b2f3b7fb93af0de7a184

I CCed the usual packaging suspects; I have not tested this at all (Sage is rebuilding…)


New commits:

9ea40f2Make three.js inclusion packager friendly

comment:3 Changed 2 years ago by git

  • Commit changed from 9ea40f299351b4907da3b2f3b7fb93af0de7a184 to 5aa1b44d0f2ca87237bd7173f49350a5a13bce03

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

5aa1b44Make three.js inclusion packager friendly

comment:4 Changed 2 years ago by fbissey

Ideally we would have a threejs config helper reporting its own version. That's the pipedream.

comment:5 follow-up: Changed 2 years ago by saraedum

I looked into making threejs a feature and reporting the version there. But it appears that threejs has no version information readily available. Even if it had that information I am not sure what we would do with it. Requiring r80 seems to be wrong as we are using a very small subset of threejs so I suspect that almost any version of threejs would work.

comment:6 Changed 2 years ago by fbissey

I agree, I think master is the one that give you the latest online. But the idea that you should use exactly the same version on and offline is not unsensible in itself.

comment:7 follow-up: Changed 2 years ago by gh-timokau

Why doesn't the example

get_display_manager().threejs_scripts(from_cdn=True)

require an # optional - internet tag? Apparently it doesn't becasue it didn't have one before either, but it seems like it should?

But it appears that threejs has no version information readily available.

Grep shows that at least on nix the version is present in package.json and package-lock.json. Not sure if that is build system specific or anything.

Why would anyone want to use the CDN version anyways?

comment:8 in reply to: ↑ 7 ; follow-up: Changed 2 years ago by saraedum

Replying to gh-timokau:

Why doesn't the example

get_display_manager().threejs_scripts(from_cdn=True)

require an # optional - internet tag? Apparently it doesn't becasue it didn't have one before either, but it seems like it should?

This just generates <script> tags. It doesn't download the scripts that these tags refer to.

But it appears that threejs has no version information readily available.

Grep shows that at least on nix the version is present in package.json and package-lock.json. Not sure if that is build system specific or anything.

These files are npm metadata. I don't think that all disrtibutions include them.

Why would anyone want to use the CDN version anyways?

We could remove the CDN option but it's probably there for a reason. Why would you want to use it? Well "it feels right" from a web-dev perspective...maybe you don't have threejs installed (in a compatible version)? Or you want to publish this somewhere online and don't want to serve all these script tags yourself?

comment:9 in reply to: ↑ 8 Changed 2 years ago by gh-timokau

Makes sense. These changes look good to me, if the doctests pass. Definite improvement, thank you!

comment:10 Changed 2 years ago by git

  • Commit changed from 5aa1b44d0f2ca87237bd7173f49350a5a13bce03 to 2ec0979ee4eedf068f9bbb2fd91b1a26f59d7dc6

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

2ec0979Import missing modules for threejs changes

comment:11 Changed 2 years ago by saraedum

I checked that this still works in jupyter and in sagenb. Let's see what the patchbots think about it.


New commits:

2ec0979Import missing modules for threejs changes

comment:12 Changed 20 months ago by saraedum

  • Status changed from new to needs_review

comment:13 Changed 20 months ago by saraedum

gh-timokau, does this mean that we can set this to positive review now?

comment:14 Changed 20 months ago by saraedum

Hm…now I am confused with the current discussion on sage-devel whether this would change the online keyword of show() to from_cdn

comment:15 Changed 20 months ago by gh-timokau

I don't know enough about three.js usages to give it a proper review myself, but I don't have any objections.

comment:16 Changed 20 months ago by git

  • Commit changed from 2ec0979ee4eedf068f9bbb2fd91b1a26f59d7dc6 to 1b27f44f52a36ac8d548f787c09cf61f5ec5aede

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

5bc0862Detect missing three.js
a3d28f3Support online parameter name
1b27f44fix typo

comment:17 Changed 20 months ago by saraedum

  • Description modified (diff)
  • Summary changed from Do not require installed_packages() to work for three.js inclusion to Do not require installed_packages() to work for three.js inclusion & show a warning if three.js cannot be found locally

comment:18 Changed 20 months ago by fbissey

Fells like we lost the original goal of the ticket there. install_package() is needed if you don't want sage to break.

comment:19 Changed 20 months ago by git

  • Commit changed from 1b27f44f52a36ac8d548f787c09cf61f5ec5aede to 17fa3cc6a898091a6c7abb5a05e1a88a817495e1

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

17fa3ccRemove option to load three.js from CDN

comment:20 follow-up: Changed 20 months ago by novoselt

So the way to deal with locally served scripts is not to serve them at all but rather dump the whole script into the output??? And if there are multiple plots, then this code will be repeated and executed again and again and scripts won't be cached? Maybe all of this does not matter in practice because computers are so fast etc., but it surely seems wrong to me. I liked the old approach much better even if it required each backend to specify where to find these scripts.


New commits:

17fa3ccRemove option to load three.js from CDN

comment:21 Changed 20 months ago by novoselt

And now remove functionality to load sensible scripts completely?!

comment:22 Changed 20 months ago by saraedum

But there does not seem to be an easy way to do this without dumping it all into the output. It's 500K per plot. That's annoying, sure, but at least it works (last time I checked at least) and nobody is going to complain about privacy.

At the meeting in Luminy last week we had virtually everybody who tried Sage stumble upon this one.

Since three.js is a standard package this should also be acceptable for packagers. They need to set the THREEJS environment variable and then the right scripts should be included.

comment:23 Changed 20 months ago by saraedum

I agree that it's not pretty. But it fixes a bug and reduces the code base, that seems like a sensible solution for me until somebody sits down to find out how to do this properly in Jupyter & CoCalc?.

comment:24 Changed 20 months ago by saraedum

  • Work issues set to needs testing - but is the general approach ok?

comment:25 in reply to: ↑ 20 Changed 20 months ago by saraedum

Replying to novoselt:

And if there are multiple plots, then this code will be repeated and executed again and again and scripts won't be cached?

As I understand this, only the "caching" part of the script is different. The script would have been executed for every plot anyway as it was included in every plot with a <script src="…"> tag.

comment:26 follow-up: Changed 20 months ago by novoselt

The current situation is:

  • by default scripts are served offline and it works in some cases
  • there is an option to easily get output with scripts served online from CDN which also has its use cases and works everywhere, as I understand, but should not be made the default
  • there are cases (important ones) where offline does not work

How about:

  • keep the existing functionality of serving "offline" scripts
  • keep the existing functionality of serving "online/CDN" scripts
  • add your new option to inject the whole code instead of script tags

The default should be then "use offline scripts if possible, but inject the code if it does not work". Is this realizable? If not, then at least please keep the option to use CDN on request, what harm comes from that?!

comment:27 in reply to: ↑ 5 Changed 19 months ago by paulmasson

Replying to saraedum:

I looked into making threejs a feature and reporting the version there. But it appears that threejs has no version information readily available. Even if it had that information I am not sure what we would do with it. Requiring r80 seems to be wrong as we are using a very small subset of threejs so I suspect that almost any version of threejs would work.

Please be careful regarding versions: any version will generally not work. Three.js is almost never backward compatible. That’s just how Mr.doob works, and if we want to use his code we need to be aware of that.

comment:28 Changed 19 months ago by saraedum

Please note that the proposed change always adds the locally installed version. That version should be compatible (the packager should make sure of that.)

comment:29 in reply to: ↑ 26 Changed 19 months ago by saraedum

Replying to novoselt:

The current situation is:

  • by default scripts are served offline and it works in some cases
  • there is an option to easily get output with scripts served online from CDN which also has its use cases and works everywhere, as I understand, but should not be made the default
  • there are cases (important ones) where offline does not work

How about:

  • keep the existing functionality of serving "offline" scripts
  • keep the existing functionality of serving "online/CDN" scripts
  • add your new option to inject the whole code instead of script tags

The default should be then "use offline scripts if possible, but inject the code if it does not work". Is this realizable? If not, then at least please keep the option to use CDN on request, what harm comes from that?!

The only harm that comes with this is that it creates more code to maintain. As opposed to only supporting one of these options.

The current situation is that this is broken for lots of users and they don't understand what's going on. (It was the most frequent issue people had with Sage at a recent introductory session of Sage.) I don't care at all how we fix this but we should just make it work by default everywhere. This here is one proposal to fix it. If it cannot be agreed upon, I am fine with anything else that works out of the box really. But I would really like to see this finally fixed. We can still improve upon this in a followup :)

comment:30 Changed 16 months ago by git

  • Commit changed from 17fa3cc6a898091a6c7abb5a05e1a88a817495e1 to be72dbe9745aae80e364912f0f9c9300de3b9a56

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

be72dbeMerge remote-tracking branch 'trac/develop' into u/saraedum/26434

comment:31 follow-up: Changed 16 months ago by saraedum

I hope that this approach is an improvement at least. The CDN option is broken imho as it relies on installed_packages().

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

Replying to saraedum:

I hope that this approach is an improvement at least. The CDN option is broken imho as it relies on installed_packages().

I'm sure I don't fully understand the problems created by installed_packages(), but the CDN option is important for sharing notebooks and/or stand-alone graphics. Please do not just remove it completely!

comment:33 in reply to: ↑ 32 Changed 16 months ago by fbissey

Replying to paulmasson:

Replying to saraedum:

I hope that this approach is an improvement at least. The CDN option is broken imho as it relies on installed_packages().

I'm sure I don't fully understand the problems created by installed_packages(), but the CDN option is important for sharing notebooks and/or stand-alone graphics. Please do not just remove it completely!

It is a bit like you hardcoding a call to rpm in your application to figure something out. Forget about apt or anything else.

comment:34 Changed 16 months ago by paulmasson

  • Cc egourgoulhon added

Adding Eric Gourgoulhon for input on sharing notebooks

comment:35 follow-up: Changed 16 months ago by paulmasson

Another option is to hard-code the version of Three.js each time an upgrade is done. I think that's what I originally did but was persuaded to use installed_packages() to make upgrades easier. If there is a THREEJSDIR variable why not add a THREEJSVER variable as well? Then a note can be added to spkg-src to remind the upgrader to change that variable as well.

I'm a web-oriented person: a big part of the reason I wrote the Three.js backend was to encourage sharing of interactive graphics over the web. I would hate to see all that functionality just disappear.

comment:36 follow-up: Changed 16 months ago by fbissey

I can see the point of the advice you were given. However it ignored the push to package on distro. Ideally the information should be queryable from the installed stuff - without requiring a package manager. Hard coding is OK for me but I would favor something I can ask from the package itself somehow. Even if it is just manually parsing a file to find a version string.

comment:37 in reply to: ↑ 36 Changed 16 months ago by paulmasson

Replying to fbissey:

I can see the point of the advice you were given. However it ignored the push to package on distro. Ideally the information should be queryable from the installed stuff - without requiring a package manager. Hard coding is OK for me but I would favor something I can ask from the package itself somehow. Even if it is just manually parsing a file to find a version string.

The main three.min.js file has a string of the form REVISION="100" so that the version is available in the browser. How about parsing the file for that when Sage loads?

comment:38 Changed 16 months ago by fbissey

Works for me. You will understand that it is hard to figure something like that exists from just looking at the file :) I am glad we had this chat with you as the author to figure it out.

comment:39 in reply to: ↑ 35 Changed 16 months ago by saraedum

Replying to paulmasson:

I'm a web-oriented person: a big part of the reason I wrote the Three.js backend was to encourage sharing of interactive graphics over the web. I would hate to see all that functionality just disappear.

The functionality does not appear. The plots just get much bigger because they contain the inline three.js. I am not proud of this, but at runtime it does not make that much of a difference to load three.js several times, I suppose.

comment:40 follow-ups: Changed 16 months ago by saraedum

Ok. So I can parse the REVISION to keep the CDN option alive. But the default would be to include all of three.js so things just work? (We could improve upon this further in a followup, e.g., add three.js to the sage kernel so it's only loaded once when in a Jupyter context.)

comment:41 Changed 16 months ago by saraedum

  • Status changed from needs_review to needs_info

comment:42 in reply to: ↑ 40 Changed 16 months ago by fbissey

Replying to saraedum:

Ok. So I can parse the REVISION to keep the CDN option alive. But the default would be to include all of three.js so things just work? (We could improve upon this further in a followup, e.g., add three.js to the sage kernel so it's only loaded once when in a Jupyter context.)

I am happy with that.

comment:43 in reply to: ↑ 40 ; follow-ups: Changed 16 months ago by paulmasson

Replying to saraedum:

Ok. So I can parse the REVISION to keep the CDN option alive. But the default would be to include all of three.js so things just work? (We could improve upon this further in a followup, e.g., add three.js to the sage kernel so it's only loaded once when in a Jupyter context.)

The original purpose of distinguishing between offline/online scripts was to use local files by default. One could then choose to set online=True to use the CDN for files that were meant to be shared. The Three.js template also has the silent fallback to the CDN that was fixed in #27575, so that even a notebook generated with the default of local files would work when shared on the web.

I would like to see the original idea kept alive: first use local files, then fall back to the CDN as necessary or by choice. The only time you would want to dump all of Three.js into a file is when there is no Internet connectivity at all, but how often does that happen nowadays?

How about if this ticket just parses three.min.js for the version number to get rid of installed_packages() and doesn't change anything else yet? Then see what other issues arise in practice. Although you'll want to use THREEJSDIR in the local script tags.

comment:44 in reply to: ↑ 43 Changed 16 months ago by saraedum

Replying to paulmasson:

Replying to saraedum:

Ok. So I can parse the REVISION to keep the CDN option alive. But the default would be to include all of three.js so things just work? (We could improve upon this further in a followup, e.g., add three.js to the sage kernel so it's only loaded once when in a Jupyter context.)

The original purpose of distinguishing between offline/online scripts was to use local files by default. One could then choose to set online=True to use the CDN for files that were meant to be shared. The Three.js template also has the silent fallback to the CDN that was fixed in #27575, so that even a notebook generated with the default of local files would work when shared on the web.

I would like to see the original idea kept alive: first use local files, then fall back to the CDN as necessary or by choice. The only time you would want to dump all of Three.js into a file is when there is no Internet connectivity at all, but how often does that happen nowadays?

How about if this ticket just parses three.min.js for the version number to get rid of installed_packages() and doesn't change anything else yet? Then see what other issues arise in practice. Although you'll want to use THREEJSDIR in the local script tags.

That also works for me. I had forgotten about #27575 in the meantime.

comment:45 in reply to: ↑ 43 Changed 16 months ago by egourgoulhon

Replying to paulmasson:

I would like to see the original idea kept alive: first use local files, then fall back to the CDN as necessary or by choice. The only time you would want to dump all of Three.js into a file is when there is no Internet connectivity at all, but how often does that happen nowadays?

+1

comment:46 Changed 16 months ago by paulmasson

The intent of this ticket is addressed in #28086 so this one can be closed

comment:47 Changed 7 months ago by paulmasson

  • Milestone changed from sage-8.4 to sage-duplicate/invalid/wontfix
  • Reviewers set to Paul Masson
  • Status changed from needs_info to positive_review

Closing as no longer needed. Reopen if you disagree.

comment:48 Changed 4 months ago by chapoton

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