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

Initial steps towards interactive documentation via JupyterLite #728

Merged
merged 11 commits into from
Mar 28, 2024

Conversation

agriyakhetarpal
Copy link
Collaborator

@agriyakhetarpal agriyakhetarpal commented Mar 14, 2024

Description

This PR adds interactive documentation via JupyterLite Pyodide-enabled kernels and tests them on Read the Docs.

The key changes here are:

  1. It enables the JupyterLite Pyodide kernel and the JupyterLite Sphinx extension for the documentation, for both building locally and for the hosted documentation on Read the Docs (this can be seen in the PR previews).
  2. It enables the JupyterLite extension for all of the doctest-based examples in the API reference wherever applicable, and adds a warning at the top of the notebook to warn users about how experimental these changes are.
  3. The style guidelines have been mimicked from those for SciPy, through this PR: DOC: Add support for interactive examples with jupyterlite-sphinx scipy/scipy#20019

Following this, users shall be able to run all of the examples by loading an installation of PyWavelets in notebooks inside the documentation, which can be opened in new tabs too, as necessary.

Footnotes

This is meant to address certain sections of gh-706, however, further follow-ups are required to enable interactivity for the rest of the available examples.

@agriyakhetarpal
Copy link
Collaborator Author

The RTD PR preview fails to appear here, @rgommers – could you please help debug?

@rgommers
Copy link
Member

The RTD PR preview fails to appear here, @rgommers – could you please help debug?

It did actually build: https://pywavelets--728.org.readthedocs.build/en/728/. I'll check why there's no entry in the list of CI jobs here.

@agriyakhetarpal
Copy link
Collaborator Author

Thanks for the link! I'll use that for now and for future PRs. It is a bit strange, but as a workaround, we can use this GitHub Action: https://github.com/readthedocs/actions/tree/v1/preview if needed. It looks like it is a pretty easy to configure?

@rgommers
Copy link
Member

I added an integration. Not too happy with how many permissions the RTD Oauth app wants, but all right - let's see if that worked.

@agriyakhetarpal
Copy link
Collaborator Author

I added an integration. Not too happy with how many permissions the RTD Oauth app wants, but all right - let's see if that worked.

Thank you! It does show up here now – we can switch to the PR preview action at any time, if all we need is just a link. It will require write permissions to edit the PR description.

@agriyakhetarpal
Copy link
Collaborator Author

agriyakhetarpal commented Mar 14, 2024

The Wavelet object example that you mentioned on Slack does not seem to be working – but I think that is because it's configured incorrectly (it is placed in the documentation in a .. sourcecode:: directive).

On the same page, other basic examples, i.e., those under an

Examples
--------

section and elsewhere are working wherever this heading is mentioned in the docstring, for example – this is for listing down the different types of Wavelet families available in PyWavelets. The size of the kernel is a tad too small – I'll configure that with other aesthetic changes, such as the size and shape of the "Try it" button(s).

The code snippet does take a bit to load, and did not work unless I opened it in a new tab (maybe I have way too many open tabs or something?) This example does work, and I confirmed via

import pywt
print(pywt.__version__)

that we have 1.4.1 being loaded currently. So, my immediate next step will be to configure how to import plus install the nightly WASM wheel, as noted in the links under this section.

@rgommers
Copy link
Member

The size of the kernel is a tad too small – I'll configure that with other aesthetic changes, such as the size and shape of the "Try it" button(s).

This is still WIP, right? It looks like this:

image

compared to in SciPy (which uses the same theme and plugins):

image

So, my immediate next step will be to configure how to import plus install the nightly WASM wheel, as noted in the links under this section.

Can you do that in a follow-up PR? This should be merge-able while using the PyWavelets version shipped by Pyodide.

@agriyakhetarpal
Copy link
Collaborator Author

Yes, I shall improve the styling here – there are a couple of guides available about this in the JupyterLite docs, or I can follow the footsteps of the SciPy docs and re-use a similar style narrative.

