Opened 4 years ago

Closed 4 years ago

Last modified 2 years ago

#22670 closed defect (fixed)

Make Three.js work offline redux

Reported by: paulmasson Owned by:
Priority: major Milestone: sage-8.0
Component: graphics Keywords:
Cc: novoselt, egourgoulhon Merged in:
Authors: Paul Masson, Andrey Novoseltsev Reviewers: Andrey Novoseltsev, Paul Masson
Report Upstream: N/A Work issues:
Branch: 668eeee (Commits) Commit:
Dependencies: Stopgaps:

Description (last modified by paulmasson)

The Three.js scripts used by various backends for offline viewing should be generated by methods in the backends, not in plot3d/base.pyx.

Continuation of discussion after #22488 was closed.

Change History (41)

comment:1 Changed 4 years ago by paulmasson

  • Cc novoselt egourgoulhon added
  • Description modified (diff)
  • Type changed from PLEASE CHANGE to defect

comment:2 Changed 4 years ago by novoselt

Some more thoughts: we should avoid code duplication as much as feasible here so that we don't have to chase all places where the version of the threejs is used when we want to change it. Perhaps something like this will work:

  • generic method returns CDN scripts with online=True and throws an exception with online=False, these scripts include the version number
  • backend-specific implementation provide appropriate paths to Sage package (which does not include the version number), and if they have to include a fall-back CDN section, they do it by invoking the parent method with online=True and embed that string in appropriate places

This way when upgrading threejs package one has also to bump version number in the generic method (and perhaps it should be mentioned in SPKG.txt), but backend ones will likely never require tweaking.

comment:3 Changed 4 years ago by paulmasson

  • Branch set to u/paulmasson/22670

comment:4 Changed 4 years ago by paulmasson

  • Commit set to 1d27bd099b05d720531e0882d52c67ad198b6807
  • Status changed from new to needs_review

Andrey, please have a look at this. I'm using the internal installed_packages table to read the version number where needed, so we'll only have to change it once in the package itself during upgrades. I didn't include an error test to make sure the package is installed since it's now standard and should always be present. The IPython notebook can't call the parent method as you'd like because that fallback is in the JavaScript, so it needs to look up the version as well, but then there will be no tweaking for upgrades.

There is currently a failing doctest for something called "BackendDoctest". I'll fix that once I know we're on the right track.


New commits:

1d27bd0Add methods for offline scripts

comment:5 Changed 4 years ago by novoselt

Automatic version matching is great, thanks for figuring it out!

It still seems to me that in base.pyx it would be better to have just

scripts = backend.threejs_scripts(options['online'])

with the base backend class handling True case and more specific classes worrying about False. That way BackendIPythonNotebook could call parent method for extra bits instead of repeating the logic. Also, everything would be more in one place rather than the same thing split between plot and repl. But overall this design is already a nice improvement over current version.

comment:6 Changed 4 years ago by novoselt

  • Reviewers set to Andrey Novoseltsev

comment:7 Changed 4 years ago by git

  • Commit changed from 1d27bd099b05d720531e0882d52c67ad198b6807 to 5c1c232846797d6fcfae891eef85019e4f532a02

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

5c1c232Move all script generation to backend

comment:8 Changed 4 years ago by paulmasson

Hopefully this is closer to what you had in mind.

I've included an error test in the base method, but this causes a doctest in plot3d/base.pyx to fail even with the keyword argument set explicitly. Do you know why the doctesting process ignores the keyword argument?

A workaround is to have the base method return an empty string when online=False but that's hardly desirable.

comment:9 Changed 4 years ago by novoselt

  • Branch changed from u/paulmasson/22670 to u/novoselt/22670

comment:10 Changed 4 years ago by novoselt

  • Commit changed from 5c1c232846797d6fcfae891eef85019e4f532a02 to 6a4bdecfd82edc43fb3f024963378f069990a33d
  • Status changed from needs_review to needs_work
  • Work issues set to option handling

