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

Alternative build system to #59 #66

Open
wants to merge 26 commits into
base: main
Choose a base branch
from
Open

Conversation

rossbar
Copy link
Contributor

@rossbar rossbar commented Dec 15, 2021

This draft PR follows from the discussion in #59. To test this build system:

pip install -r requirements.txt
make html

This is the standard Makefile you get from sphinx-quickstart. I incorporated @stefanv's fixes to make some of the examples run again, but there were some that haven't been fixed yet, e.g. adv{3, 4, 5}_*. Even though these notebooks encounter exceptions, they site is still generated and can still be viewed in the browser (e.g. firefox _build/html/index.html). This is really handy for reviewing and works will with common CI site preview workflows. The caching is now handled by jupyter_cache rather than being handled in a Makefile. It's not perfect (and doesn't handle dependencies anywhere near as nicely as a Makefile does) but should have the property that only notebooks that have been modified will be executed on a rebuild.

@rossbar rossbar marked this pull request as draft December 15, 2021 06:17
@rossbar
Copy link
Contributor Author

rossbar commented Dec 15, 2021

I used a script to autoconvert all the .ipynb files to markdown-based notebook format, but this really gummed up the PR with diffs that aren't relevant for the discussion... I will back those changes out.

@rossbar
Copy link
Contributor Author

rossbar commented Jan 13, 2022

I managed to figure out how to unwind my "convert all ipynb -> md w/ jupytext" commit and limit the conversions to only those files that are actually used in the site (so far). The toctrees in introduction.md and applications.md summarize which files have been converted.

@rossbar rossbar marked this pull request as ready for review January 13, 2022 02:19
@rossbar
Copy link
Contributor Author

rossbar commented Jan 13, 2022

I'm partial to CircleCI for building sphinx sites because of the nice preview options but I didn't bother adding a whole new CI service in this PR. In the absence of a CI preview, here's what the site looks like rendered as of 8b7a95a : https://rossbar.github.io/skimage-tutorials/

@stefanv
Copy link
Member

stefanv commented Jan 13, 2022

Hey, Ross, this looks great and is a huge improvement over what we have right now. I'm happy to merge, but there's a small issue with the "imageconverts" package not found.

@rossbar
Copy link
Contributor Author

rossbar commented Jan 13, 2022

Hey, Ross, this looks great and is a huge improvement over what we have right now. I'm happy to merge, but there's a small issue with the "imageconverts" package not found.

Gah a silly mistake, it should be imagecodecs

@stefanv
Copy link
Member

stefanv commented Jan 13, 2022

The build failed, but it's hard to see why. How about adding those logs as build artifacts?

@rossbar
Copy link
Contributor Author

rossbar commented Jan 13, 2022

How about adding those logs as build artifacts?

One of the advantages of this build system is that failures in the notebook execution actually show up in the rendered site. The adv3, adv4, and adv5 notebooks all fail to execute to completion (this is expected, they didn't execute in #59 either) and you can see the failures in the rendered site. For example,

It looks like there was also a cell execution timeout in three_dimensional_image_processing. myst-nb throws a cell execution timeout error cells that take longer than 30 seconds to execute (the timeout period is configurable).

I've found that CircleCI is really a great fit for this workflow because you can upload the _build/html folder to get both the execution logs (which aren't as useful as they could be) as well as the site for previewing, even when the execution fails.

@stefanv
Copy link
Member

stefanv commented Jan 14, 2022

@rossbar Would you like to add CircleCI now or do that in another PR? I'm happy with either.

@alexdesiqueira
Copy link
Member

Hey @stefanv @rossbar,
I think we forgot this 🙂

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.

None yet

3 participants