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
DOC: Add notebook infrastructure for the docs #17322
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This looks great!
We could also add a guide on opening the tutorial as a Jupyter notebook.
Yes please! For a novice user, I've no idea how to start a tutorial. Do I type the markdown in a text editor (and then what), do I write in a jupyter notebook (and then what), how to pair the NB and md etc.
Also, would be great to have a guidance on how to make intersphinx links both from a new-style md document into existing rst pages and vice versa.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
That's great! +1 for this and the editing you've done to existing content. Just some suggestions.
I would not spend too much time on the review of the tutorial itself and leave this for later. We can even say nobody looks at this, it's just to see how it renders and we remove it before merging (my preference). Then we can do a proper PR to add content and take our time.
529e743
to
afeac0b
Compare
36bb904
to
8cb74cd
Compare
I have created an issue in MyST-NB about the |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for the update @melissawm! I have some minor suggestions but otherwise LGTM
environment.yml
Outdated
- docutils<0.18.1 | ||
#- jupytext | ||
- myst-nb |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Leftover?
- docutils<0.18.1 | |
#- jupytext | |
- myst-nb | |
- docutils<0.18.1 | |
- myst-nb |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I added this back - not sure here though. On the one hand makes it easier for folks adding their own notebooks, and folks with this setup can easily open md files as notebooks (that is what Jupytext does). On the other hand, it is an extra dependency. What do you think?
[skip ci]
Co-authored-by: Pamphile Roy <roy.pamphile@gmail.com>
I think failure in CI is related. Seems to be due to a deprecation warning in SQLAlchemy and raised by |
It is 🥲 I also observed that locally. I will investigate |
After some testing, it looks like this is currently blocked on Python 3.9. Using Python 3.10 brings all sorts of unexpected deprecation warnings from sqlalchemy, nbclient and myst_nb. I am not sure how to solve it at this point unfortunately. |
If it's just warnings, we can selectively disable them in the config file for the doc (around L150). |
Ok! Here's the update:
Let me know if this is acceptable - if it's too much extra stuff we can reevaluate, but most of these can and should be removed in the near future. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for the update @melissawm!
I would be fine moving forward with this and silencing a few warnings seems ok to me.
There is still an issue with the CI. Did you see this locally as well?
# Environment variables needed for notebooks | ||
# See gh-17322 | ||
make_params.append('SQLALCHEMY_SILENCE_UBER_WARNING=1') | ||
make_params.append('JUPYTER_PLATFORM_DIRS=1') |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I would be fine with these.
(In the same spirit of the dep tracker issue, maybe we should have a place to note things we need to do when something updates. Be it a Python version or another library. Otherwise this might stay there longer than intended, not that it would be a big issue though.)
I think this is a docutils issue - I don't see it locally but have found similar in other CIs. Let me check |
Well this is fun. It looks like the failure only happens for Python 3.9, and only if you enable |
@melissawm do you have a full traceback? If it's understood and it will be robust for Python >=3.10 we may consider just documenting the issue and moving on. |
+1 for moving the whole job to 3.10 if this works. |
5f84250
to
d4fe3d9
Compare
Ok I think I finally managed to pin this down.
nodes.Node.traverse() is obsoleted by Node.findall().Traceback (most recent call last):
File "/opt/miniconda/envs/scipy-dev/lib/python3.9/site-packages/sphinx/cmd/build.py", line 281, in build_main
app.build(args.force_all, args.filenames)
File "/opt/miniconda/envs/scipy-dev/lib/python3.9/site-packages/sphinx/application.py", line 347, in build
self.builder.build_update()
File "/opt/miniconda/envs/scipy-dev/lib/python3.9/site-packages/sphinx/builders/__init__.py", line 310, in build_update
self.build(to_build,
File "/opt/miniconda/envs/scipy-dev/lib/python3.9/site-packages/sphinx/builders/__init__.py", line 376, in build
self.write(docnames, list(updated_docnames), method)
File "/opt/miniconda/envs/scipy-dev/lib/python3.9/site-packages/sphinx/builders/__init__.py", line 568, in write
self._write_parallel(sorted(docnames),
File "/opt/miniconda/envs/scipy-dev/lib/python3.9/site-packages/sphinx/builders/__init__.py", line 592, in _write_parallel
doctree = self.env.get_and_resolve_doctree(firstname, self)
File "/opt/miniconda/envs/scipy-dev/lib/python3.9/site-packages/sphinx/environment/__init__.py", line 591, in get_and_resolve_doctree
self.apply_post_transforms(doctree, docname)
File "/opt/miniconda/envs/scipy-dev/lib/python3.9/site-packages/sphinx/environment/__init__.py", line 637, in apply_post_transforms
transformer.apply_transforms()
File "/opt/miniconda/envs/scipy-dev/lib/python3.9/site-packages/sphinx/transforms/__init__.py", line 80, in apply_transforms
super().apply_transforms()
File "/opt/miniconda/envs/scipy-dev/lib/python3.9/site-packages/docutils/transforms/__init__.py", line 173, in apply_transforms
transform.apply(**kwargs)
File "/opt/miniconda/envs/scipy-dev/lib/python3.9/site-packages/sphinx/transforms/post_transforms/__init__.py", line 35, in apply
self.run(**kwargs)
File "/opt/miniconda/envs/scipy-dev/lib/python3.9/site-packages/myst_nb/ext/execution_tables.py", line 79, in run
for node in self.document.traverse(ExecutionStatsNode):
File "/opt/miniconda/envs/scipy-dev/lib/python3.9/site-packages/docutils/nodes.py", line 225, in traverse
warnings.warn('nodes.Node.traverse() is obsoleted by Node.findall().',
PendingDeprecationWarning: nodes.Node.traverse() is obsoleted by Node.findall().
Exception occurred:
File "/opt/miniconda/envs/scipy-dev/lib/python3.9/site-packages/docutils/nodes.py", line 225, in traverse
warnings.warn('nodes.Node.traverse() is obsoleted by Node.findall().',
PendingDeprecationWarning: nodes.Node.traverse() is obsoleted by Node.findall().
The full traceback has been saved in /tmp/sphinx-err-ohqk40ub.log, if you want to report the issue to the developers.
Please also report this if it was a user error, so that a better error message can be provided next time.
A bug report can be filed in the tracker at <https://github.com/sphinx-doc/sphinx/issues>. Thanks!
make: *** [Makefile:110: html-build] Error 2
(scipy-dev) ~/projects/scipy (notebook-infra ✗) sphinx.errors.SphinxParallelError: DeprecationWarning: There is no current event loop/home/melissa/projects/scipy/doc/source/notebooks/interp_transition_guide.md: Executing notebook using local CWD [mystnb]
/opt/miniconda/envs/scipy-dev/lib/python3.10/site-packages/sphinx/util/parallel.py:84: RuntimeWarning: coroutine 'NotebookClient.async_execute' was never awaited
ret = (errmsg, traceback.format_exc())
RuntimeWarning: Enable tracemalloc to get the object allocation traceback
Traceback (most recent call last):
File "/opt/miniconda/envs/scipy-dev/lib/python3.10/site-packages/sphinx/cmd/build.py", line 281, in build_main
app.build(args.force_all, args.filenames)
File "/opt/miniconda/envs/scipy-dev/lib/python3.10/site-packages/sphinx/application.py", line 347, in build
self.builder.build_update()
File "/opt/miniconda/envs/scipy-dev/lib/python3.10/site-packages/sphinx/builders/__init__.py", line 310, in build_update
self.build(to_build,
File "/opt/miniconda/envs/scipy-dev/lib/python3.10/site-packages/sphinx/builders/__init__.py", line 326, in build
updated_docnames = set(self.read())
File "/opt/miniconda/envs/scipy-dev/lib/python3.10/site-packages/sphinx/builders/__init__.py", line 431, in read
self._read_parallel(docnames, nproc=self.app.parallel)
File "/opt/miniconda/envs/scipy-dev/lib/python3.10/site-packages/sphinx/builders/__init__.py", line 487, in _read_parallel
tasks.join()
File "/opt/miniconda/envs/scipy-dev/lib/python3.10/site-packages/sphinx/util/parallel.py", line 105, in join
if not self._join_one():
File "/opt/miniconda/envs/scipy-dev/lib/python3.10/site-packages/sphinx/util/parallel.py", line 126, in _join_one
raise SphinxParallelError(*result)
sphinx.errors.SphinxParallelError: DeprecationWarning: There is no current event loop
Sphinx parallel build error:
DeprecationWarning: There is no current event loop
make: *** [Makefile:110: html-build] Error 2
(scipy-dev) ~/projects/scipy (notebook-infra ✗)
From my tests this version I just pushed should work with all Python versions and parallel execution as well. (I didn't need to bump the CircleCI Python version) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Awesome 👏 thanks for tracking this down! Approving on my side. We can try to discuss this tomorrow to get this in. Unless someone wants to merge now which is fine with me too OC 😃
EDIT: folks and maintainers present at todays meeting were in favour of this change.
This is awesome. Regarding questions 1-3 of your email to the mailing list, I am not personally concerned with 1 (the new doc build dependencies), and I wouldn't want this PR to be held up by 2/3. Can those be addressed if we start to notice a problem? How do you envision that new tutorials would be added? (e.g. would they all go in this executable notebook section at the bottom? How would that be organized, and what would the relationship be with the existing tutorials?) Would it be worthwile to convert existing tutorials to this new format (maybe breaking them up into separate pages) so that new tutorials can be fit into the existing "User Guide"? |
+1 from me. Would be great to do a test-drive while there's time until the next release. Note to self to update the |
This looks quite good to me - doc dependencies and build time don't seem to be problems I'd say. One question: there's mention of these as notebooks and being downloadable, but how? I don't see a link or button. |
Hi all!
Absolutely, I'd be happy to revisit any of these decisions if we feel like performance is an issue or we want to move these out. Nothing here is irreversible.
That is an initial proposal that I'm also happy to revisit later, I don't have strong opinions on this.
I'd say this is up for discussion. This is an easier way to share executable tutorials with users, but this also relates to the next point.
This is true - the PyData Sphinx theme, as I understand it, does not readily support the download buttons you would see on other themes (see, for example, https://numpy.org/numpy-tutorials/). However, each individual .md file here, if downloaded, can be opened by jupyter classic or jupyterlab as long as the jupytext extension is installed - so downloading this file allows you to open it as a notebook and execute locally, edit it etc. This can absolutely be perfected here or in the future. |
I'd just remove the mention on the front page of the tutorials then that these are downloadable notebooks - it's just going to lead to confusion. Once pydata-sphinx-theme gets support, the statement can be put back. |
[skip azp] [skip cirrus] [skip actions]
Also +1, looks like a nice addition |
Let's get this in 😃 thanks Melissa for the heavy lifting and everyone for the review and testing! |
Thank you all! |
This is a first proposal for adding MyST-NB notebooks to the SciPy docs.
This PR is still in draft as there are a few things still to be decided:
Reference issue
See #5233
What does this implement/fix?
Adds myst_nb as a Sphinx extension, adds a new MyST notebook tutorial (as a markdown file) and also reorganizes the user guide landing page.