Tweaked a bit backend code.

Regarding option ignoring - that's because you ignore them instead of doing opts = self._process_viewing_options(kwds) as other methods ;-) I'm looking at how things work and hope to post a solution shortly, will be useful for my understanding of option handling.


New commits:

6a4bdecUse a single copy of CDN threejs scripts

comment:11 Changed 4 years ago by git

  • Commit changed from 6a4bdecfd82edc43fb3f024963378f069990a33d to b73ce25630b98d1b6128d940c2bf6f970ad4a334

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

b73ce25Pick up constructor options for threejs rendering

comment:12 Changed 4 years ago by novoselt

Improved option handing, but now the error is

Failed example:
    sphere(online=True)._rich_repr_threejs()
Exception raised:
    Traceback (most recent call last):
      File "/home/novoselt/sage/local/lib/python2.7/site-packages/sage/doctest/forker.py", line 498, in _run
        self.compile_and_execute(example, compiler, test.globs)
      File "/home/novoselt/sage/local/lib/python2.7/site-packages/sage/doctest/forker.py", line 861, 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 424, in sage.plot.plot3d.base.Graphics3d._rich_repr_threejs (build/cythonized/sage/plot/plot3d/base.c:10615)
        html = html.replace('SAGE_OPTIONS', json.dumps(options))
      File "/home/novoselt/sage/local/lib/python/json/__init__.py", line 244, in dumps
        return _default_encoder.encode(obj)
      File "/home/novoselt/sage/local/lib/python/json/encoder.py", line 207, in encode
        chunks = self.iterencode(o, _one_shot=True)
      File "/home/novoselt/sage/local/lib/python/json/encoder.py", line 270, in iterencode
        return _iterencode(o, 0)
      File "/home/novoselt/sage/local/lib/python/json/encoder.py", line 184, in default
        raise TypeError(repr(o) + " is not JSON serializable")
    TypeError: Texture(texture42, 6666ff) is not JSON serializable

which makes me wonder why these options are sent to the output at all?..

comment:13 Changed 4 years ago by novoselt

OK, looking at the template I see how they are used, but I am not sure what's the correct way to proceed: fix representation of texture option, drop if from threejs output, or send only options which are known to be used in JavaScript?

comment:14 Changed 4 years ago by paulmasson

I don't see how invoking _process_viewing_options(kwds) fixes anything: all it appears to do is add a bunch of options that won't be used in JavaScript, at least not yet. Where is online actually getting passed on to the doctest backend with your changes?

All the other changes look good. And FYI decimals is Three.js specific.

comment:15 follow-up: Changed 4 years ago by paulmasson

FYI the options explicitly set by me are the only ones used in the template, which is why I just created my own dictionary.

comment:16 Changed 4 years ago by novoselt

online=True passed to the constructor is saved in the object, _process_viewing_options assembles default options, options passed to constructor, and options passed directly in kwds, it also does some normalization of them, so I am pretty sure it is "the right thing" to call it here as well. I've tried to mark decimals as Three.js specific - they are set there to a default if not passed via other means and then another section is dealing with type conversion.

comment:17 in reply to: ↑ 15 ; follow-up: Changed 4 years ago by novoselt

Replying to paulmasson:

FYI the options explicitly set by me are the only ones used in the template, which is why I just created my own dictionary.

But you are using some of the options that are "standard" like axes and the problem was that it was not possible to pass threejs-specific options via standard means. So I am still convinced that my changes are on the right track. And it seems to me now that the next step is to keep only whitelisted options in this dictionary before sending them to template.

comment:18 in reply to: ↑ 17 Changed 4 years ago by paulmasson

Replying to novoselt:

Replying to paulmasson:

FYI the options explicitly set by me are the only ones used in the template, which is why I just created my own dictionary.

But you are using some of the options that are "standard" like axes and the problem was that it was not possible to pass threejs-specific options via standard means. So I am still convinced that my changes are on the right track. And it seems to me now that the next step is to keep only whitelisted options in this dictionary before sending them to template.

