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

Convert all notebooks to sphinx gallery format #2377

Merged
merged 17 commits into from
Sep 11, 2023
Merged

Conversation

StFroese
Copy link
Member

@StFroese StFroese commented Jul 6, 2023

As discussed in #2221

@review-notebook-app
Copy link

Check out this pull request on  ReviewNB

See visual diffs & provide feedback on Jupyter Notebooks.


Powered by ReviewNB

@StFroese
Copy link
Member Author

StFroese commented Jul 6, 2023

@maxnoe do you know why the docs build is failing? I only see Warnings in the log of the CI but no errors

@StFroese
Copy link
Member Author

StFroese commented Jul 6, 2023

nvm, fixed it. Now it's failing because of magic commands

@StFroese StFroese marked this pull request as ready for review July 6, 2023 10:52
@StFroese StFroese requested review from maxnoe and kosack July 6, 2023 10:52
@StFroese
Copy link
Member Author

StFroese commented Jul 6, 2023

fmt: skip doesn't work, any ideas?

(cta-dev) ~/D/p/c/examples ❯❯❯ black --preview examples/core/containers.py                                       ✘ 146 sphinx-gallery-notebooks ✭ ✚ ◼
error: cannot format examples/core/containers.py: Cannot parse: 90:0: ?EventContainer # fmt: skip

Oh no! 💥 💔 💥
1 file failed to reformat.

@StFroese
Copy link
Member Author

StFroese commented Jul 6, 2023

@maxnoe @kosack Does one of you know how to add sphinx-gallery to the readthedocs CI?

@maxnoe
Copy link
Member

maxnoe commented Jul 6, 2023

You need to add the needed packages to the docs extra in setup.cfg

.github/workflows/ci.yml Outdated Show resolved Hide resolved
@kosack
Copy link
Contributor

kosack commented Jul 6, 2023

I do have a problem building the environment for python 3.11:

$ mamba env create -n cta-v0.19-newdocs -f environment.yml
Could not solve for environment specs
Encountered problems while solving:
  - package numba-0.56.2-py310h62db5c2_0 requires python_abi 3.10.* *_cp310, but none of the providers can be installed

With the main branch, I can install 3.11, so it must have something to do with sphinx's dependencies. With 3.10 it works

@StFroese
Copy link
Member Author

StFroese commented Jul 6, 2023

With the main branch, I can install 3.11, so it must have something to do with sphinx's dependencies

I get the same error when I switch to the main branch and set python=3.11 in the environment.yml

@kosack
Copy link
Contributor

kosack commented Jul 6, 2023

I get the same error when I switch to the main branch and set python=3.11 in the environment.yml

Could be a macos issue: I installed the ctapipe env with mamba and 3.11 a while ago, and haven't had issues. However, it could have changed if something updated in the mean time. I opened #2379 to check.

With 3.10, I also can't build the docs on macos, but not sure if that is a general issue. The file "theta_square.py" fails with hundreds of errors like:

/Users/kkosack/Projects/CTA/Working/ctapipe/ctapipe/tools/train_particle_classifier.py:docstring of ctapipe.tools.train_particle_classifier.TrainParticleClassifier.examples:: WARNING: py:class reference target not found: t.Union
/Users/kkosack/Projects/CTA/Working/ctapipe/ctapipe/tools/train_particle_classifier.py:docstring of ctapipe.tools.train_particle_classifier.TrainParticleClassifier.examples:: WARNING: py:class reference target not found: Unicode
/Users/kkosack/Projects/CTA/Working/ctapipe/ctapipe/tools/train_particle_classifier.py:docstring of ctapipe.tools.train_particle_classifier.TrainParticleClassifier.name:: WARNING: py:class reference target not found: t.Union
/Users/kkosack/Projects/CTA/Working/ctapipe/ctapipe/tools/train_particle_classifier.py:docstring of ctapipe.t

Copy link
Contributor

@kosack kosack left a comment

Choose a reason for hiding this comment

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

The galleries seem to be missing all outputs and images.

image

And cells which should have output in between are joined and no output is generated:

image

@StFroese
Copy link
Member Author

StFroese commented Jul 6, 2023

Ah yes, I was busy with compiling this without errors. I will take a look.

@StFroese
Copy link
Member Author

Some pretty funny stuff is happening that I don't understand :D

So this is currently failing since Tools.ipynb gets stuck in an infinite loop.
I found out the following things (clone this branch and build the docs to reproduce otherwise you won't have the same directories):

  1. If I copy Tools.ipynb and Tools.json from docs/examples/core to examples/examples/core the tool gets stuck in this cell
try:
    tool2.run(argv=["--config", "Tools.json"])
except SystemExit as e:
    assert e.code == 0, f"Tool returned with error status {e}"

The debug output prints some stuff about 2023-07-11 09:39:45,239 INFO [builtins.mytool] which already seems odd since this is not a builtin.
It also states bad value: The 'iterations' trait of a MyTool instance expected an int, not the str 'badval'.. Setting the iterations to badval belongs to a cell above and this happens in a try-except block, this should actually never happen.

  1. If I copy Tools.ipynb and Tools.json from docs/examples/core to examples/examples or create a new folder examples/examples/examples, the code runs without any problem and the debug output states something like 2023-07-11 09:41:51,286 INFO [__main__.mytool]

@maxnoe, @kosack do you have any idea what's going on?

@maxnoe
Copy link
Member

maxnoe commented Jul 11, 2023

Is there another python script in the core directory that might shadow another module? Especially a standard library module?

@StFroese
Copy link
Member Author

examples/examples/core:

-rw-r--r--   1 stefan  staff      5246 Jul  7 16:09 InstrumentDescription.py
-rw-r--r--   1 stefan  staff        66 Jul  7 16:09 README.rst
-rw-r--r--   1 stefan  staff    183546 Jul 11 09:59 Tools-thisbranch.ipynb
-rw-r--r--   1 stefan  staff       158 Jul  7 16:11 Tools.json
-rw-r--r--   1 stefan  staff     10125 Jul  7 16:11 Tools.py
-rw-r--r--   1 stefan  staff     13533 Jul  7 16:10 container.h5
-rw-r--r--   1 stefan  staff      5852 Jul  7 16:09 containers.py
-rw-r--r--   1 stefan  staff  32022561 Jul 11 09:39 mytool.provenance.log
-rw-r--r--   1 stefan  staff      3212 Jul  7 16:09 provenance.py
-rw-r--r--   1 stefan  staff      7941 Jul  7 16:09 table_writer_reader.py

@StFroese
Copy link
Member Author

the problem seems to be having a file Tools.py in the same directory as Tools.ipynb.

@StFroese
Copy link
Member Author

@maxnoe, @kosack pls review again :)