Can you do that in a follow-up PR? This should be merge-able while using the PyWavelets version shipped by Pyodide.

I personally don't think it is a good idea to merge this on the development version of the documentation when we do not have the nightly wheels set up, but a suitable workaround for now could be to make a note about this to users in the currently added admonition – referencing that the version of PyWavelets available may be a bit outdated and therefore some of the examples might not work?

I will make and verify some additional changes to ensure that the button is available on all of the examples running under the doctests. Do we need to add the button to the Usage examples too, or just these API reference examples would be enough for this PR?

@rgommers
Copy link
Member

There are no new functions in the 1.5.0 and 1.6.0 releases, and only a very small amount of behavioral changes (e.g., stricter input validation); there is nothing that will affect how the examples behave AFAIK.

Do we need to add the button to the Usage examples too, or just these API reference examples would be enough for this PR?

I'd say API reference is a good first step here. I'm assuming that that is automatic, while Usage examples will require inserting directives into the .rst files - is that correct? If so, let's do API reference only to get some experience, and leave Usage examples for later.

@agriyakhetarpal
Copy link
Collaborator Author

agriyakhetarpal commented Mar 22, 2024

I'd say API reference is a good first step here. I'm assuming that that is automatic, while Usage examples will require inserting directives into the .rst files - is that correct? If so, let's do API reference only to get some experience, and leave Usage examples for later.

Yes, the Usage examples will likely require using the NotebookLite directive. I just tested the display for all of the doctest-based examples, which are making use of the TryExamples directive (enabled by global_enable_try_examples = True in conf.py). All of them are working as expected on all but the following pages under API reference (where there are no doctest-based examples, but other code-block based examples are present):

  1. Other functions
  2. Overview of multilevel wavelet decompositions
  3. Signal extension modes
  4. Multiresolution Analysis

These can be updated in a separate PR as necessary, but I am happy to try configuring those examples to the doctests here too.

One thing that isn't working so far and what I'm currently investigating is the size of the JupyterLite notebook that gets loaded, for example:

This screenshot displays the PyWavelets documentation deployed with Sphinx locally, where a page from the API reference is currently open.

is too short and doesn't expand to display all of the code cells, in comparison to the deployment showcased in scipy/scipy#20019. It might get fixed with a try-examples.json file that I am missing.

@agriyakhetarpal
Copy link
Collaborator Author

It might be fixed with a try-examples.json file that I am missing.

This was indeed the missing thing, now everything works!

@agriyakhetarpal agriyakhetarpal changed the title [WIP]: Interactive documentation via JupyterLite Initial steps towards interactive documentation via JupyterLite Mar 22, 2024
@agriyakhetarpal agriyakhetarpal marked this pull request as ready for review March 22, 2024 07:48
@rgommers
Copy link
Member

These can be updated in a separate PR as necessary, but I am happy to try configuring those examples to the doctests here too.

Sure, if you know how to do it then why not do it straight away. All I was trying to say is that incremental improvements are okay too.

This is starting to look pretty good!

This commit moves the example for the function `pywt.data.demo_signal()` to the doctests for the
function instead of having it inside reST.

This shall render the example interactive through
the use of JupyterLite and the TryExamples
Sphinx directive.
@agriyakhetarpal
Copy link
Collaborator Author

agriyakhetarpal commented Mar 22, 2024

Okay, I have now restructured:

  • an example for pywt.data.demo_signal() from the Other functions page – I moved it to the doctests for the function so as to let jupyterlite-sphinx be able to access it, and, similarly,
  • most of the other examples that were interlaced with .. sourcecode:: python directives. They use the newer directive, which seems to offer code syntax highlighting by default.

