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

[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.

[saraedum]

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

---

New commits:

​9ea40f2

Make three.js inclusion packager friendly

[git]

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

​5aa1b44

Make three.js inclusion packager friendly

[fbissey]

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

[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.

[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.

[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?

[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?

[gh-timokau]

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

[git]

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

​2ec0979

Import missing modules for threejs changes

[saraedum]

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

---

New commits:

​2ec0979

Import missing modules for threejs changes

[saraedum]

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

[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…

[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.

[git]

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

​5bc0862	Detect missing three.js
​a3d28f3	Support online parameter name
​1b27f44	fix typo

[fbissey]

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

[git]

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

​17fa3cc

Remove option to load three.js from CDN

[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:

​17fa3cc

Remove option to load three.js from CDN

[novoselt]

And now remove functionality to load sensible scripts completely?!

[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.

[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?.

[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.

[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?!

[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.

[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.)

[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 :)

[git]

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

​be72dbe

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

[saraedum]

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

[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!

[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.

[paulmasson]

Adding Eric Gourgoulhon for input on sharing notebooks

[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.

[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.

[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?

[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.

[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.

[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.)

[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.

[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.

[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.

[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

[paulmasson]

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

[paulmasson]

Closing as no longer needed. Reopen if you disagree.