sagemath / sage

Main repository of SageMath
https://www.sagemath.org
Other
1.42k stars 477 forks source link

Live documentation in Jupyter using Thebe #20690

Closed nthiery closed 8 years ago

nthiery commented 8 years ago

Thebe is a Jupyter javascript plugin for static sites that allows for rendering selected divs of the HTML as live cells connected to a Jupyter server:

https://oreillymedia.github.io/thebe/

The goal of this ticket is to use this technology to implement live documentation in the Jupyter notebook as we had in the legacy Sage notebook.

Kudos to Rob Beezer for pointing us to Thebe in his presentation of MathBookXML at Sage Days 77.

Steps:

TODO:

This ticket solves #17269. Follow-up ticket for possible improvements: #20893.

Depends on #21309

CC: @videlec @vbraun @rbeezer @slel @sagetrac-tmonteil

Component: documentation

Keywords: days77, jupyter, thebe, notebook, sd75

Author: Florent Cayré, Nicolas M. Thiéry

Branch: 8c8e731

Reviewer: Vincent Delecroix, Thierry Monteil

Issue created by migration from https://trac.sagemath.org/ticket/20690

nthiery commented 8 years ago
comment:35

Hi Vincent, Thierry, Besides merging the latest #21309 when it's polished, do you consider this as ready to go? It would be nice to be done with this! Thanks,

edd8e884-f507-429a-b577-5d554626c0fe commented 8 years ago

Changed branch from u/nthiery/thebe-20690 to u/tmonteil/thebe-20690

edd8e884-f507-429a-b577-5d554626c0fe commented 8 years ago

Changed commit from 4278753 to 11720b4

edd8e884-f507-429a-b577-5d554626c0fe commented 8 years ago
comment:37

I changed the symlink from local/share/thebejs/thebe.js to local/share/thebe/thebe.js according to the renaming done in #21309. Since the two branches were completely entangled (lot of merges), i went back to the last Florent's commit, fixed the symlink and added (and updated) Nicola's comment in thebe-sage.js header. Now the two branches are completely independant, apart from the very last merge.

If you plan to modify something in the current ticket or in #21309, please undo the last merge before, or it will start to knit again.


New commits:

e4768df#20690 : use thebe.js from the thebe package instead of the hardcoded one.
7199979#20690 : report updated version of Nicola's change in the disentangled branch.
520cdd0#21309 : thebejs -> thebe ; use hash in pkg name ; link to trac URL.
11720b4Merge branch 't/21309/package_thebe_js' into t/20690/thebe-20690
edd8e884-f507-429a-b577-5d554626c0fe commented 8 years ago
comment:38

Replying to @nthiery:

Hi Vincent, Thierry, Besides merging the latest #21309 when it's polished, do you consider this as ready to go? It would be nice to be done with this! Thanks,

I did not inspect thebe-sage.js as i would have done for Python files. But its behaviour corresponds to the claimed feature and the code seem not malicious. So i guess it will be OK for me once the dependencies will be satisfied.

nthiery commented 8 years ago
comment:39

Thanks Thierry for the final version of #21309 (Jeroen is double checking it right now) and for cleaning the history.

I agree that we are on a thin line here since we don't have strong review standards set for js files like this one. For example we are missing tests! However writing such tests would take some infrastructure; we should discuss how to setup such infrastructure with the Jupyter people at some point, but for now I guess that's ok.

Vincent, any further comment?

Cheers, Nicolas

edd8e884-f507-429a-b577-5d554626c0fe commented 8 years ago
comment:40

I have a doubt: to avoid race condition, i guess we should add thebe to the doc build dependencies.

7ed8c4ca-6d56-4ae9-953a-41e42b4ed313 commented 8 years ago

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

b8a7f32Merge branch 'develop' into t/20690/thebe-20690
22ae637#20690 : let the doc build depend on thebe.
7ed8c4ca-6d56-4ae9-953a-41e42b4ed313 commented 8 years ago

Changed commit from 11720b4 to 22ae637

7ed8c4ca-6d56-4ae9-953a-41e42b4ed313 commented 8 years ago

Changed commit from 22ae637 to 8c8e731