Some more points:

  1. The Overview of multilevel wavelet decompositions page has its its reference files in doc/pyplots/ and they are end-to-end examples of analyses. It would be better to club changes to them in another PR, and so is the case for the Signal extension modes page; it contains a plotting example and some in-line examples, which I have marked with the .. try_examples:: directive manually like those mentioned in point 2 (side note: it is good thing PyWavelets isn't doesn't have a massive documentation reference haha).
  2. Also, Multiresolution Analysis, a.k.a. pywt.mra does not have any examples in the docstrings (just the two scripts in the demo/ folder).
  3. For the CWT examples, there are some that contain the # doctest: +SKIP label (but only inside the JupyterLite notebook, not the Sphinx docs). Is there something we can do about them – we don't want to break the doctests either? I do not think it is a big deal, though.

This is ready for another review, whenever you get a chance to do so! The aftermath of this PR can take care of the other pages. Maybe we should use nbsphinx or Myst-NB to render them as actual notebooks when they have been converted to such (the latter would be better and more stable in its configuration – it's developed more actively).


P.S. I modified the CSS to make the buttons left-aligned instead of being right-aligned (like SciPy's were). I feel that the left-aligned buttons look better under a left-aligned heading, but that's just my preference. Do you have thoughts on that?

@agriyakhetarpal
Copy link
Collaborator Author

agriyakhetarpal commented Mar 22, 2024

Okay, this is strange. I am receiving errors like File Load Error for 158988af_6816_4f65_a71c_affe1c6d5d90.ipynb when I try to start the kernel on any example on Read the Docs, but it works perfectly locally. The button is right-aligned too, and not left-aligned as I had configured it.

Edit: seems to be resolved now!

@agriyakhetarpal
Copy link
Collaborator Author

agriyakhetarpal commented Mar 22, 2024

Another thing I am noticing right now: the "Try it in your browser" text works on code snippets in docstrings under the

Examples
--------

heading, but it does not propagate to custom in-line examples, i.e., where we are using the .. try_examples:: directive manually – the button still shows the default "Try it with Jupyterlite" text instead of getting it from conf.py. jupyterlite-sphinx does offer a :button_text option to configure a particular example's button's text, but if it isn't present – it should consider the global configuration value.

This is as observed on the Continuous Wavelet Transform (CWT) page, and I think this is a bug. Let me file this on jupyterlite-sphinx's issue tracker.

Copy link
Member

@rgommers rgommers left a comment

Choose a reason for hiding this comment

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

LGTM, very nice work @agriyakhetarpal! The styling looks polished now, and all the examples I tried worked as expected. I think this is ready to go in, time for testing in the wild and seeing if we get any feedback/issues.

The last 5 commits are all useful, and I had zero comments on them.

A few responses:

  • .. sourcecode:: is obsolete indeed
  • Regarding pywt.mra not having examples: it'd be welcome if they were contributed, but it's a minor thing and unrelated to the interactive docs work
  • Re # doctest: +SKIP: in principle not that hard to get rid of, but I'd like to wait until SciPy updates its approach to doing that. It has nicer skip comments that actually say what the issue is (e.g., # may vary for output that, you guessed, can vary)

Maybe we should use nbsphinx or Myst-NB to render them as actual notebooks when they have been converted to such

myst-nb seems fine to me.

P.S. I modified the CSS to make the buttons left-aligned instead of being right-aligned (like SciPy's were). I feel that the left-aligned buttons look better under a left-aligned heading, but that's just my preference. Do you have thoughts on that?

I like how it looks! Happy to stay with your choice here.

@rgommers rgommers merged commit 8c63926 into PyWavelets:main Mar 28, 2024
15 checks passed
@agriyakhetarpal agriyakhetarpal deleted the test-interactive-docs branch March 28, 2024 22:30
@agriyakhetarpal
Copy link
Collaborator Author

Actually, I was working on submitting a PR today for the issue I opened over at jupyterlite-sphinx and tagged above – I should have asked you here to hold off on merging this :) But that's not a big deal and it is a minor fix too. I hope to be done with that soon, and I can always put up another PR.

@rgommers
Copy link
Member

That's perfectly okay I think - I'm sure there will be several follow-up PRs to this one to fix some issues and polish the experience.

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.

2 participants