Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add ThebeLab support to notebook style galleries #713

Open
wants to merge 4 commits into
base: master
Choose a base branch
from

Conversation

sdhiscocks
Copy link
Contributor

@sdhiscocks sdhiscocks commented Jun 15, 2020

These initial changes allow for notebook style galleries to be run with Thebe Lab, allowing custom configuration for ThebeLab to be passed with details on repo, etc.

I wanted to get some initial feedback before creating documentation and tests.

TODO:

  • Documentation
  • Tests

@larsoner
Copy link
Contributor

Looks like a reasonable start to me, @choldgraf can you look / comment?

@GaelVaroquaux
Copy link
Contributor

GaelVaroquaux commented Jun 15, 2020 via email

@choldgraf
Copy link
Contributor

choldgraf commented Jun 15, 2020

Hey all - as I see we're thinking about thebelab functionality in sphinx-gallery, it makes me wonder if it would be useful if there were a separate sphinx extension that provided some base thebelab functionality, that projects like sphinx-gallery could build on. I just opened up an issue to brainstorm and discuss here: executablebooks/meta#68 would love input if any of you have thoughts on whether this would be useful!

(more specifically to this PR, I think it is a great idea, being able to spin up live kernels to sphinx-gallery docs would be great)

@codecov
Copy link

codecov bot commented Jun 15, 2020

Codecov Report

Merging #713 into master will decrease coverage by 0.24%.
The diff coverage is 57.14%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master     #713      +/-   ##
==========================================
- Coverage   97.52%   97.28%   -0.25%     
==========================================
  Files          32       32              
  Lines        3514     3531      +17     
==========================================
+ Hits         3427     3435       +8     
- Misses         87       96       +9     
Impacted Files Coverage Δ
sphinx_gallery/gen_gallery.py 93.70% <28.57%> (-1.17%) ⬇️
sphinx_gallery/gen_rst.py 96.78% <71.42%> (-0.81%) ⬇️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update a090463...50cbf80. Read the comment docs.

@sdhiscocks
Copy link
Contributor Author

Thanks @larsoner, @GaelVaroquaux and @choldgraf. So overall the implementation works well (very little changes, as Thebe Lab does all the hard work).

Would you prefer to leave this for now, waiting on proposal in executablebooks/meta#68, or would you like for me to add some tests and documentation for inclusion in sphinx-gallery as is?

@choldgraf
Copy link
Contributor

I don't think we should block this PR on a more general implementation (though I started playing around with this here if you're interested: https://github.com/executablebooks/sphinx-thebelab). I'm +1 on merging this once it's ready and then deciding whether we want to remove some code in lieu of sphinx-thebelab later on

@choldgraf
Copy link
Contributor

choldgraf commented Jun 18, 2020

Hey all - just a note that I made faster progress on this than I expected :-)

I put together a proof-of-concept repository that could give any Sphinx site Thebelab functionality, and that could also be easily built on top of in other themes, extensions, etc. Here's the repo:

https://github.com/executablebooks/sphinx-thebelab

My hope is that I can switch Jupyter Book to just using this extension, and maybe we can have one Sphinx extension for our Thebelab needs and all build on top of it. Here are some docs:

https://sphinx-thebelab.readthedocs.io/en/latest/

Again, still prototype-y, so would love feedback!

@sdhiscocks
Copy link
Contributor Author

I've updated this code to use sphinx-thebelab. Seems to work great @choldgraf.

Only issue I had was with the padding in code cells, but this may be to do with RTD theme. I've overridden this in the sphinx-gallery theme overrides for now.

I've also just added install of sphinx-thebelab direct from GitHub in the CI environments so at least it'll build. But note that as this repository doesn't have all packages needed in requirements.txt, so not all examples will work. I've added binder requirements file that'll work in future. For now, you can use for example !pip install matplotlib at top of the first cell.

@@ -24,7 +24,7 @@ jobs:
- restore_cache:
keys:
- cache-pip
- run: pip install --user numpy matplotlib pyqt5 seaborn sphinx_rtd_theme pillow joblib sphinx pytest vtk traits traitsui pyface mayavi memory_profiler ipython pandas
- run: pip install --user numpy matplotlib pyqt5 seaborn sphinx_rtd_theme pillow joblib sphinx pytest vtk traits traitsui pyface mayavi memory_profiler ipython pandas git+https://github.com/executablebooks/sphinx-thebelab#egg=sphinx_thebelab
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

note: if you're interested in using this for this PR, and if others are +1 on it, then I can make a pip release pretty easily

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

At this stage whilst PR is in draft, I just added this to get in to build. I'd hold off on this PR until sphinx-thebelab is ready, if that's the route that the project wants to go down.

@GaelVaroquaux
Copy link
Contributor

