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 try_examples directive for adding interactivity to sphinx Examples sections #111

Merged
merged 26 commits into from
Nov 7, 2023

Conversation

steppi
Copy link
Collaborator

@steppi steppi commented Oct 20, 2023

Reference issue

closes gh-105.

What does this implement/fix?

This PR adds the try_examples directive, which takes Examples sections like below

image

and generates notebooks based on them, providing a button to swap the example content in place with a notebook

image

Additional information

You can find this deployed for the SciPy documentation here, https://steppi.github.io/scipy/. I've managed to fix a lot of small bugs in notebook generation and this has finally got to the state where I haven't found any issues on that front in the SciPy docs.

For content under the .. try_examples:: directive, doctest blocks are turned into code cells and RST text blocks are turned into markdown cells. Both inline and block math is handled so that the math displays correctly in the notebooks, as seen above.

To avoid having to manually add the directive, I've created a global_enable_try_examples config option, which automatically inserts the directive within Examples sections during the autodoc-process-docstring phase. This requires the docstring to be in the format produced by numpydoc during this phase ( I think this is format is also produced by sphinx.ext.napoleon.) It will need to be clearly documented how to set things up so that global_enable_try_examples will work. At the moment I've only tested this with SciPy's documentation, which has NumPy style docstrings and used the following extensions

extensions = [
    'sphinx.ext.autodoc',
    'sphinx.ext.autosummary',
    'sphinx.ext.coverage',
    'sphinx.ext.mathjax',
    'sphinx.ext.intersphinx',
    'numpydoc',
    'sphinx_design',
    'scipyoptdoc',
    'doi_role',
    'matplotlib.sphinxext.plot_directive',
    'myst_nb',
    'jupyterlite_sphinx',
]

I've added handling for math with sphinx.ext.mathjax, worked around cruft added by matplotlib.sphinxext.plot_directive that shouldn't go into notebooks, and cleaned up the raw output for references produced by numpydoc so it looks nicer in the notebooks. Other extensions and workflows may cause issues, particularly if using global_enable_try_examples. Using the directive directly on a vanilla docstring should work fine.

For, now I've set the heights of the iframes for the generated notebooks to be the same as the heights of the rendered Examples content. This works pretty well in most cases, but looks bad for particularly short Examples sections. I plan to make this more configurable, but would appreciate feedback on the best way to do that. For now there are configuration options for the whether or not the toolbar should appear by default, the jupyterlite theme, and the css for relevant buttons. I think configuration should be more flexible.

I haven't documented the extension yet, but clear documentation will be very important.

@martinRenou I have one important question. At least for Firefox, I've found that my browser prompts me to select a kernel every time I open one of the generated notebooks, even though Pyodide is the only kernel installed. You can try that out here. I think this will be too annoying for users, so hope there is a way the kernel can be specified in advance.

@jtpio jtpio added the enhancement New feature or request label Oct 25, 2023
@jtpio
Copy link
Member

jtpio commented Oct 25, 2023

Thanks @steppi!

At least for Firefox, I've found that my browser prompts me to select a kernel every time I open one of the generated notebooks, even though Pyodide is the only kernel installed. You can try that out here. I think this will be too annoying for users, so hope there is a way the kernel can be specified in advance.

Agree the popup can be quite annoying. Maybe it's an issue in JupyterLite (or JupyterLab), which is not able to pick the default (and only) kernel when available.

Looking at one of the example notebooks: https://raw.githubusercontent.com/steppi/scipy/gh-pages/lite/files/004e9741_32fd_4b41_9716_052cde466244.ipynb, it looks like the metadata field of the notebook is empty. But adding python or the Pyodide kernel to the notebook metadata may also help JupyterLite pick the correct kernel on startup.

@steppi
Copy link
Collaborator Author

steppi commented Oct 26, 2023

Thanks so much @jtpio! Adding python to the notebook metadata worked.

@steppi
Copy link
Collaborator Author

steppi commented Oct 26, 2023

https://steppi.github.io/scipy/ has been updated with the fix and now doesn't have the annoying pop up to select the kernel.

One piece of feedback I've received recently is that the button to try the examples should be at the top of the Examples section to make it more noticeable.

