Skip to content

Fix incorrect paths for JupyterLite Notebook interface URLs, unpin jupyterlite-sphinx, and update JupyterLite integration docs#1417

Merged
lucyleeow merged 7 commits intosphinx-gallery:masterfrom
agriyakhetarpal:fix/jupyterlite-notebook-ui-paths
Jan 6, 2025
Merged

Fix incorrect paths for JupyterLite Notebook interface URLs, unpin jupyterlite-sphinx, and update JupyterLite integration docs#1417
lucyleeow merged 7 commits intosphinx-gallery:masterfrom
agriyakhetarpal:fix/jupyterlite-notebook-ui-paths

Conversation

@agriyakhetarpal
Copy link
Copy Markdown
Contributor

Description

I noticed that while JupyterLite dropped the "retro" string in the URL for the Notebook interface at some point in time, sphinx-gallery still seems to have it – and I recently hit this on scikit-image/scikit-image#7644 where I wanted to use it for the gallery notebooks. This PR fixes it, so that the buttons inserted by sphinx-gallery now open the correct link:

http://127.0.0.1:8000/lite/notebooks/index.html?path=my_notebook.ipynb,

instead of the broken one below

http://127.0.0.1:8000/lite/retro/notebooks/index.html?path=my_notebook.ipynb

It appears that this was not caught in other projects, such as scikit-learn, because the "Lab" interface is used there, for which the URL is correct here.

I also tried to add a minimal test to ensure that this breakage does not happen again, by creating a minimal web server as a pytest fixture, but realised that it would require a JupyterLite site to be built during the test suite – which is quite overkill.

Additional context

N/A

@agriyakhetarpal
Copy link
Copy Markdown
Contributor Author

For example, the Lab URL for plot_1_exp.ipynb from one of the recent PRs (#1410) works, but the corresponding URL for the Notebook interface does not, which will be fixed in this PR to point to the correct URL.

Thank you!

@lucyleeow lucyleeow added the bug label Dec 26, 2024
Copy link
Copy Markdown
Contributor

@lucyleeow lucyleeow left a comment

Choose a reason for hiding this comment

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

LGTM thanks!
Just for info, I assume this change was bought about when https://github.com/jupyterlab/retrolab was archived and integrated into Jupyter Notebook?

(CI failures unrelated)

@agriyakhetarpal
Copy link
Copy Markdown
Contributor Author

Thanks for the review!

Yes, this was indeed the case – the last instance of retrolab existing was prior to Notebook 4 here: https://github.com/jupyterlite/jupyterlite/blob/fa5f44ff3f0eec2c75cada62fa659f13c9f26ab0/packages/retro-application-extension/src/index.ts#L73

So your assessment is right, it was dropped with jupyterlite/jupyterlite#1019

@larsoner
Copy link
Copy Markdown
Contributor

CI failures should be fixed by #1416

We should probably add/update jupyterlite min pin to 0.4.2 then, since it seems like that was the release with the linked PR. Or maybe we at least need some doc update?

Also our CIs (.github/install.sh and .circleci/config.yml) appear to use "jupyterlite-sphinx>=0.8.0,<0.9.0" which is pretty old, they're on 0.17 now. We should probably update that, too, and make sure that CircleCI looks okay. @agriyakhetarpal do you want to give it a try?

@agriyakhetarpal
Copy link
Copy Markdown
Contributor Author

Ah, thank you for fixing the CI failures before I was able to! :)

I agree with both adding a minimum version for JupyterLite and updating the docs about it – just pushed the relevant changes via 821d17d and 616e547. Suggestions on improving the documentation are appreciated. I think it could make sense to move the sentence to an admonition too.

Minor points about jupyterlite-sphinx and the surrounding jupyterlite-pyodide-kernel dependencies: we released version 0.17.1 for the former which fixed a metadata issue with 0.17.0, so I would recommend that as the minimum here; for the latter, I would recommend dropping the upper bound, since both the Pyodide version and micropip are bound to jupyterlite-pyodide-kernel and users might not be able to find that out themselves without a few rounds of searching online.

@agriyakhetarpal
Copy link
Copy Markdown
Contributor Author

Also, jupyterlite-sphinx pins jupyterlite-core, and this pin is relaxed with each new release of JupyterLite, so sphinx-gallery will receive a higher/supported version as a transitive dependency.

Copy link
Copy Markdown
Contributor

@lucyleeow lucyleeow 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 for the doc updates. Just some nits, you have to double backtick here (single is just italics) because we don't set a default.

@agriyakhetarpal agriyakhetarpal changed the title Fix incorrect paths for JupyterLite Notebook interface URLs Fix incorrect paths for JupyterLite Notebook interface URLs, unpin jupyterlite-sphinx, and update JupyterLite integration docs Dec 31, 2024
Copy link
Copy Markdown
Contributor

@lucyleeow lucyleeow 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!

@lucyleeow lucyleeow merged commit 23ad58d into sphinx-gallery:master Jan 6, 2025
@agriyakhetarpal agriyakhetarpal deleted the fix/jupyterlite-notebook-ui-paths branch January 6, 2025 08:14
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants