Skip to content

ci+scripts: address review nits from #170#174

Merged
MilagrosMarin merged 1 commit into
mainfrom
fix/notebook-tooling-followups
May 21, 2026
Merged

ci+scripts: address review nits from #170#174
MilagrosMarin merged 1 commit into
mainfrom
fix/notebook-tooling-followups

Conversation

@dimitri-yatsenko
Copy link
Copy Markdown
Member

Summary

Follow-ups to @MilagrosMarin's review on #170. Each of the four nits she flagged is now addressed in one self-contained PR — no notebook re-execution involved.

# Nit (from #170 review) Fix here
1 check_notebook_versions.py not in CI New .github/workflows/check-notebooks.yml runs the check on PRs that touch notebooks, mkdocs.yaml, or the script
2 Pre-cache failure swallowed silently Warnings now go to sys.stderr (visible in CI logs) without escalating to a non-zero exit — a transient gitlab.com hiccup shouldn't block the whole run
3 skimage imported at module-execute time import skimage.data wrapped in try/except; script degrades gracefully when scikit-image isn't installed
4 had_banner short-circuits after first stale match Drop the inner break; collect stale versions per notebook in a set so multiple stale banners are all reported, deduped

Design note on #1

Milagros suggested wiring into .github/workflows/development.yml, but that workflow only triggers on push: main (it's the gh-pages deploy). Adding the check there would fire post-merge and never block a PR. A separate pull_request-triggered workflow that just runs the script (no Docker, no datajoint-python checkout) gives the intended fail-the-PR behavior in seconds.

Test plan

  • CI: this PR itself should run the new Check notebooks workflow once green (since .github/workflows/check-notebooks.yml is in its own paths filter)
  • Local: python scripts/check_notebook_versions.pyOK: 22 notebook(s) with banners are on DataJoint 2.2.x (verified)
  • Local: python scripts/execute_notebooks.py --help works without scikit-image installed (verified — argparse exits before import anyway, and import is now guarded)
  • Manual smoke: temporarily bump a notebook's banner to 2.0.x, run the check, confirm it's reported (not just the first match) — exits 1

Follow-ups from Milagros' review of #170:

- Add `.github/workflows/check-notebooks.yml` so
  `check_notebook_versions.py` runs on PRs that touch notebooks,
  `mkdocs.yaml`, or the script itself. Stale connection banners now fail
  the PR instead of relying on a contributor running the check manually.
  Kept as a separate lightweight workflow rather than wiring into
  `development.yml`, which only triggers on push to main (deploy).

- `execute_notebooks.py`: route pre-cache warnings to stderr so a
  transient gitlab.com hiccup is visible in CI logs without silently
  re-introducing "Downloading file ..." chatter on the next refresh.
  Wrap the `skimage.data` import in try/except so the script degrades
  gracefully (and prints to stderr) when scikit-image isn't installed.

- `check_notebook_versions.py`: drop the inner `break` so a notebook
  with multiple stale banners reports all of them, deduped via a set.
Copy link
Copy Markdown
Collaborator

@MilagrosMarin MilagrosMarin left a comment

Choose a reason for hiding this comment

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

Thanks @dimitri-yatsenko! All four follow-ups verified end-to-end:

✅ CI workflow triggers on pull_request with a tight path filter — your design-note pushback is right, development.yml only fires post-merge on gh-pages deploy.
✅ Pre-cache warnings now visible via sys.stderr; not escalating to non-zero exit is a reasonable trade-off.
python scripts/execute_notebooks.py --help works locally without scikit-image installed.
✅ Multiple stale banners are now all reported, sorted, deduped — verified with a 2-banner injection test.

Also confirmed the positive case after #171: OK: 22 notebook(s) with banners are on DataJoint 2.2.x, exit 0.

Approving — clean follow-up.

@MilagrosMarin MilagrosMarin merged commit 3820b0b into main May 21, 2026
3 checks passed
@dimitri-yatsenko dimitri-yatsenko deleted the fix/notebook-tooling-followups branch May 21, 2026 16:28
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.

2 participants