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

Support the usage of Markdown-based notebooks with "Lite" directives #221

Open
wants to merge 14 commits into
base: main
Choose a base branch
from

Conversation

agriyakhetarpal
Copy link
Collaborator

@agriyakhetarpal agriyakhetarpal commented Dec 3, 2024

Description

Tip

This PR is a work in progress, and the description shall be updated when ready.

Please refer to the proposal in #191 for additional context and the rationale behind this change. This PR adds preprocessing functionalities to the base "Lite" directive (in the _LiteIframe class), which will allow a user to pass in a plain Markdown/MyST-Markdown notebook file to a directive, which will then get converted on the fly when JupyterLite is triggered. The conversion is performed via jupytext, which has been included as a dependency.

There is also the option of leaving this as an experimental feature behind an experimental flag and/or an optional dependency on jupytext, as suggested in #191 (comment). However, I haven't implemented that change and would like to wait for further direction on this before I do so.

Changes made

  • Preprocessing added to the run() method of the _LiteIframe class to convert .md files to .ipynb files and pass them on to JupyterLite
  • Documentation for all directives updated to showcase this functionality
  • A sample MyST Markdown notebook added

Additional context

This is supposed to make interacting with Markdown-based notebook workflows easier – a few notable examples are scipy/scipy#20303 and PyWavelets/pywt#741, which have their own ways of dealing with this intermediate step that might make sense to include in jupyterlite-sphinx itself through this PR, such as https://github.com/scipy/scipy/blob/82ee02556872b9f91576b835a8f40f1d9eb1fd8d/doc/convert_notebooks.py and https://github.com/agriyakhetarpal/pywt/blob/fd3e01c3003a18cca770fc6f33752f8c07084e21/doc/source/conf.py#L33-L49 respectively.

cc: @melissawm @steppi @gabalafou

@agriyakhetarpal agriyakhetarpal added the enhancement New feature or request label Dec 3, 2024
@agriyakhetarpal agriyakhetarpal added this to the 0.16.6 milestone Dec 3, 2024
@agriyakhetarpal
Copy link
Collaborator Author

I am facing some unrelated troubles when testing it locally with the SciPy documentation from SciPy's main branch right now, and I intend to test this a bit more thoroughly with multiple projects, also to ensure that things like scipy/scipy#21433 do not break with this change.

Comment on lines 344 to 348
# For MyST Markdown notebooks, we create a unique target filename to avoid
# collisions with other IPyNB files that may have the same name.
if notebook_path.suffix.lower() == '.md':
target_name = f"{notebook_path.stem}_{uuid4().hex[:8]}.ipynb"
target_path = notebooks_dir / target_name
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Here, this is an assumption I make, which, while would be true for the jupyterlite-sphinx documentation because I've added a file my_notebook.md of the same name to accompany the IPyNB file – is a bit unlikely to be true for downstream projects:

  • the notebooks would either be all IPyNB files, or all Markdown-based, or some other format entirely
  • even if the notebooks are undergoing a migration from one format to another by a contributor such (as is the case for SciPy), it's unlikely that two notebooks with the same name but different formats, foo.ipynb and foo.md, would exist under the same folder (or different folders for the same matter).

If I am right in my assessment, we can drop the use of the UUID here and go ahead with these assumptions. Could someone help validate?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

The commit 0401426 resolves this, handling the collision by using a _converted suffix for the MyST notebook but leaves IPyNB files intact. For example, it can be tried out here: https://jupyterlite-sphinx--221.org.readthedocs.build/en/221/directives/voici.html for the IPyNB and the MD variants.

My observation regarding UX for this is that this change in the notebooks' filename(s) would not be noticeable for usage within the Voici and NotebookLite directives, since they do not showcase the filename at all when the kernel is spawned. The JupyterLite directive, however, does do so (regardless of whether we are in a new tab or within the same tab), but I think most users won't find it bothersome. Further, if one switches to the "Simple Interface" under "Appearance", the notebook occupies the entire screen and does not appear in a tab.

I wonder if there is a method to open notebooks under our JupyterLite's new tab option with the Simple Interface directly, though, because it's neater that way... I'll follow up on jupyterlite/jupyterlite#385.

@agriyakhetarpal
Copy link
Collaborator Author

I am facing some unrelated troubles when testing it locally with the SciPy documentation from SciPy's main branch right now, and I intend to test this a bit more thoroughly with multiple projects, also to ensure that things like scipy/scipy#21433 do not break with this change.

Update: I've tested it multiple times by now and this is working perfectly well with the SciPy docs 🎉 though the use of a UUID to append to the filename would break the caching (obviously) of the notebook builds and also adds additional notebooks to the built docs. So, I've chosen to adapt the caching-related check from scipy/scipy#21433 in e3b6882, and I can confirm that there are no regressions as those that were noticed in scipy/scipy#21394. This does mean that I've had to revert the commit via d1c7f7d where I added a sample my_notebook.md file to accompany our existing my_notebook.ipynb file, because of the naming conflict that will be caused after conversion (which is something I'm looking for opinions on – please see #221 (comment) for context!)

When this PR becomes part of a release, we can remove the convert_notebooks.py script and the convert-notebooks Make target in SciPy, essentially eliminating the changes introduced through scipy/scipy#21433, and instead of having to write:

.. jupyterlite:: ../../_contents/hypothesis_chi2_contingency.ipynb
   :new_tab: True

we can just write:

.. jupyterlite:: hypothesis_chi2_contingency.md
   :new_tab: True

which would be much cleaner – and I can submit a follow-up PR to SciPy for this, or step back if you wish to do so, @melissawm. :) This PR should now be ready for review now!

@agriyakhetarpal agriyakhetarpal marked this pull request as ready for review December 3, 2024 23:42
@agriyakhetarpal
Copy link
Collaborator Author

Note; we can also allow both notebook_X.ipynb and notebook_X.md to coexist in the same folder, and just add a suffix for the Markdown-based case, so that using .. notebooklite:: notebook_X.ipynb opens the original notebook, but using .. notebooklite:: notebook_X.md would convert notebook_X.md to notebook_X_converted.ipynb (or perhaps something like notebook_X.md.jupyterlite) and open it, which would perform both actions: resolving such an edge case, and keeping both notebooks intact in the documentation's source folder.

The docs build is failing because of this, but I would like to receive further direction before I proceed with this approach.

@melissawm
Copy link
Contributor

Go ahead - that would be great!

@agriyakhetarpal
Copy link
Collaborator Author

Thanks – will do when this is reviewed and released!

@jtpio
Copy link
Member

jtpio commented Dec 4, 2024

Thanks @agriyakhetarpal, nice to see support for markdown-based notebooks being added here!

Looks like the docs failure may be relevant though: https://readthedocs.org/projects/jupyterlite-sphinx/builds/26477597/

FileNotFoundError: [Errno 2] No such file or directory: '/home/docs/checkouts/readthedocs.org/user_builds/jupyterlite-sphinx/checkouts/221/docs/directives/my_notebook.md'

@agriyakhetarpal
Copy link
Collaborator Author

Thanks @agriyakhetarpal, nice to see support for markdown-based notebooks being added here!

Looks like the docs failure may be relevant though: readthedocs.org/projects/jupyterlite-sphinx/builds/26477597

FileNotFoundError: [Errno 2] No such file or directory: '/home/docs/checkouts/readthedocs.org/user_builds/jupyterlite-sphinx/checkouts/221/docs/directives/my_notebook.md'

Yes, this is related to #221 (comment) and #221 (comment). Let me push a few more changes to handle that; however, I'll fix it for now by reverting the previous commit!

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.

3 participants