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

Tutorials infrastructure: switch to text-based notebook workflow #59

Closed
wants to merge 38 commits into from

Conversation

alexdesiqueira
Copy link
Member

Changes the workflow to use a text-based format for Jupyter notebooks and sphinx+myst for running and testing them, continuing changes added by @jni.
Kindly stolen from @rossbar's numpy-tutorials.

@alexdesiqueira alexdesiqueira added the wip Work in progress label Jun 23, 2021
@pep8speaks
Copy link

pep8speaks commented Jun 23, 2021

Hello @alexdesiqueira! Thanks for updating this PR. We checked the lines you've touched for PEP 8 issues, and found:

There are currently no PEP 8 issues detected in this Pull Request. Cheers! 🍻

Comment last updated at 2021-12-10 07:09:55 UTC

@alexdesiqueira alexdesiqueira linked an issue Jun 23, 2021 that may be closed by this pull request
4 tasks
@alexdesiqueira
Copy link
Member Author

Website looks like this so far:
image

@alexdesiqueira
Copy link
Member Author

How it looks so far:
image

@alexdesiqueira
Copy link
Member Author

Alright, I think that's it as a starting point.
We need to discuss how the tutorials will be structured, but the main machinery is here already.

@alexdesiqueira alexdesiqueira removed the wip Work in progress label Jun 23, 2021
@alexdesiqueira alexdesiqueira requested review from jni and stefanv June 23, 2021 23:55
Copy link
Member

@jni jni left a comment

Choose a reason for hiding this comment

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

@alexdesiqueira in addition to the minor comments below, why are the existing tutorials not migrated here? I would prefer that ongoing PRs only add content rather than removing it! 😂

- name: GitHub Pages action
if: github.repository == 'scikit-image/skimage-tutorials' && github.ref == 'refs/heads/main' && github.event_name == 'push'
uses: peaceiris/actions-gh-pages@v3.6.1
with:
github_token: ${{ secrets.GITHUB_TOKEN }}
publish_dir: ./_build/html
publish_dir: ./website/_build/html
Copy link
Member

Choose a reason for hiding this comment

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

Why this move? I don't love it...

Copy link
Member Author

Choose a reason for hiding this comment

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

I felt it'd make things more organized. I don't mind, though. Would you like me to leave it as it was?

_toc.yml Outdated Show resolved Hide resolved
content/00_images_are_arrays.md Outdated Show resolved Hide resolved
@alexdesiqueira
Copy link
Member Author

alexdesiqueira commented Sep 13, 2021

why are the existing tutorials not migrated here? I would prefer that ongoing PRs only add content rather than removing it!

@jni* We're not removing anything yet! 😂 If I convert everything now, this PR will be huge; the idea here was only to add the infrastructure — the tutorial(s) presented here are examples 🙂

@alexdesiqueira
Copy link
Member Author

Thank you @stefanv!

@alexdesiqueira alexdesiqueira requested a review from jni December 3, 2021 00:36
@stefanv
Copy link
Member

stefanv commented Dec 3, 2021

Something isn't right with the caching: it rebuilds all notebooks on each make.

@stefanv
Copy link
Member

stefanv commented Dec 3, 2021

The Makefile is definitely not right... refers to a lessons dir that does not exist, directories are off, etc.

@alexdesiqueira
Copy link
Member Author

The Makefile is definitely not right... refers to a lessons dir that does not exist, directories are off, etc.

@stefanv thanks; gonna check that.

@stefanv
Copy link
Member

stefanv commented Dec 3, 2021

I will push up a patch, then you can work on top of that.

@stefanv
Copy link
Member

stefanv commented Dec 3, 2021

Actually, the whole thing is completely broken :)

@alexdesiqueira
Copy link
Member Author

😂 Sorry about that.

@stefanv
Copy link
Member

stefanv commented Dec 3, 2021

OK, I pushed up a new Makefile and switched to using jupytext to render directly. Jupytext correctly passes through failing cells with the raises-exception tag. Other tags (remove-input, remove-output) are handled by myst_nb.

Process now is:

  1. Files are stored as myst notebooks (.md)
  2. Jupytext converts to ipynb and executes inline
  3. myst_nb renders those notebooks inside Sphinx

There's no caching, but make will not rebuild a notebook unless it has changed. I.e., if a .md source file is modified, the notebook is re-rendered and re-executed.

There will be failures on CI. These are real failures that need to be fixed.

@rossbar
Copy link
Contributor

rossbar commented Dec 14, 2021

Adding my two cents from afar...

Though the build system is a departure from the jupyter-book or pure-sphinx workflows, it LGTM and shouldn't make a difference to tutorial authors one way or the other once everything is working. From trying it out, there are two main features that you get from the sphinx/jb workflows that are missing here:

  • For whatever reason, the execution caching doesn't seem to be working. It seems to me based on the Makefile that the notebooks should only be executed on a re-build if their content has changed, but in practice all the notebooks are re-executed on each build. Maybe the myst-nb sphinx extension is touch-ing or otherwise modifying the timestamps of the generated .ipynb files during the sphinx build? It's not immediately obvious to me why this isn't working...
  • Another feature for the build system that would be nice would be the ability to continue building even after execution of a notebook fails. Currently if the jupytext --execute fails, then the full make command exits and the outputs aren't generated. With the sphinx workflow, the default behavior is that execution failures are treated as warnings and the full execute-and-convert-to-html procedure continues to completion. This is a nice feature because authors/reviewers can see the execution failures in the output html, which IME has helped a lot with reviewing etc. in numpy-tutorials and nx-guides.

@stefanv
Copy link
Member

stefanv commented Dec 14, 2021

@rossbar do you want to update the build system to use the Sphinx workflow? Fine with me, as long as we have the ability to error on failed build for CI purposes.

@stefanv
Copy link
Member

stefanv commented Dec 14, 2021

I am a bit surprised at the rebuilds. That's the whole point of make, but I observe the same thing.

@rossbar
Copy link
Contributor

rossbar commented Dec 14, 2021

do you want to update the build system to use the Sphinx workflow?

Sure, I'm happy to do so, if for no other reason than to provide a point of comparison.

Fine with me, as long as we have the ability to error on failed build for CI purposes.

Yeah this is a bit of a sore point: I do this with the SPHINXOPTS=-W --keep-going flags: -W to elevate warnings to errors, and --keep-going to ensure that everything runs to completion. The downside is that it elevates all sphinx warnings to errors, not just execution failures. FWIW this is a requested enhancement in myst-nb, see e.g. executablebooks/MyST-NB#248

@alexdesiqueira
Copy link
Member Author

Superseded by #66; closing this one in favor of #66 then. Thank you @rossbar!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Migrating tutorials to website todo
5 participants