I frown at this PR: this additional external dependency in our build chain will make things more fragile.

Either there is a thebe integration with sphinx, and in this case it should live fully contained and configurable outside of sphinx-gallery, or there is a thebe integration inside sphinx-gallery. The proposed solution is in the middle and creates too much coupling.

@choldgraf
Copy link
Contributor

@GaelVaroquaux what I'm proposing is your first case. I just have a direct to github install url so people could try it out and tell me if it would be useful. I'm hoping to have a single package that can handle thebelab in sphinx instead of each package reinventing it

@choldgraf
Copy link
Contributor

I'm also happy with having this logic inside sphinx gallery until sphinx-thebelab matures enough

@GaelVaroquaux
Copy link
Contributor

GaelVaroquaux commented Jun 21, 2020 via email

@choldgraf
Copy link
Contributor

choldgraf commented Jun 21, 2020

So right now sphinx-thebelab is shaping up to require these pieces of config:

  1. Tell it what's the HTML selector it should use to "find" a code cell, its inputs, and its outputs (right now, it's .thebelab, .pre, and .output, respectively)
  2. Place a button somewhere on the page that activates Thebelab

I think that if we wanted to use sphinx-thebelab within this package, we'd need to do that configuration for the user (either configure sphinx-thebelab to find code cells on pages generated by sphinx gallery, or modify the rST generated by sphinx-gallery so that it outputs code cells that match the patterns that thebelab looks for). Then, add a button to each page. We could also always just tell users how to configure sphinx-thebelab on their own, but in that case they wouldn't get something like an "activate thebelab" button that is automatically added to each gallery page, which I think would make the connection not very useful.

If that's too much intermingling, then I think we should just implement thebelab functionality 💯 in sphinx-gallery.

@GaelVaroquaux
Copy link
Contributor

GaelVaroquaux commented Jun 21, 2020 via email

@choldgraf
Copy link
Contributor

Tbh I don't think that either approach would be too complex. For the button, it's just a matter of adding the "thebelab.bootstrap" function to a click event on some page element... Otherwise there's no way for a user to activate it. Sphinx-thebelab has a directive to quickly add this button, but I assume sphinx gallery would want its own styling etc for the button.

If we implement it ourselves, I think we'd basically need to copy the logic that is currently in sphinx-thebelab. That is basically these two files

https://github.com/executablebooks/sphinx-thebelab/blob/master/sphinx_thebelab/_static/thebelab.js

And

https://github.com/executablebooks/sphinx-thebelab/blob/master/sphinx_thebelab/__init__.py

(might be a little cleaner than those because we could make something that only works for sphinx gallery instead of something more generic)

This makes me wonder if we could automatically add a smaller edit button to each code cell (eg a "play" button symbol). Then we wouldn't need to add a button to each page and I feel like we could get away with just documenting how to use sphinx-thebelab without needing a full integration

(sorry my comments are not exactly honing down on a single solution here...)

@sdhiscocks
Copy link
Contributor Author

sdhiscocks commented Jun 21, 2020

So a pure sphinx-gallery working solution is in commits up to 1a30dfc (although I'd fix this up a little having seen approach in sphinx-thebelab and I've got to know sphinx-gallery code base a little better).

Either way, the code in required in sphinx-gallery is very small, as most the hard work is done in Thebe Lab javascript library.

I'd be happy to work with either approach.

@sdhiscocks sdhiscocks force-pushed the thebe_lab branch 2 times, most recently from 64cb030 to 3c37d12 Compare June 22, 2020 12:18
@sdhiscocks
Copy link
Contributor Author

I've updated this branch now so it includes changes that allow sphinx-gallery to use Thebe Lab without additional extensions. This modifies the RST output, so code blocks are wrapped in containers independent if Thebe Lab is enabled (as this also may be useful in other cases, like custom themes). This means the only change when enabling Thebe Lab is addition of button and configuration values/javascript.

This is also written so that use of sphinx-thebelab could also be used very easily. The following changes would be required: changing the function the button calls; and moving configuration to sphinx-thebelab configuration (auto populating correct configuration parameters).

diff --git a/doc/conf.py b/doc/conf.py
index 3bc440f..1cbe588 100644
--- a/doc/conf.py
+++ b/doc/conf.py
@@ -44,6 +44,7 @@ extensions = [
     'sphinx.ext.todo',
     'sphinx.ext.coverage',
     'sphinx.ext.mathjax',
+    'sphinx_thebelab',
     'sphinx_gallery.gen_gallery',
 ]
 
@@ -361,12 +362,10 @@ sphinx_gallery_conf = {
     # each code block
     'capture_repr': ('_repr_html_', '__repr__'),
     'matplotlib_animations': True,
-    'thebelab': {
-        "requestKernel": True,
-        "binderOptions": {
-            "repo": "sphinx-gallery/sphinx-gallery.github.io",
-        },
-    },
+}
+
+thebelab_config = {
+    'repository_url': 'https://github.com/sphinx-gallery/sphinx-gallery.github.io',
 }
 
 # Remove matplotlib agg warnings from generated doc when using plt.show