The %%timeit cells don't work in the documentation but they are executable in the generated .ipynb notebooks.

@StFroese StFroese requested review from kosack and maxnoe July 11, 2023 10:46
@maxnoe
Copy link
Member

maxnoe commented Jul 11, 2023

Some pretty funny stuff is happening that I don't understand :D

We figured out what happened. The converted notebook file is called Tools.py and the config file we give to the tool is called Tools.json.

Traitlets loads all files in the directory with the same basename and extensions it supports for configuration files. So in the notebook, the tool tried to load the notebook itself as config file, resulting in an endless loop.

I opened an issue here: ipython/traitlets#850

For now I'd just propose to call the config file config.json or config.yaml instead of Tools.yaml.

Copy link
Contributor

@kosack kosack left a comment

Choose a reason for hiding this comment

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

The animations don't seem to render correctly. See here for example

In the old docs, this appears as an HTML5 video

@StFroese
Copy link
Member Author

StFroese commented Jul 13, 2023

@kosack animations are rendering now

@StFroese
Copy link
Member Author

StFroese commented Sep 8, 2023

@maxnoe done

Copy link
Member

@maxnoe maxnoe left a comment

Choose a reason for hiding this comment

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

There is a weird double examples/examples structure now and things that are rendered as part of the docs are living outside of the docs/ directory, I don't like that very much.

Why move out these things from where they are also in the toc hierarchy? Makes finding where things are defined much harder.

Also, I'd prefer to have the title be Tutorials and Examples and have the Tutorials come first.

maxnoe
maxnoe previously approved these changes Sep 8, 2023
.. _tutorials_and_examples_gallery:

Tutorials and Examples gallery
==============================
Copy link
Contributor

Choose a reason for hiding this comment

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

Minor style issue: this headline is at the section level, so the ones below it (each example/tutorial) should be a level deeper (so subsection)

For RST:

# with overline, for parts
* with overline, for chapters
= for sections
- for subsections
^ for subsubsections
" for paragraphs

Copy link
Contributor

Choose a reason for hiding this comment

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

That would fix the wrong hierarchy here:
image

Copy link
Member

Choose a reason for hiding this comment

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

Why is this a README.txt file and not a .rst file called index?

Copy link
Member Author

Choose a reason for hiding this comment

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

It's just the default sphinx gallery style

By default, Sphinx-Gallery creates all the files that are written in the sphinx-build directory, either by generating rst and images from a *.py in the gallery-source, or from creating index.rst from README.txt in the gallery-source.

I think I can change it

You may also want to pass raw rst from the gallery-source to the sphinx-build[...] To accommodate this, you may set copyfile_regex in sphinx_gallery_conf. The following copies across rst files.

sphinx_gallery_conf = {
    ...
   'copyfile_regex': r'.*\.rst',
}

Copy link
Member Author

Choose a reason for hiding this comment

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

Maybe the copyfile_regex is not needed since source and build dir are the same in this case

Copy link
Member Author

Choose a reason for hiding this comment

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

But if I do this the toctree will not be generated by sphinx-gallery

Copy link
Member

Choose a reason for hiding this comment

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

By default, Sphinx-Gallery creates all the files that are written in the sphinx-build directory,

Why did you change that so that the generated files clutter the docs source directory?

maxnoe
maxnoe previously approved these changes Sep 11, 2023
@maxnoe
Copy link
Member

maxnoe commented Sep 11, 2023

Thanks a lot!

@maxnoe
Copy link
Member

maxnoe commented Sep 11, 2023

Ok, maybe we do need to move the scripts out of the docs directory. It seems the "doctest-modules" hangs on plots being opened because it tries to run the examples.

maxnoe
maxnoe previously approved these changes Sep 11, 2023
examples/index.rst Outdated Show resolved Hide resolved
This interfered with doctests collection, running
the examples during the collection,
which, since they are not modules but scripts, blocked it.

Also added git and sphinx build dirs to pytest ignore directories.
@maxnoe
Copy link
Member

maxnoe commented Sep 11, 2023

@kosack @StFroese ok, I think this is ready now

@maxnoe maxnoe merged commit 3c5b4f3 into main Sep 11, 2023
10 of 11 checks passed
@maxnoe maxnoe deleted the sphinx-gallery-notebooks branch September 11, 2023 15:53
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants