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

Add missing dependency on jupytext #236

Merged

Conversation

agriyakhetarpal
Copy link
Member

Description

The 0.17.0 release added jupytext via #221 as a dependency, but it was added in [docs] but not here – which means that jupyterlite-sphinx will fail on import (see conda-forge/jupyterlite-sphinx-feedstock#37).

This PR corrects it (my apologies!) and adds it as a default dependency instead. I think the reason I added it to an optional dependency was that I was going to plan it as an optional dependency under an experimental flag, but we didn't have a discussion about it later.

@agriyakhetarpal
Copy link
Member Author

@steppi, could we please yank the 0.17.0 release on PyPI with this reason, and publish a new 0.17.1 release? Thanks!

@steppi
Copy link
Collaborator

steppi commented Dec 22, 2024

@steppi, could we please yank the 0.17.0 release on PyPI with this reason, and publish a new 0.17.1 release? Thanks!

I don't have access to yank on PyPI, but can publish the new release. @martinRenou, @jtpio, would one of you be able to yank 0.17.0?

Also, we should add tests in CI that build docs for a simple site to make sure everything works.

@steppi steppi merged commit f51bdc6 into jupyterlite:main Dec 22, 2024
5 of 6 checks passed
@agriyakhetarpal
Copy link
Member Author

Also, we should add tests in CI that build docs for a simple site to make sure everything works.

I agree; I was writing an issue about it just now, will post my thoughts there.

@steppi steppi added bug Something isn't working and removed maintenance labels Dec 22, 2024
@jtpio
Copy link
Member

jtpio commented Dec 22, 2024

hmm do we need the dependency on jupytext?

I didn't fully followed #221, but could markdown support be achieved with myst_parser or its markdown_it dependency? Also since myst may be a more common dependency to have in environment where jupyterlite-sphinx is used (typically docs).

@jtpio
Copy link
Member

jtpio commented Dec 22, 2024

I don't have access to yank on PyPI, but can publish the new release. @martinRenou, @jtpio, would one of you be able to yank 0.17.0?

Done:

image

@agriyakhetarpal
Copy link
Member Author

hmm do we need the dependency on jupytext?

I didn't fully followed #221, but could markdown support be achieved with myst_parser or its markdown_it dependency? Also since myst may be a more common dependency to have in environment where jupyterlite-sphinx is used (typically docs).

No, I don't think so, @jtpio – while MyST-parser and markdown-it are essential parts of the tooling around enabling documentation with Markdown, they are solving different problems in comparison – they largely help Sphinx understand Markdown and parse it to HTML, but what we needed for #221 was a way to be able to be able to convert Markdown notebooks to IPyNB files that we could pass on to JupyterLite. For this, Jupytext was the only project in the ecosystem that apparently has the requisite functionality. I had assumed a project like nbconvert could do this, but—on the contrary—converts IPyNB files to Markdown (which are probably, then, passed on to MyST-parser or MyST-NB for integration with Sphinx).

Thus, jupyterlite-sphinx's dependency on jupytext seems to be alright to me, because it will be a transitive one for end users, and because we currently use it only in one place.

In the medium term, to achieve proper Markdown notebook support without the need for our internal conversion, I would suggest that we advocate for being able to run Markdown notebooks themselves in JupyterLite: jupyterlite/jupyterlite#1301. When I looked at it some months ago, the only blocker here looked like this comment of yours: jupyterlite/jupyterlite#731 (comment). I wonder if there have been any updates to its surrounding behaviours recently, or if not, could you please share how one can approach a fix for it?

I recently did try using the JupyterLab-MyST plugin with JupyterLite through an overrides.json file, but it didn't work, and my lack of awareness of the tools in the Jupyter ecosystem seems to elude me. From its name, I had (incorrectly) assumed that it would allow connecting to a kernel and executing Markdown notebooks as if they were IPyNB files – what it seems to do is that it allows writing the MyST flavour of Markdown in Markdown cells in Jupyter Notebooks.

@agriyakhetarpal agriyakhetarpal deleted the fix/missing-dependency-on-jupytext branch December 22, 2024 23:41
@jtpio
Copy link
Member

jtpio commented Dec 24, 2024

Thus, jupyterlite-sphinx's dependency on jupytext seems to be alright to me, because it will be a transitive one for end users, and because we currently use it only in one place.

