CI: align build safety with lecture-python.zh-cn and the source repos - #270
Merged
Merged
Conversation
Ports QuantEcon/lecture-python.zh-cn#185 to this edition, closing the QuantEcon/meta#340 gap here. ci.yml - `-n -W --keep-going` on the sphinx-tojupyter build. It previously ran with no flags, so a notebook whose cells raised still exited zero. - `set -eo pipefail` in that same block. The flag alone is insufficient: `shell: bash -l {0}` is a custom shell spec, so GitHub injects no `-eo pipefail`, and the trailing mkdir/cp become the step's exit code. `--keep-going` makes it worse, guaranteeing the .ipynb files exist for `cp` to succeed on. Found by Copilot review on zh-cn#185. - Upload Execution Reports (Download Notebooks), so a failure in that step leaves a traceback artifact. - Clear stale Sphinx environment before the builds. - HTML artifact renamed execution-reports -> execution-reports-html. Required, not cosmetic: upload-artifact runs with `overwrite: false`, so a second upload under the same name would collide when both steps fail in one run. cache.yml - `-W --keep-going` -> `-n -W --keep-going`, so the weekly cold build is strict on missing references as well as warnings, matching lecture-python.zh-cn. ci-cn.yml is deliberately unchanged: it has no sphinx-tojupyter step and its HTML build is a single command, so its exit code is already the build's. That is why it — and not ci.yml — caught the prob_dist break in #269. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
✅ Deploy Preview for playful-platypus-17d3bb ready!
To edit notification comments on pull requests, go to your Netlify project configuration. |
There was a problem hiding this comment.
Pull request overview
This pull request tightens CI correctness for this edition by making the sphinx-tojupyter (download notebooks) build fail on execution/warning errors the same way the upstream/peer editions do, and by exposing execution reports when failures occur.
Changes:
- Clear cached Sphinx doctrees before building, avoiding stale-environment artifacts affecting builds.
- Make the notebook build strict and failure-propagating (
set -eo pipefail+-n -W --keep-going) and upload its execution reports on failure. - Align the weekly cache workflow to also run with
-n -W --keep-goingso scheduled “green” is meaningfully strict.
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated no comments.
| File | Description |
|---|---|
.github/workflows/ci.yml |
Ensures notebook builds fail correctly under bash -l {0}, clears stale doctrees, and uploads distinct execution-report artifacts on failure. |
.github/workflows/cache.yml |
Makes the scheduled cache build strict on missing refs (-n) in addition to warnings (-W). |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
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.
Ports QuantEcon/lecture-python.zh-cn#185 to this edition, closing the QuantEcon/meta#340 gap here. This is the follow-up flagged in #269.
Why now
prob_distwas broken onmainfor two days (resync in #256 → fixed in #269) and this repo's CI never caught it. The sphinx-tojupyter step ran with no flags, so notebook execution errors did not fail it. The break surfaced only because #268 happened to trigger a build, and was independently confirmed by the scheduled cache run at 04:14 today, which failed on the sameFileNotFoundError.The weekly cron is real protection, but it is a week wide. This closes the gap to per-PR.
What changes
ci.ymlset -eo pipefail+-n -W --keep-goingif: failure()-nW --keep-goingcache.yml—-W --keep-going→-n -W --keep-going, so the weekly cold build is strict on missing references as well as warnings. Without this, "green" means something weaker here than onlecture-python.zh-cn, which is a confusing property for two editions that are meant to be verified the same way.Two non-obvious points
The flag alone would not have worked.
shell: bash -l {0}is a custom shell spec, so GitHub injects no-eo pipefail— it only does that for the bareshell: bashshorthand. The step's trailingmkdir/cpbecome its exit code, and--keep-goingguarantees the.ipynbfiles exist forcpto succeed on, so a failing build still reports green. This was caught by Copilot review on zh-cn#185 and is the reasonset -eo pipefailis here. Verified:bash -l -c 'false; echo hi'exits 0.The HTML artifact rename is required, not cosmetic.
upload-artifactruns withoverwrite: false, so adding a second upload under the existingexecution-reportsname would collide whenever both steps fail in the same run.Deliberately unchanged
ci-cn.ymlhas no sphinx-tojupyter step, and its HTML build is a single command — so its exit code is already the build's and it fails correctly. That is precisely why it, and notci.yml, caught theprob_distbreak. Itsexecution-reportsartifact name is also left alone: it is the only upload in that workflow, so there is nothing for it to collide with.The PDF/LaTeX build in
ci.ymlstays commented out. CJK under LaTeX is hard (see #25), so this looks like a considered choice rather than an oversight and needs a human decision, not mechanical alignment.Expected result
Unlike the equivalent PR next door, this one should go green: a cold build of
mainpassed 20 minutes ago (run 29720229050) — the first cold verification of this edition since 2026-07-16, which predates the 41-PR resync wave. So the corpus is known-clean going in.Verification
ci.yml13 steps,cache.yml7,ci-cn.yml9.set -eo pipefailbehaviour confirmed empirically.Related: QuantEcon/meta#340 — note the amendment recorded there, that the same masking is still live in
lecture-python.mystandlecture-python-programming, the two repos that issue treats as already aligned.🤖 Generated with Claude Code