8000 ENH Add functionality to modify Jupyterlite notebooks based on their content by lesteve · Pull Request #1113 · sphinx-gallery/sphinx-gallery · GitHub
[go: up one dir, main page]
Skip to content

ENH Add functionality to modify Jupyterlite notebooks based on their content #1113

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.

Sign up for GitHub

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

Jump to bottom
Merged
merged 12 commits into from
Mar 28, 2023
Merged

ENH Add functionality to modify Jupyterlite notebooks based on their content #1113

merged 12 commits into from
Mar 28, 2023

Conversation

lesteve
Copy link
Member
@lesteve lesteve commented Mar 23, 2023 •
edited
Loading

To be able to run in JupyterLite, notebooks may need additional code, e.g. %pip install seaborn since seaborn (or any pure-Python code) is not part of Pyodide.

Also this allows to put a warning in a markdown cell to manage user expectations at the beginning of the notebook. We could add a better warning for notebooks that we don't expect to run any time soon, e.g. the ones using pyvista.

No tests or doc for now, but this still may be enough to get some feed-back on the code.

xref #1100

@larsoner
Copy link
Contributor

I was going to complain that you need to u pdate the doc but I see you're not at that stage yet :)

@StefRe can you look to see if this makes sense?

@lesteve
Copy link
Member Author
lesteve commented Mar 24, 2023

Yep planning to write doc + tests for this when the implementation has settled down a bit.

The main things I am wondering about right now (mentioned in the TODOs), what should the signature be for the function that modify a Jupyterlite notebook (and is user-defined in conf.py):

  • modify(json_dict: dict) -> None: the one I have implemented for now. json_dict is what you get when you do json.load(open(notebook_filename)) and the function modifies the dict in place.
  • modify(json_dict: dict, notebook_filename: str) -> None. This can be convenient to have the filename to change the notebook according to its name, e.g. all the notebooks in the pyvista folder
  • modify(notebook_filename: str). The function is expected to load the notebook and writes a modification to the same file. This implies more boilerplate code in conf.py but maybe is simpler? Also you can use additional packages in conf.py, e.g. nbformat rather than our custom code for creating and writing notebooks. without adding an additional dependency of sphinx-gallery.

@StefRe
Copy link
Contributor
StefRe 8000 commented Mar 24, 2023

@larsoner: The approach seems reasonable to me, looks good.

@lesteve: I'd prefer modify(json_dict: dict, notebook_filename: str) -> None along with a note in the docs that notebook_modification_function should not / does not need to use the filename for writing (or let the function return True if it already wrote back the file or False / None if not and SG should do the writing).

@larsoner
Copy link
Contributor

+1 for dict and filename

@lesteve
Copy link
Member Author
lesteve commented Mar 27, 2023 •
edited
Loading

I think this is ready for review.

  • I have changed the signature as advised
  • I have added some doc and some tests
  • I have added more specific warnings in conf.py doc to mention more clearly which notebooks are not expected to work (the pyvista ones, plus the plotly one)

@larsoner
Copy link
Contributor

@StefRe can you take a look? If you're happy with the API and docs I'll do a pass and merge

StefRe
StefRe reviewed Mar 27, 2023
View reviewed changes
doc/configuration.rst Outdated
when you do ``json.load(open(notebook_filename))``. The function is expected
to modify ``json_dict`` in place by adding notebook cells. It is not expected
to write to the file, since ``sphinx-gallery`` is in charge of this.
``notebook_filename`` is provided for convenience becauses it is useful to
Copy link
Contributor

Choose a reason for hiding this comment

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

typo: becauses --> because

StefRe
StefRe reviewed Mar 27, 2023
View reviewed changes
sphinx_gallery/interactive_example.py Outdated
notebook_modification_function(notebook_content, notebook_filename)

with open(notebook_filename, "w") as f:
json.dump(notebook_content, f)
Copy link
Contributor

Choose a reason for hiding this comment

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

Maybe we should add here indent=2 as in

sphinx-gallery/sphinx_gallery/notebook.py

Line 347 in 7f7087d

json.dump(work_notebook, out_nb, indent=2)

so that the modified notebook keeps its readability

8000
StefRe
StefRe reviewed Mar 27, 2023
View reviewed changes
doc/configuration.rst Outdated
@@ -1196,9 +1213,11 @@ extra things will happen:
2. The built Jupyter Notebooks from the documentation will be copied to a
folder called ``<jupyterlite_contents>/`` (relative to Sphinx source
directory)
3. The rST output of each Sphinx-Gallery example will now have a
3. If ``notebook_modification_function`` is not None, this function is going to
Copy link
Contributor

