-
-
Notifications
You must be signed in to change notification settings - Fork 487
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
DOC: Add interactive notebooks to pages in the "Usage Examples" section #741
base: main
Are you sure you want to change the base?
DOC: Add interactive notebooks to pages in the "Usage Examples" section #741
Conversation
Quite strange. The documentation is building without any problems locally... Edit: I can reproduce locally if I delete a few of the previously generated files – this is coming from Edit, again: they were failing because the |
8b4fcc8
to
4d1dcf0
Compare
4d1dcf0
to
970439b
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks @agriyakhetarpal. Don't forget to set jupyterlite_silence = True
again now that we're done debugging. After that I think this is good to merge.
cc @rgommers
Looks like that last action was done, and everything is green. Are you happy with this PR as is @agriyakhetarpal? |
Yes, I'm happy with this, @rgommers. I tested all notebooks, and everything seems to be working with |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for the hard work @agriyakhetarpal! The code and doc changes in this PR look carefully done and fine to me (modulo the rendered output discussion below), and things seem to work as advertised. Squash-merging is fine I think, no need to rewrite history.
I would like to follow up on the final result here. You already added a lot of comments in the PR description, and I largely agree with them:
The styling of the NotebookLite directive can be improved because it takes way too much of the screen's space, perhaps through a user-provided option in conf.py just like how the TryExamples directive can be configured.
Yes, this is a pretty significant regression I think. The output is too verbose, and also harder to understand because the only difference between input and output is a bit of green coloring on the left side of the input cells (rather than >>>
). Also for accessibility that is not great. It looks fixable to me, since it's only a rendering choice.
The SciPy changes (scipy/scipy#20303) seem to have the same problem - I haven't been able to follow that work/review, so let me ping @melissawm here for thoughts.
The traceback rendering is also a lot worse. We go from this:
to this:
That's 14 extra lines of unwanted output that makes the example harder to understand.
References to the API for a package (in this case, public classes and methods for PyWavelets) are not linked to when running the notebook in its inline frame, because the notebook is not running under Sphinx and cannot access Sphinx's generated HTML pages. This can be worked around via better writing the notebooks by exploring other ways to reference pages and sections from the API (say, by adding Markdown-based headings or links, etc.).
Wouldn't it be better to drop most/all of the links? Something like {func}`dwt`
isn't too clear in the downloaded or in-browser notebooks. It'd be okay to have fewer links from long-form docs to API docs I think.
Jupytext frontmatter added to all of the notebooks so that they can be executed at the time of building the documentation
Not a showstopper by itself, but it's a lot of boilerplate that I imagine either a Sphinx extension or a bit of codegen can remove.
Taking a step back, a bigger-picture question: what are we getting from this that scikit-learn isn't getting from sphinx-gallery
? My impression is that we lost the individual interactive buttons from the jupyterlite-sphinx .. try-examples::
directive, and we end up with something that's harder to maintain and visually not as nice as what sphinx-gallery
does? I'm probably missing something important here, but I don't quite see it right now.
```{eval-rst} | ||
.. currentmodule:: pywt | ||
|
||
.. dropdown:: 🧑🔬 This notebook can be executed online. Click this section to try it out! ✨ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Unresolving this to keep the discussion visible - this is an important topic I think. The sphinx-gallery
example renders a lot nicer visually; the sidebar links are much cleaner and it shouldn't require as much boilerplate in the .md
files to get those links. It looks like it'd be good for pydata-sphinx-theme
or a separate extension to have something similar.
Not a show-stopper for PyWavelets, because there's only a handful of .md
files. But I doubt that what we have here will work for a much larger project.
Styling-wise (please keep in mind I'm no frontend developer 😅 ) would it look better/make it more visually distinctive if we had labels, such as the following screenshot? This is just some CSS so would not be too much of a burden and we could apply it to SciPy as well. As far as the gallery question, my take is that the main goal here is to have live notebooks that can be edited and run from the browser immediately without having to fire up a server locally. |
Thanks for the detailed review, @rgommers!
I think @steppi would have more to comment about the NotebookLite directive's button's sizing. Now that I've revisited this after some time from when I made that comment: my understanding of the problem is that the clickable element, i.e., the yellow button with the "Open notebook" text resides in
Yes, that work involved @melissawm expanding the
I agree. I'm not sure how to improve this, but I'll try exploring the MyST-NB configuration to see if they have something to control the stack level for the traceback.
Yes, sure – I'll do that in a subsequent commit and drop them all. Keeping API terms in regular backticks without any links seems fine to me. I had thought of doing this as a last resort, but I do get that the "API reference" section is always one click (and then a few) away via the navbar, so users have easy access to classes and methods.
I would suggest keeping the notebooks in this manner. I've posted a feature request for
I can certainly improve this by undertaking the task of writing a separate Sphinx extension for this or exploring Jinja-based templating. We do have the building blocks in the PR I linked above (sphinx-gallery/sphinx-gallery#1312). I am open to suggestions if we have to modify the approach entirely – rendering this PR a playground or a canary of sorts as a precursor to larger projects where similar changes in this manner will be carried out helps us in the way that we won't need to go back to the sounding board to improve the approach. |
I think so, yes. In/Out labels will be familiar to many users, and even for those who have never used a notebook it's easy to understand. I'd say it's (module the vertical space taken) about the same as
Isn't that the same as what the "launch lite" button in the right sidebar of sphinx-gallery examples like https://scikit-learn.org/stable/auto_examples/classification/plot_classifier_comparison.html does? |
Yes, but that would only work for gallery examples. I'm not sure you could use sphinx-gallery to have that in other documents. Alternatively you could migrate all of the narrative documentation you want to be interactive to sphinx-gallery, but I'm not sure how that would impact the documentation organization. |
I did this in 1dc3a0f and replaced all of the links with just text that gets highlighted as code text. Though, now that I've looked at the rendered docs locally, I do think that this is a step back from having the links. The links are non-existent/don't work just in the interactive notebooks that JupyterLite brings forth, so removing them from the main documentation pages so that the interactive notebook gets to be cleaner doesn't seem to be the best way forward. @rgommers, please let me know if this was what you had in mind or whether there should be a way for |
This would definitely be an improvement, short of having the actual links (this is on the roadmap for MyST-nb/jupytext as far as I understand). But I'm not sure this should be done now, maybe it's a future feature request for jupyterlite-sphinx? |
True, that doesn't work for everything. Neither does a single notebook though, since that loses the
I think I agree. Perhaps we should ignore it for now, and indeed consider whether that syntax could be auto-cleaned by
It would be useful to spend some time exploring this. I don't have concrete suggestions because I don't know the various code bases of the packages involved well enough. I'm worried about doing double work here though, so some coherent picture of what is common with |
My understanding is that the |
Would it be better have a button like for the |
I don't think so, that seems pretty much always worse than a new tab for larger narrative docs. |
We just had a chat about this with @agriyakhetarpal, @melissawm, @steppi and @Carreau. Outcomes:
|
There is
|
Description
Tip
Those interested can try it at https://pywavelets--741.org.readthedocs.build/en/741/regression/index.html
The Usage Examples section, following #728 and as requested in #737 (comment), is rendered interactive through the changes in this PR through the use of
jupyterlite-sphinx
and Markdown-based notebooks that are executed by MyST-NB. Please read below for a granular overview of all of these details:Which issue does this PR solve/reference?
Addresses a part of #706
Key changes made
.rst
based files underdoc/source/regression
converted to Markdown and reformatted as notebooksconf.py
jupyterlite-sphinx
to run the notebooks under WASM in the same tab (and something like Add the option to open JupyterLite window in new tab jupyterlite/jupyterlite-sphinx#165 can be added for this directive as well).ipynb
-based notebooks that get generated during the process in order to keep the documentation build warning-free. Generating them at build time is for theNotebookLite
directive to be able to access them and load them, since the NotebookLite directive currently does not load.md
files or notebook files in other formats.i. I imagine this will be helpful for DOC: stats: Convert sampling tutorial to MyST-md scipy/scipy#20303 as well where it is required to load the notebooks in an interactive manner under a specific folder. The notebooks are not executed by Jupytext at the time of conversion, and therefore, based on past experiments, it takes ~10 seconds to convert 30 or so notebooks.
Additional context
This is just a pilot run of how notebook-based examples can be configured for Sphinx-based documentation websites, so there are a few corner cases that I have noticed so far:
conf.py
just like how theTryExamples
directive can be configured.A brief to-do list
Besides the points mentioned above, smaller tasks can be looked into:
.md
and.ipynb
)