If it gets things to work then good, but I'm not yet seeing the standard means. Sorry about the decimals mix up: was editing my statement when you responded. And one correction: online isn't used in the template so doesn't need to be whitelisted.

comment:19 Changed 4 years ago by git

  • Commit changed from b73ce25630b98d1b6128d940c2bf6f970ad4a334 to f5144256fd873eb051f174e4b86daf7abef1d4a5

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

f514425Do not embedd all Sage options into a threejs scene

comment:20 Changed 4 years ago by novoselt

By "standard means" I meant that this is how other similar functions process options: of the main interest to us is that it picks up options passed to constructor, but there is some extra logic to e.g. handle aspect_ratio. After all, in the ideal world there would be no backend/viewer specific options, e.g. why can't other viewers support axes_labels?

Anyway, the last commit lets tests pass, but let me fiddle a little more with backends.

comment:21 Changed 4 years ago by git

  • Commit changed from f5144256fd873eb051f174e4b86daf7abef1d4a5 to e81038e11c2a0d8fbf289d65cf31c64f71862677

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

e81038eDo not access display backend directly from plot 3d

comment:22 Changed 4 years ago by fbissey

The use of installed_packages looks nifty in theory. It automates things nicely. The only problem with it, is that it depends on you using sagemath's packaging system in full. sage-on-distro or if we switch (cross fingers) to a more effective packaging solution this will just be in the way.

As a sage-on-distro person I disapprove of anything using sage.misc.package at runtime.

I don't have a better solution at this stage. Are different release on the CDN all that different? Do they have some backward or forward compatibility?


New commits:

e81038eDo not access display backend directly from plot 3d

comment:23 follow-up: Changed 4 years ago by novoselt

  • Status changed from needs_work to needs_review
  • Work issues option handling deleted

OK, how about this one? It still bugged me that _backend of display manager was accessed directly, so I moved the CDN method to DisplayManager and made backends responsible for offline case only as in your original version. It is somewhat ugly that they call that CDN method when handling offline case, but it is kind of due to things not done completely properly. Why SageNB is using CDN for offline case? Doesn't it have access to Sage installation which includes three.js?

comment:24 follow-up: Changed 4 years ago by novoselt

Regarding CDN releases - in principle the version matters, although the problems are presumably more frequent when using old instead of new. So we may try to omit the version and use the master branch of threejs.

comment:25 in reply to: ↑ 24 Changed 4 years ago by paulmasson

Replying to novoselt:

Regarding CDN releases - in principle the version matters, although the problems are presumably more frequent when using old instead of new. So we may try to omit the version and use the master branch of threejs.

With Three.js the version number can matter greatly, unfortunately. Mr. Doob has made radical changes in the past that render the code not backward compatible. That doesn't happen too often but it will be an ongoing concern.

comment:26 Changed 4 years ago by novoselt

OK, possible solutions:

  1. Have hard-coded version number and add some comment to SPKG.txt as I originally proposed.
  2. Pick up the version number from the library file itself, presumably this is possible.
  3. Put an extra file together with the library into share/threejs
  4. =2+3: Pick up the version number from the library file and then write it into some file for speed. Or just some variable to avoid disk operations and access issues...

comment:27 follow-up: Changed 4 years ago by fbissey

Options 2,3 and 4 rely on theejs to be installed locally, a distro may very decide to make that optional (if at all - properly packaging this kind of stuff is a pain) and to rely on online availability by default.

Alternative to (1) declare a variable in sage.env.py it could be migrated to a separate file installed by the spkg once we work out #22652. That method would be ok to me as I can supply an arbitrary extra file for my distro.

comment:28 in reply to: ↑ 23 ; follow-up: Changed 4 years ago by paulmasson

Replying to novoselt:

It still bugged me that _backend of display manager was accessed directly

Why is this a bad thing to do?

It is somewhat ugly that they call that CDN method when handling offline case, but it is kind of due to things not done completely properly.