I noticed that when pressing the try examples button, you just get a blank white iframe for a second or two until the notebook loads. I think I need to add some kind of indication that the notebook is loading, like you do for the existing directives. I'll also work on adding documentation and more configurability.

@martinRenou
Copy link
Member

That looks great! Thanks!

I'll also work on adding documentation and more configurability

Would you like to get a review already or should we wait for another iteration from your side?

@steppi
Copy link
Collaborator Author

steppi commented Nov 6, 2023

That looks great! Thanks!

I'll also work on adding documentation and more configurability

Would you like to get a review already or should we wait for another iteration from your side?

Thanks! It would be great if you could give it a first pass, even if just a quick one, just to help save me from putting in extra work finishing things up along a path that shouldn’t have been taken, if it turns out something isn’t quite right.

@martinRenou
Copy link
Member

You can find this deployed for the SciPy documentation here, https://steppi.github.io/scipy/

Following the link your provided, I cannot test this feature. Do you have a link to a particular page in the documentation where I can try it?

This works pretty well in most cases, but looks bad for particularly short Examples sections

Maybe an harcoded (or configurable) minimum size for the IFrame could improve this?

@steppi
Copy link
Collaborator Author

steppi commented Nov 7, 2023

Following the link your provided, I cannot test this feature. Do you have a link to a particular page in the documentation where I can try it?

Sorry, here's an example page for the function dblquad. If you want to see more, these interactive docstrings are in the API reference section of the docs.

Maybe an harcoded (or configurable) minimum size for the IFrame could improve this?

Yeah, I think that's a good idea. I think we can make it configurable with a sane default.

@martinRenou
Copy link
Member

Sorry, here's an example page for the function dblquad. If you want to see more, these interactive docstrings are in the API reference section of the docs.

Neat 👍🏽

Copy link
Member

@martinRenou martinRenou left a comment

Choose a reason for hiding this comment

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

This all look sensible to me.

I'm just afraid that manually parsing the rst like this would not scale very well and may be difficult to maintain.
I wonder if we can use docutils's parsers or write our own parser inheriting from the base ones.
Anyway, I don't think this should be a blocker to this PR for now. We may want to turn this comment into an issue once the PR is merged.

jupyterlite_sphinx/jupyterlite_sphinx.js Outdated Show resolved Hide resolved
@steppi
Copy link
Collaborator Author

steppi commented Nov 7, 2023

This all look sensible to me.

I'm just afraid that manually parsing the rst like this would not scale very well and may be difficult to maintain. I wonder if we can use docutils's parsers or write our own parser inheriting from the base ones. Anyway, I don't think this should be a blocker to this PR for now. We may want to turn this comment into an issue once the PR is merged.

Yeah, that makes sense. Scalability seems surprisingly Ok, SciPy is probably among the top packages for amount of docstring content, and the impact on the already enormous doc build times wasn't terrible. 100% agree on this manual parsing being more difficult to maintain and just overall more brittle. I didn't know how to make it work with an existing parser so just kind of went for it with the manual approach. Thanks for not considering this a blocker, and good idea to make an issue.

@martinRenou
Copy link
Member

SciPy is probably among the top packages for amount of docstring content, and the impact on the already enormous doc build times wasn't terrible

Yep that's very good!

One thing I'm wondering is how it would work for documentation that's written in Markdown using something like myst-parser. Can we expect this code to work in that case?

Happy to merge this as-is now and we can iterate on it :)

@steppi
Copy link
Collaborator Author

steppi commented Nov 7, 2023

One thing I'm wondering is how it would work for documentation that's written in Markdown using something like myst-parser. Can we expect this code to work in that case?

Huh, not sure. I imagine the parsing would be simpler in that case. It would probably better as a separate directive I think.

Happy to merge this as-is now and we can iterate on it :)

Sounds great! Thanks!

Copy link
Member

@martinRenou martinRenou left a comment

Choose a reason for hiding this comment

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

Thank you! 🚀

@martinRenou martinRenou merged commit f8a77cc into jupyterlite:main Nov 7, 2023
4 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Generate notebooks from Examples section in docstring
3 participants