Choose a reason for hiding this comment

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

None --> None

StefRe
StefRe reviewed Mar 27, 2023
View reviewed changes
Copy link
Contributor
@StefRe StefRe left a comment

Choose a reason for hiding this comment

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

@lesteve Excellent! Just some minor nits.
@larsoner LGTM.

< 8000 a class="d-inline-block" data-hovercard-type="user" data-hovercard-url="/users/StefRe/hovercard" data-octo-click="hovercard-link-click" data-octo-dimensions="link_type:self" href="/StefRe">@StefRe
Copy link
Contributor
StefRe commented Mar 27, 2023 •
edited
Loading

@lesteve maybe you could also fix the double "some" in the existing docs https://github.com/sphinx-gallery/sphinx-gallery/blame/master/doc/configuration.rst#L1134:

see `here
  <https://pyodide.org/en/latest/usage/wasm-constraints.html>`__ for some

instead of

see some `here
  <https://pyodide.org/en/latest/usage/wasm-constraints.html>`__ for some

and also remove 2 of the 3 ..., lines here https://github.com/sphinx-gallery/sphinx-gallery/blame/master/doc/configuration.rst#L1159

@lesteve
Copy link
Member Author
lesteve commented Mar 28, 2023

Thanks a lot for the review, I think I have tackled all of the comments, let me know if you have some more!

@StefRe
Copy link
Contributor
StefRe commented Mar 28, 2023

@lesteve Perfect, no more comments 👍 .

larsoner
larsoner reviewed Mar 28, 2023
View reviewed changes
Copy link
Contributor
@larsoner larsoner left a comment

Choose a reason for hiding this comment

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

Just one tiny idea, otherwise LGTM!

@lesteve @larsoner
Update sphinx_gallery/interactive_example.py
96a75f0 
Co-authored-by: Eric Larson <larson.eric.d@gmail.com>
@larsoner larsoner merged commit 509c458 into sphinx-gallery:master Mar 28, 2023
@larsoner
Copy link
Contributor

Thanks @lesteve !

@StefRe StefRe mentioned this pull request Mar 28, 2023
Open
@lesteve lesteve mentioned this pull request Apr 14, 2023
Closed
clrpackages referenced this pull request in clearlinux-pkgs/pypi-sphinx_gallery Apr 18, 2023
@fenrus75
pypi-sphinx_gallery: Autospec creation for update from version 0.12.2…
a19d2a8 
… to version 0.13.0

v0.13.0
-------

**Implemented enhancements:**

-  ENH: Create backreferences for default roles `#1122 <https://github.com/sphinx-gallery/sphinx-gallery/pull/1122>`__ (`StefRe <https://github.com/StefRe>`__)
-  ENH raise error in check_jupyterlite_conf with unknown key `#1119 <https://github.com/sphinx-gallery/sphinx-gallery/pull/1119>`__ (`lesteve <https://github.com/lesteve>`__)
-  ENH Add functionality to modify Jupyterlite notebooks based on their content `#1113 <https://github.com/sphinx-gallery/sphinx-gallery/pull/1113>`__ (`lesteve <https://github.com/lesteve>`__)
-  ENH: Add support for WebP `#1111 <https://github.com/sphinx-gallery/sphinx-gallery/pull/1111>`__ (`StefRe <https://github.com/StefRe>`__)

**Fixed bugs:**

-  ENH Clean-up code by early initialization of sphinx_gallery_conf `#1120 <https://github.com/sphinx-gallery/sphinx-gallery/pull/1120>`__ (`lesteve <https://github.com/lesteve>`__)
-  FIX JupyterLite button links `#1115 <https://github.com/sphinx-gallery/sphinx-gallery/pull/1115>`__ (`lesteve <https://github.com/lesteve>`__)
-  Fix thumbnail text formatting `#1108 <https://github.com/sphinx-gallery/sphinx-gallery/pull/1108>`__ (`StefRe <https://github.com/StefRe>`__)

(NEWS truncated at 15 lines)
@lesteve lesteve deleted the enable-jupyterlite-notebooks-modification branch April 18, 2023 09:51
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@larsoner larsoner larsoner left review comments

@StefRe StefRe StefRe left review comments

Assignees
No one assigned
Labels
enhancement
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@lesteve @larsoner @StefRe
0