7ed8c4ca-6d56-4ae9-953a-41e42b4ed313 commented 8 years ago

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

7d4fe86#21309 : package thebejs.
28346f621309: added a brief line stating why Sage uses thebe
520cdd0#21309 : thebejs -> thebe ; use hash in pkg name ; link to trac URL.
8c8e731Merge branch 't/21309/package_thebe_js' into t/20690/thebe-20690
edd8e884-f507-429a-b577-5d554626c0fe commented 8 years ago
comment:43

Replying to @sagetrac-tmonteil:

I have a doubt: to avoid race condition, i guess we should add thebe to the doc build dependencies.

For this, i had to merge with sage.7.3 because the exact same line in build/make/deps changed there. Let us test the build like this (it might take some time). Again, if you plan to do some modifications, do not forget to undo the very last merge (with #21309) before.

nthiery commented 8 years ago
comment:44

Replying to @sagetrac-tmonteil:

Replying to @sagetrac-tmonteil:

I have a doubt: to avoid race condition, i guess we should add thebe to the doc build dependencies.

For this, i had to merge with sage.7.3 because the exact same line in build/make/deps changed there. Let us test the build like this (it might take some time).

Ok, thanks!

Again, if you plan to do some modifications, do not forget to undo the very last merge (with #21309) before.

We discussed this with Jeroen this morning. Unless Vincent has a specific requestion, we don't plan for further modifications.

videlec commented 8 years ago
comment:45

(good for me)

kiwifb commented 8 years ago
comment:47

I don't want to remove the positive review but I would like some info about /src/doc/common/themes/sage/static/thebe.js which reads

../../../../../../local/share/thebe/thebe.js

Would there be a better way to refer to the thebe.js file installed by the thebe package? The above assume a particular sage install. I don't usually let people get away with putting /local/ in anything these days as it is bad for stuff like sage-on-gentoo and more broadly sage-on-distro.

edd8e884-f507-429a-b577-5d554626c0fe commented 8 years ago
comment:48

I have no idea on how to do that. This kind of symlink already appears for mathjax, but the symlink is done when sagenb is installed. Unfortunately, there is no sage_documentation package where we could add a spkg-install script to install the symlink at build time. I you have an idea to improve this situation, it would be nice to implement it here and for mathjax as well.

kiwifb commented 8 years ago
comment:49

I have no solution right now. I will have to find a way to live with it. I'll come up with something, I usually do in the end. It doesn't always make sense in vanilla sage unfortunately.

vbraun commented 8 years ago

Changed branch from u/tmonteil/thebe-20690 to 8c8e731

jdemeyer commented 8 years ago

Changed commit from 8c8e731 to none

jdemeyer commented 8 years ago
comment:51

Adding a symbolic link from the sources to some installation directory really makes no sense, especially because it is hardcoded. Having it installed by the thebe package would at least make slightly more sense. Of course, ideally we would just use thebe.js from the installation directory.

jdemeyer commented 8 years ago
comment:52

See #21527 for a follow-up on the symbolic link issue.

edd8e884-f507-429a-b577-5d554626c0fe commented 8 years ago
comment:53

Here is some feedback after real-life testing. I am currently teaching for a 3-week Sage tutorial, and there is a workflow issue, somehow thebe.js miss the main point of the sagenb live documentation.

On sagenb notebook, say the students work on some thematic tutorial, they do some tests, copy some lecturer's examples and want to keep their changes. What they can do is to click on File (top left) and then Copy worksheet so that the live documentation becomes a true sagenb worksheet they can save to come back on it later.

In the current case, we can do some tests and modifications (which is great), but all the work is lost when we close the webpage. We should have a button to export the current state into a (possibly degradated, we do not really care about the subtle html structure) .ipynb file that can then be saved.

nthiery commented 8 years ago
comment:54

Thanks Thierry for the feedback!

Yes, this is a very desirable feature, which we had already listed on #20893:

  • [ ] Add support in Thebe for basic export to Jupyter notebooks. A quality loss (in particular in terms of the hierarchical structure) is acceptable.

Do you mind merging this item with the new one you added there?

jdemeyer commented 7 years ago
comment:55

While preparing the report for OpenDreamKit, I just noticed that this is completely broken: #22458