diff --git a/sphinx_gallery/gen_gallery.py b/sphinx_gallery/gen_gallery.py
index 986bc2a..10faa67 100644
--- a/sphinx_gallery/gen_gallery.py
+++ b/sphinx_gallery/gen_gallery.py
@@ -319,17 +319,14 @@ def _complete_gallery_conf(sphinx_gallery_conf, src_dir, plot_gallery,
                               % (css, _KNOWN_CSS))
         if gallery_conf['app'] is not None:  # can be None in testing
             gallery_conf['app'].add_css_file(css + '.css')
-    if gallery_conf['thebelab'] is not None:
-        gallery_conf['thebelab']['selector'] = \
-            ".sphx-glr-code>:not(.sphx-glr-output)"
-        gallery_conf['thebelab']['outputSelector'] = ".sphx-glr-output"
-        gallery_conf['thebelab']['predefinedOutput'] = True
-        gallery_conf['app'].add_js_file(
-            None,
-            body=json.dumps(gallery_conf['thebelab']),
-            type="text/x-thebe-config")
-        gallery_conf['app'].add_js_file(
-            "https://unpkg.com/thebelab@latest/lib/index.js")
+    if gallery_conf['thebelab'] is None and app is not None:
+        gallery_conf['thebelab'] = 'sphinx_thebelab' in app.config.extensions
+        if gallery_conf['thebelab']:
+            conf = app.config.html_context
+            conf['thebelab_selector'] = '.sphx-glr-code'
+            conf['thebelab_selector_code'] = 'div:not(.sphx-glr-output)'
+            conf['thebelab_selector_output'] = '.sphx-glr-output'
+
 
     return gallery_conf
 
diff --git a/sphinx_gallery/gen_rst.py b/sphinx_gallery/gen_rst.py
index 1a1e62c..d451515 100644
--- a/sphinx_gallery/gen_rst.py
+++ b/sphinx_gallery/gen_rst.py
@@ -179,7 +179,7 @@ thebelab_badge = """\n
 
     <script>
       $('.sphx-glr-thebelab-badge a').click(
-        function(){{thebelab.bootstrap(); return false;}}
+        function(){{initThebelab(); return false;}}
       );
     </script>
 """

Copy link
Contributor

@choldgraf choldgraf left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

A few quick comments in there, I think it looks reasonable right now, and agree it would be easy to shuffle this off onto sphinx-thebelab in the future if we ever wanted to shirk testing, maintenance, and feature improvement duties on this :-)

@@ -317,6 +319,17 @@ def call_memory(func):
% (css, _KNOWN_CSS))
if gallery_conf['app'] is not None: # can be None in testing
gallery_conf['app'].add_css_file(css + '.css')
if gallery_conf['thebelab'] is not None:
gallery_conf['thebelab']['selector'] = \
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

here we should probably use get methods, and/or explicitly check for some keys and raise helpful errors if they are required for thebelab to work

gallery_conf['app'].add_js_file(
None,
body=json.dumps(gallery_conf['thebelab']),
type="text/x-thebe-config")
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

this is a reasonable way to do the configuration for now, it may get trickier if there is a future possibility people will want per-page configuration of Thebelab (e.g. if two different kernels are used in the same gallery). In that case we'd need to add the Thebelab configuration at page build time

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

ah actually I just realized we might need to use custom thebelab config on each page, because we'll need to set the kernel path to point to whatever sub-directory the gallery page is in. Otherwise Thebelab will initialize the kernel in the root of the docs, which may break relative paths.

.. raw:: html

<script>
$('.sphx-glr-thebelab-badge a').click(
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I can't remember - does sphinx-gallery assume jquery is installed?

example_rst += code_output
example_rst += indent(code_rst, ' '*4)
if code_output.strip():
example_rst += " .. container:: sphx-glr-output\n"
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

+1 to wrapping the cell / inputs / outputs in their own containers so they can be more explicitly tagged

if is_example_notebook_like:
example_rst += code_rst
example_rst += code_output
example_rst += indent(code_rst, ' '*4)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

this is minor, but instead of doing ' '* 8, I like to define a tab variable and then use multiples of that. e.g.

tab = '    '
indent(code_rst, tab*2)

Reduces one cognitive step of dividing the number by the number of spaces per tab :-)

These initial changes allow for notebook style galleries to be run with
ThebeLab, allow custom configuration for ThebeLab to be passed with
details on repo, etc.

Documentation and tests still required.
Also, configuration is per page, using rst location as path currently.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants