Opened 3 years ago
Closed 10 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, GitHub, GitLab) | Commit: | be72dbe9745aae80e364912f0f9c9300de3b9a56 |
Dependencies: | Stopgaps: |
Description (last modified by )
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 3 years ago by
- Branch set to u/saraedum/26434
comment:2 Changed 3 years ago by
- Commit set to 9ea40f299351b4907da3b2f3b7fb93af0de7a184
comment:3 Changed 3 years ago by
- Commit changed from 9ea40f299351b4907da3b2f3b7fb93af0de7a184 to 5aa1b44d0f2ca87237bd7173f49350a5a13bce03
Branch pushed to git repo; I updated commit sha1. This was a forced push. New commits:
5aa1b44 | Make three.js inclusion packager friendly
|
comment:4 Changed 3 years ago by
Ideally we would have a threejs config helper reporting its own version. That's the pipedream.
comment:5 follow-up: ↓ 27 Changed 3 years ago by
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 3 years ago by
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: ↓ 8 Changed 3 years ago by
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: ↓ 9 Changed 3 years ago by
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
andpackage-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 3 years ago by
Makes sense. These changes look good to me, if the doctests pass. Definite improvement, thank you!
comment:10 Changed 3 years ago by
- Commit changed from 5aa1b44d0f2ca87237bd7173f49350a5a13bce03 to 2ec0979ee4eedf068f9bbb2fd91b1a26f59d7dc6
Branch pushed to git repo; I updated commit sha1. New commits:
2ec0979 | Import missing modules for threejs changes
|
comment:11 Changed 3 years ago by
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
|
comment:12 Changed 2 years ago by
- Status changed from new to needs_review
comment:13 Changed 2 years ago by
gh-timokau, does this mean that we can set this to positive review now?
comment:14 Changed 2 years ago by
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 2 years ago by
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 2 years ago by
- Commit changed from 2ec0979ee4eedf068f9bbb2fd91b1a26f59d7dc6 to 1b27f44f52a36ac8d548f787c09cf61f5ec5aede
comment:17 Changed 2 years ago by
- 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 2 years ago by
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 2 years ago by
- Commit changed from 1b27f44f52a36ac8d548f787c09cf61f5ec5aede to 17fa3cc6a898091a6c7abb5a05e1a88a817495e1
Branch pushed to git repo; I updated commit sha1. New commits:
17fa3cc | Remove option to load three.js from CDN
|
comment:20 follow-up: ↓ 25 Changed 2 years ago by
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
|
comment:21 Changed 2 years ago by
And now remove functionality to load sensible scripts completely?!
comment:22 Changed 2 years ago by
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 2 years ago by
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 2 years ago by
- Work issues set to needs testing - but is the general approach ok?
comment:25 in reply to: ↑ 20 Changed 2 years ago by
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: ↓ 29 Changed 2 years ago by
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 2 years ago by
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 2 years ago by
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 2 years ago by
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 22 months ago by
- Commit changed from 17fa3cc6a898091a6c7abb5a05e1a88a817495e1 to be72dbe9745aae80e364912f0f9c9300de3b9a56
Branch pushed to git repo; I updated commit sha1. New commits:
be72dbe | Merge remote-tracking branch 'trac/develop' into u/saraedum/26434
|
comment:31 follow-up: ↓ 32 Changed 22 months ago by
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: ↓ 33 Changed 22 months ago by
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 22 months ago by
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 22 months ago by
- Cc egourgoulhon added
Adding Eric Gourgoulhon for input on sharing notebooks
comment:35 follow-up: ↓ 39 Changed 22 months ago by
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: ↓ 37 Changed 22 months ago by
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 22 months ago by
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 22 months ago by
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 22 months ago by
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: ↓ 42 ↓ 43 Changed 22 months ago by
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 22 months ago by
- Status changed from needs_review to needs_info
comment:42 in reply to: ↑ 40 Changed 22 months ago by
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: ↓ 44 ↓ 45 Changed 22 months ago by
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 22 months ago by
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 ofinstalled_packages()
and doesn't change anything else yet? Then see what other issues arise in practice. Although you'll want to useTHREEJSDIR
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 22 months ago by
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 22 months ago by
The intent of this ticket is addressed in #28086 so this one can be closed
comment:47 Changed 13 months ago by
- 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 10 months ago by
- Resolution set to duplicate
- Status changed from positive_review to closed
I CCed the usual packaging suspects; I have not tested this at all (Sage is rebuilding…)
New commits:
Make three.js inclusion packager friendly