Make jupytext an optional dependency - #336
Conversation
|
Hi @mfisher87, I requested a review from you too, assuming that you are interested in reviewing PRs going forward! Thanks for your work on |
mfisher87
left a comment
mfisher87
left a comment
There was a problem hiding this comment.
Hi @mfisher87, I requested a review from you too, assuming that you are interested in reviewing PRs going forward! Thanks for your work on
jupyterlite-sphinxtowards the latter half of last year when I was (sort of) absent from development for a while 😄
Happy to review! LGTM! One bit of feedback.
| if jupytext is None: | ||
| raise ImportError( | ||
| "jupyterlite-sphinx requires the jupytext package to process Markdown notebooks. " | ||
| 'Install it with: pip install "jupyterlite-sphinx[markdown]"' |
There was a problem hiding this comment.
| 'Install it with: pip install "jupyterlite-sphinx[markdown]"' | |
| 'Install "jupyterlite-sphinx[markdown]" with your package manager of choice.' |
With the rate of uv adoption I wonder if we should avoid pip-specific instructions? Either way is fine by me! :)
There was a problem hiding this comment.
Makes sense for the instructions to be agnostic to package managers! Your suggestion also helps with conda-forge. Thanks!
There was a problem hiding this comment.
Although unfortunately conda doesn't support an extras syntax :(
There was a problem hiding this comment.
Ah, right. I think we will have to keep it as a dependency there (I don't think it's worth the effort to set up jupyterlite-sphinx-base and then add another build output or any of those shenanigans). In that case, conda-forge users will get the dependency and never reach this scenario.
Perhaps this will be better?
"Install
jupyterlite-sphinx[markdown]orjupytextwith your package manager of choice."
There was a problem hiding this comment.
If we're keeping jupytext as a dependency of the conda-forge package, I think we're good with the message as-is (conda-forge users won't see it), but this is also OK :)
There was a problem hiding this comment.
On second thought, we will have to think through this a bit, actually. @jtpio previously had some thoughts about Jupytext being included in #236 (comment), and I believe that it won't be helpful to keep it as a dependency on conda-forge either, as users from there will still see (and have been seeing) Jupytext popping up in the JupyterLite launcher at a time when Jupytext is not yet supported in JupyterLite (jupytext/jupytext#1225). We should evaluate whether we should remove the dependency from conda-forge as well. One user of jupyterlite-sphinx's converted-Markdown-notebooks functionality from conda-forge is SciPy, but there may be others as well.
There was a problem hiding this comment.
I'm 👎🏽 on having jupytext as a dependency in the conda-forge package. Unfortunately conda-forge does not have a notion of extras, so we can provide some jupyterlite-sphinx-markdown package that is a metapackage pulling jupyterlite-sphinx and jupytext, making the feedstock of jupyterlite-sphinx a multi-ouput recipe.
There was a problem hiding this comment.
Yes, let's do that! I don't think I've actually worked with multi-output recipes and metapackages in conda-forge before, but I think it shouldn't be too hard.
There was a problem hiding this comment.
I can provide help. You will need to make an admin request to conda-forge to add a new output to the feedstock, see conda-forge/admin-requests#1567 for example
There was a problem hiding this comment.
Gotcha, thanks! I'll open an issue in the jupyterlite-sphinx feedstock to remind ourselves of this when we do the release.
Co-authored-by: Matt Fisher <3608264+mfisher87@users.noreply.github.com>
3 participants
Closes #329
Closes #258