CI: gate notebook execution errors on the sphinx-tojupyter step - #809
Merged
Conversation
The "Build Download Notebooks (sphinx-tojupyter)" step is the one that
actually executes the notebooks, and in this repo it could not fail:
- it carried no `-n -W`, so a CellExecutionError was a non-fatal warning
- its exit code was `cp`'s, not `jb build`'s
The second point is the less obvious one. The step runs three commands
under `shell: bash -l {0}`. GitHub only injects `-eo pipefail` for the
bare `shell: bash` shorthand; an explicit custom shell spec gets neither
`-e` nor `-o pipefail`. So a failing `jb build` was followed by `mkdir`
and `cp`, and the step exited with `cp`'s status. `--keep-going` makes
that worse rather than better, since it forces Sphinx to emit output
despite the errors, guaranteeing the .ipynb files exist for `cp` to
succeed on.
Adding the flags alone would therefore not have gated anything. Both
changes are needed together.
This matches the step already merged in lecture-python.zh-cn. Checked
before making the change: the repo has no `raises-exception` tags and no
intentionally-erroring cells, and all 47 published lecture pages render
without error output, so this should not turn CI red on existing content.
Refs QuantEcon/meta#340
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
✅ Deploy Preview for taupe-gaufre-c4e660 ready!
To edit notification comments on pull requests, go to your Netlify project configuration. |
There was a problem hiding this comment.
Pull request overview
Updates the CI workflow to ensure notebook execution failures are treated as fatal during the sphinx-tojupyter notebook-generation step, so CI reliably fails when notebook execution produces errors or warnings that should gate merges.
Changes:
- Adds
set -eo pipefailto prevent subsequent commands from maskingjb buildfailures undershell: bash -l {0}. - Runs
jb buildwith-n -W --keep-goingfor thecustom/jupyterbuilder to promote warnings to errors and enforce execution gating.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Corrects the previous commit, which gated the wrong step. `lectures/_config.yml` sets `execute_notebooks: "cache"`, so only the FIRST `jb build` in the workflow executes notebooks — every later builder reads the cache. In this repo the first build is "Build PDF from LaTeX", not the sphinx-tojupyter step. Gating tojupyter therefore caught nothing that matters: by the time it runs, execution has already happened. The LaTeX step was the real hole. It had `-n --keep-going` with no `-W`, so a CellExecutionError was a non-fatal warning, and it carries the same trailing-cp exit-code masking. Both are fixed here; the tojupyter step is reverted to its original form, so there is exactly one gate and it sits on the step that executes. Note for anyone porting this: the equivalent change in lecture-python.zh-cn sits on the tojupyter step, and that is correct there — its LaTeX step is commented out, making tojupyter the first jb build. The rule is "gate the first jb build", not "gate a particular step name". Refs QuantEcon/meta#340 Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
This was referenced Jul 30, 2026
Merged
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Closes the
lecture-python-introhalf of QuantEcon/meta#340.The problem
Two independent defects meant a broken notebook could pass CI green in this repo.
1. The gate was on no step that could fail.
lectures/_config.ymlsetsexecute_notebooks: "cache", so only the firstjb buildin the workflow actually executes notebooks — every later builder reads the cache. The first build here is "Build PDF from LaTeX", and it ran with-n --keep-goingand no-W, so aCellExecutionErrorwas a non-fatal warning.2. That step's exit code was
cp's, notjb build's. It runs three commands undershell: bash -l {0}. GitHub only injects-eo pipefailfor the bareshell: bashshorthand; an explicit custom shell spec gets neither-enor-o pipefail. So a failingjb buildwas followed bymkdirandcp, and the step exited withcp's status.--keep-goingmakes this worse rather than better, because it forces Sphinx to emit output despite the errors, guaranteeing the PDF exists forcpto succeed on.Either fix alone is insufficient. Both are applied, to one step.
The change
set -eo pipefailand-Won the "Build PDF from LaTeX" step — the firstjb build, and therefore the one that executes. No other step is touched and no_config.ymlchange is needed.On why it is this step and not the notebooks step: the first revision of this PR gated
Build Download Notebooks (sphinx-tojupyter), copying the equivalent change already merged inlecture-python.zh-cn. That was wrong here. It is correct there because that repo's LaTeX step is commented out, which makes tojupyter its firstjb build. The portable rule is "gate the firstjb build", not "gate a particular step name" — and which step that is varies across the six repos in QuantEcon/meta#340. Caught by @copilot's review and by @mmcky.Checks done before making the change
raises-exceptiontags anywhere inlectures/, and no intentionally-erroring cells — nothing is relying on an error being tolerated.output_error, tracebacks, ANSI red). Zero hits — the lectures execute cleanly today.suppress_warnings: [mystnb.unknown_mime_type, myst.domains]is already configured, so the two known-noisy warning classes will not be promoted by-W.What CI on this PR is testing
This step has never been able to fail, so this PR is the first real exercise of it. The scan above covers execution errors on published content;
-Walso promotes Sphinx warnings to errors, and the LaTeX builder emits warnings the HTML builder does not — that class has been invisible here.If CI goes red, it has found something genuine that was previously silent, and that should be fixed before this merges rather than the flags being softened.
Refs QuantEcon/meta#340
🤖 Generated with Claude Code