Only the IPython notebook does this, to provide a fallback requested by Eric. How would you do this more properly?

Why SageNB is using CDN for offline case? Doesn't it have access to Sage installation which includes three.js?

The server directory to Three.js files doesn't exist in the current version of the notebook. A pull request was merged to provide that and the method will be altered when a newer version of the notebook is available.

Couple minor things:

1) In display manager documentation, "Three.js" should be capitalized and "offline" is one word.

2) I may want to reuse the options dictionary for handling future features, like setting iframe size from figsize, so it shouldn't be completely overwritten. Please introduce a different name on the next commit.

comment:29 Changed 4 years ago by paulmasson

Andrey, scenes render for all three backends and both on/offline, but you've introduced a problem with IPython offline. The closing script tags need to be written as <\/script> with the forward slash escaped to prevent the remainer of that line of code, the ');, from printing in the cell above the scene. Please do a string replace before returning.

comment:30 in reply to: ↑ 27 Changed 4 years ago by novoselt

Replying to fbissey:

Options 2,3 and 4 rely on theejs to be installed locally, a distro may very decide to make that optional (if at all - properly packaging this kind of stuff is a pain) and to rely on online availability by default.

Alternative to (1) declare a variable in sage.env.py it could be migrated to a separate file installed by the spkg once we work out #22652. That method would be ok to me as I can supply an arbitrary extra file for my distro.

Our three.js package contrains only two files that we use instead of the whole repository, so it does not seem a big deal to me to keep it installed as part of Sage. In any case I think that for now (i.e. this ticket) we should keep it as is and modify to something else once (and if) it becomes a problem.

comment:31 follow-up: Changed 4 years ago by fbissey

For sage on its own, in its corner playing all alone, by itself, there is no problem. There is never a problem, you are self contained. I don't suggest you change the status standard package for threejs either. I am encouraging you to think of a bigger picture regarding package management when you are not on your little island.

It will be a problem for anyone doing sage-on-distro/conda as soon as it is merged. I probably should publicise this ticket on the sage-packaging mailing list and see what everyone think of the threejs business as it is now, since the current situation is hardly ideal either.

comment:32 Changed 4 years ago by git

  • Commit changed from e81038e11c2a0d8fbf289d65cf31c64f71862677 to 8d1b1555d032b999e2f779bd481544e2ec03d483

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

8d1b155Little fixes to threejs scripts handling

comment:33 in reply to: ↑ 28 Changed 4 years ago by novoselt

Replying to paulmasson:

Replying to novoselt:

It still bugged me that _backend of display manager was accessed directly

Why is this a bad thing to do?

Because backends are supposed to be isolated! It was really, really painful to have a lot of places spread over Sage that were doing different things depending on the way it was launched and I would prefer none of them coming back ;-)

It is somewhat ugly that they call that CDN method when handling offline case, but it is kind of due to things not done completely properly.

Only the IPython notebook does this, to provide a fallback requested by Eric. How would you do this more properly?

No how, it is just a bit unfortunate that this problem arises in the first place.

Why SageNB is using CDN for offline case? Doesn't it have access to Sage installation which includes three.js?

The server directory to Three.js files doesn't exist in the current version of the notebook. A pull request was merged to provide that and the method will be altered when a newer version of the notebook is available.

Great!

Couple minor things:

1) In display manager documentation, "Three.js" should be capitalized and "offline" is one word.

Fixed.

2) I may want to reuse the options dictionary for handling future features, like setting iframe size from figsize, so it shouldn't be completely overwritten. Please introduce a different name on the next commit.

That's why I put this code right next to template filling, but different name works just as well, done.

comment:34 in reply to: ↑ 31 ; follow-up: Changed 4 years ago by novoselt

Replying to fbissey:

For sage on its own, in its corner playing all alone, by itself, there is no problem. There is never a problem, you are self contained. I don't suggest you change the status standard package for threejs either. I am encouraging you to think of a bigger picture regarding package management when you are not on your little island.