Ah ok, I was wondering if having jupytext installed by default could have some side effects with regular usage of JupyterLab / JupyterLite. For example this was the case recently with real time collaboration, in environments where jupytext was installed, which was not obvious to figure out at first: jupyterlab/jupyter-collaboration#214

Another option could be to make the jupytext dependency optional (in an extras_require), although it may complicate a little bit the installation process.

@agriyakhetarpal
Copy link
Member Author

Ah, I understand; yes, it would be nice not to inject jupytext into the JupyterLite environment and not dilute any extensions. I did suggest keeping it as an optional dependency in #191 (comment) under a flag, but no one talked about it in the code review, so I suppose it was not addressed.

It makes me think that we are constructing JupyterLite instances with shared dependencies (such as from a project's [docs] extras), which also creates the possibility of dependency conflicts that are not exposed to the user, besides such corruptions in the JupyterLite environment that you shared. As a solution, how would you feel about "separating" the Sphinx docs builds and the JupyterLite builds better – say, through constructing a virtual environment with virtualenv and activating it, installing JupyterLite and any extensions into that environment separately, and then discarding said environment at the end of the Sphinx build when we have all the files? This way, jupytext would be retained for converting notebooks from Markdown to IPyNB, but wouldn't have a role with JupyterLite, as it would not exist when the static files are built – further limiting its usage.

We could then have configuration options to control the version of JupyterLite, and other dependencies (such as juptyerlite-pyodide-kernel, etc.) including their versions from conf.py itself:

jupyterlite_version = "0.5.0"

# or maybe construct a conda env and add "jupyterlite_conda_dependencies" as well?
jupyterlite_pypi_dependencies = [
"jupyterlab_materialdarker>=0.6.0",
"jupytext==1.14.0"
]

# maybe? or is the recommended way to install nowadays just "pip"?
jupyterlite_npm_dependencies = [
"base16-gruvbox-dark"
]

It might look like a bit much, but could be more correct in the long run. Still, it might be possible to find a middle ground in terms of the complexity.

@jtpio
Copy link
Member

jtpio commented Jan 14, 2025

As a side-effect of having the jupytext dependency, there now seems to be a new section in the JupyterLab launcher, for example when testing with the deployment for this repo: https://jupyterlite-sphinx.readthedocs.io/en/stable/lite/lab/index.html

image

@agriyakhetarpal
Copy link
Member Author

agriyakhetarpal commented Jan 14, 2025

Thanks for that, @jtpio. I noted something the other day where the use of Jupytext shows up in Sphinx-Gallery as well, as Sphinx-Gallery uses it as a dependency:

UserWarning: Sphinx Gallery in version 0.8.2 is not supported by Jupytext. Please use sphinx-gallery<=0.7.0 instead. If that is an issue, feel free to report it at https://github.com/mwouts/jupytext/issues, or even better, prepare a PR to handle the new signature of sphinx_gallery.notebook.rst2md.

This warning doesn't fail the Sphinx build and can be suppressed relatively safely – but it does bring back the thought of separating the dependencies required by jupyterlite-sphinx from the ones that are required for the JupyterLite deployment.

@agriyakhetarpal
Copy link
Member Author

Side note and a request: in the deployment you shared above, I'm unable to press the d key when typing out code in a notebook. Could you please disable the hotkeys from the Read the Docs admin panel?

This request is similar to jupyterlite/pyodide-kernel#141, which, BTW, can be closed as it seems to have been fixed.

@jtpio
Copy link
Member

jtpio commented Jan 14, 2025

Yes maybe we can open a new issue to have it on the radar. It's not a big issue, but it could be confusing to some users and may lead them to think jupytext is fully supported in JupyterLite.

Side note and a request: in the deployment you shared above, I'm unable to press the d key when typing out code in a notebook. Could you please disable the hotkeys from the Read the Docs admin panel?

I can add you to the project on ReadTheDocs directly. If your handle there the same as on GitHub?

@agriyakhetarpal
Copy link
Member Author

Yes maybe we can open a new issue to have it on the radar. It's not a big issue, but it could be confusing to some users and may lead them to think jupytext is fully supported in JupyterLite.

Ah, that's a fair point as well. I'll open a new issue.

Side note and a request: in the deployment you shared above, I'm unable to press the d key when typing out code in a notebook. Could you please disable the hotkeys from the Read the Docs admin panel?

I can add you to the project on ReadTheDocs directly. If your handle there the same as on GitHub?

Thank you so much. Yes, my handle is the same there: https://app.readthedocs.org/profiles/agriyakhetarpal/

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants