Opened 2 years ago

Closed 2 years ago

#25665 closed defect (fixed)

Don't use installed_packages() for threejs URL

Reported by: egourgoulhon Owned by:
Priority: blocker Milestone: sage-8.3
Component: graphics Keywords: threejs
Cc: embray, jdemeyer, arojas, fbissey Merged in:
Authors: Frédéric Chapoton Reviewers: Eric Gourgoulhon
Report Upstream: N/A Work issues:
Branch: 867e1ff (Commits) Commit: 867e1ffe5b15a3827d2828ad57ac23f392f7f4ee
Dependencies: Stopgaps:

Description (last modified by egourgoulhon)

Since Sage 8.3.beta3, running the command

show(sphere(), viewer='threejs', online=True)

in a Jupyter notebook displays nothing. It was fine in Sage <= 8.3.beta2. Note that the option online=True is required by tools like http://nbviewer.jupyter.org/ and CoCalc.

It seems that the issue is due to the following change introduced in #25040 (which has been merged in Sage 8.3.beta3):

sage: installed_packages()['threejs']
'r80.p0'

while in Sage <= 8.3.beta2, one has

sage: installed_packages()['threejs']
'r80'

Indeed the value of installed_packages()['threejs'] is used to form the url in lines 749-750 of src/sage/repl/rich_output/display_manager.py

Change History (13)

comment:1 Changed 2 years ago by egourgoulhon

  • Cc embray jdemeyer added

comment:2 in reply to: ↑ description ; follow-up: Changed 2 years ago by jdemeyer

Replying to egourgoulhon:

Indeed the value of installed_packages()['threejs'] is used to form the url in lines 749-750 of src/sage/repl/rich_output/display_manager.py

That's a really bad idea for multiple reasons.

comment:3 Changed 2 years ago by jdemeyer

  • Description modified (diff)
  • Summary changed from threejs viewer broken after Sage 8.3.beta3 to Don't use installed_packages() for threejs URL

comment:4 Changed 2 years ago by egourgoulhon

  • Description modified (diff)

comment:5 in reply to: ↑ 2 Changed 2 years ago by egourgoulhon

Replying to jdemeyer:

Replying to egourgoulhon:

Indeed the value of installed_packages()['threejs'] is used to form the url in lines 749-750 of src/sage/repl/rich_output/display_manager.py

That's a really bad idea for multiple reasons.

Yes probably. I guess that was to ensure compatibility between the version of threejs actually used by Sage. By the way, in the directory upstream, we have threejs-r80.tar.gz, so where does the .p0 come from?

Last edited 2 years ago by egourgoulhon (previous) (diff)

comment:6 Changed 2 years ago by gh-antonio-rojas

  • Cc gh-antonio-rojas added

comment:7 Changed 2 years ago by novoselt

That was indeed to ensure that the same version is used. If there is a better way - please implement it! And an obvious way to get rid of patched suffixes for this particular case is to just use the part of the version before the dot.

comment:8 Changed 2 years ago by fbissey

  • Cc fbissey added

comment:9 Changed 2 years ago by arojas

  • Cc arojas added; gh-antonio-rojas removed

comment:10 Changed 2 years ago by chapoton

  • Authors set to Frédéric Chapoton
  • Branch set to u/chapoton/25665
  • Commit set to 867e1ffe5b15a3827d2828ad57ac23f392f7f4ee
  • Status changed from new to needs_review

New commits:

867e1fftrac 25665 fixing threejs URL

comment:11 Changed 2 years ago by fbissey

The use of is installed_packages at runtime in sage is of course one of my pet hates. I don't have an alternative to offer at the present time. In fact I am just realising how this bit is broken in sage-on-gentoo right now (I don't offer an offline option either). Where could I find a list of valid versions for the online cdn? Is there something like "latest"?

comment:12 Changed 2 years ago by egourgoulhon

  • Reviewers set to Eric Gourgoulhon
  • Status changed from needs_review to positive_review

Thanks for the fix!

comment:13 Changed 2 years ago by vbraun

  • Branch changed from u/chapoton/25665 to 867e1ffe5b15a3827d2828ad57ac23f392f7f4ee
  • Resolution set to fixed
  • Status changed from positive_review to closed
Note: See TracTickets for help on using tickets.