It will be a problem for anyone doing sage-on-distro/conda as soon as it is merged. I probably should publicise this ticket on the sage-packaging mailing list and see what everyone think of the threejs business as it is now, since the current situation is hardly ideal either.

I am not trying to stay put on my island and I am all for isolation etc. But if use of sage.misc.package is prohibited, then perhaps it should not be exposed and look like an accessible functionality. How should we know that you disapprove of it? Please improve the current non-ideal situation, but if depends on a new ticket I would like to merge this one for the moment since it does improve threejs situation and we hope to make it default viewer in 8.0.

comment:35 in reply to: ↑ 34 Changed 4 years ago by fbissey

Replying to novoselt:

Replying to fbissey:

For sage on its own, in its corner playing all alone, by itself, there is no problem. There is never a problem, you are self contained. I don't suggest you change the status standard package for threejs either. I am encouraging you to think of a bigger picture regarding package management when you are not on your little island.

It will be a problem for anyone doing sage-on-distro/conda as soon as it is merged. I probably should publicise this ticket on the sage-packaging mailing list and see what everyone think of the threejs business as it is now, since the current situation is hardly ideal either.

I am not trying to stay put on my island and I am all for isolation etc. But if use of sage.misc.package is prohibited, then perhaps it should not be exposed and look like an accessible functionality. How should we know that you disapprove of it? Please improve the current non-ideal situation, but if depends on a new ticket I would like to merge this one for the moment since it does improve threejs situation and we hope to make it default viewer in 8.0.

Yes, you are right there should be a higher level policy on this. sage.misc.package is an artefact of sage being its own packaging system and it has unwelcome tentacles all over the place. I am trying to convince people not create any more tentacles - like I killed most uses of SAGE_ROOT before. I shall bring it on sage-devel.

comment:36 Changed 4 years ago by paulmasson

Documentation won't build. Looks like some extra whitespace somewhere:

[repl     ] /Users/Masson/Downloads/GitHub/sage/local/lib/python2.7/site-packages/sage/repl/rich_output/backend_ipython.py:docstring of sage.repl.rich_output.backend_ipython.BackendIPythonNotebook.threejs_offline_scripts:13: WARNING: Block quote ends without a blank line; unexpected unindent.
[repl     ] /Users/Masson/Downloads/GitHub/sage/local/lib/python2.7/site-packages/sage/repl/rich_output/display_manager.py:docstring of sage.repl.rich_output.display_manager.DisplayManager.threejs_scripts:22: WARNING: Block quote ends without a blank line; unexpected unindent.
Error building the documentation.

comment:37 Changed 4 years ago by paulmasson

  • Branch changed from u/novoselt/22670 to u/paulmasson/22670

comment:38 Changed 4 years ago by paulmasson

  • Authors changed from Paul Masson to Paul Masson, Andrey Novoseltsev
  • Commit changed from 8d1b1555d032b999e2f779bd481544e2ec03d483 to 668eeee7f0f03c5c2ccfb44f33925fabaf28a63c
  • Reviewers changed from Andrey Novoseltsev to Andrey Novoseltsev, Paul Masson
  • Status changed from needs_review to positive_review

Figured out it was the \n in two doctests. Replaced with ... and documentation now builds and doctests still pass.

Setting to positive review unless someone else wants to test it further.


New commits:

668eeeeAllow documentation to build

comment:39 Changed 4 years ago by tscrim

I suspect it might have been fixed if you made the string raw, i.e., use r""".

comment:40 Changed 4 years ago by vbraun

  • Branch changed from u/paulmasson/22670 to 668eeee7f0f03c5c2ccfb44f33925fabaf28a63c
  • Resolution set to fixed
  • Status changed from positive_review to closed

comment:41 Changed 2 years ago by jdemeyer

  • Commit 668eeee7f0f03c5c2ccfb44f33925fabaf28a63c deleted

See #25665 for a follow-up

Note: See TracTickets for help on using tickets.