Skip to content

Reorganize CI to separate upstream-dev signal from PR gating#700

Merged
h-mayorquin merged 7 commits into
devfrom
improve_CI
May 19, 2026
Merged

Reorganize CI to separate upstream-dev signal from PR gating#700
h-mayorquin merged 7 commits into
devfrom
improve_CI

Conversation

@h-mayorquin

Copy link
Copy Markdown
Collaborator

The PR-gating workflow currently runs three jobs that depend on upstream development branches. Before describing the change, here is what each of those three jobs is for:

Workflow file Job name What it does
pynwb-dev-tests.yml test-pynwb-dev Runs nwbinspector's own test suite against the pynwb dev branch (installed from GitHub). Catches upstream pynwb changes that would break nwbinspector.
dandi-dev.yml test-dandi-dev Clones dandi-cli master and runs DANDI's own test suite against the current nwbinspector, with DANDI_TESTS_NONETWORK=1 so tests requiring network are skipped. Catches local-only DANDI integration breakage.
dandi-dev-live-services.yml test-dandi-dev-live Same as test-dandi-dev but with network enabled, so the tests can hit dandiarchive.org, the staging sandbox, and S3. Catches integration breakage that only surfaces against live infrastructure.

When any of those three upstream branches break for reasons unrelated to this repository, every nwbinspector PR turns red, which blocks review on changes that have nothing to do with the upstream churn. This PR adopts the three-tier CI pattern I have been using in neuroconv: deploy-tests.yml runs only stable jobs and gates PRs, dailies.yml runs the same stable jobs on a daily cron, and a new dev-dailies.yml runs the three upstream-dev jobs on a daily cron. Maintainers still receive notifications about upstream failures via daily email, but those failures no longer add noise to every unrelated PR's CI signal. In other words, the split separates upstream churn from day-to-day development while still monitoring it.

Workflow Before After
deploy-tests.yml (PR gating) run-tests, run-streaming-tests, run-read-nwbfile-tests, test-dandi-latest, test-pynwb-dev, test-dandi-dev, test-dandi-dev-live run-tests, run-streaming-tests, run-read-nwbfile-tests, test-dandi-latest
dailies.yml (daily cron) run-daily-tests, run-daily-doc-link-checks, test-dandi-latest, test-dandi-dev, test-dandi-dev-live run-daily-tests, run-daily-doc-link-checks, test-dandi-latest
dev-dailies.yml (new daily cron) does not exist test-pynwb-dev, test-dandi-dev, test-dandi-dev-live

The daily failure emails are now split into per-workflow notification jobs, each with a subject line that names the failing workflow ("NWB Inspector Daily Failure - pynwb dev branch", "NWB Inspector Daily Failure - DANDI dev branch (offline)", etc.). Previously a single generic email fired if any daily job failed, so the recipient had to open the Actions tab to see which one. The per-workflow pattern makes the email itself actionable.

The Python and OS version matrices for testing.yml are now centralized into all_python_versions.txt and all_os_versions.txt, loaded by a load_python_and_os_versions job that exposes them as workflow outputs. Bumping the supported Python or OS sweep is now a one-file edit. The streaming, read-nwbfile, and DANDI-release workflows intentionally use different matrices (skipping macOS with a TODO comment, or pinning to a specific Python) so I have left them on their hardcoded matrices to avoid silently re-enabling broken combinations.

I also removed Steph from the failure email recipient list since she is no longer on the team.

@h-mayorquin

h-mayorquin commented May 14, 2026

Copy link
Copy Markdown
Collaborator Author

While I am at it I decided to fix some failures of the CI. This and #681 (the DynamicTableRegion HDMF 5 update) should then fix the CI. The fixes are:

1. read_nwbfile no longer fails the ROS3 streaming test. h5py recently tightened the ROS3 driver to require an explicit aws_region. Rather than surface each new upstream kwarg as a named argument, I added a generic keyword-only backend_kwargs: dict on read_nwbfile that forwards to NWBHDF5IO on the streaming paths:

read_nwbfile(url, method="ros3", backend_kwargs={"aws_region": "us-east-1"})

This keeps the helper a thin pass-through, and future upstream kwarg requirements do not need an API change here.

2. Windows run-read-nwbfile-tests no longer fails with ModuleNotFoundError: No module named 'h5py'. The previous pip uninstall h5pyconda install h5py pattern silently failed on Windows because the conda activation did not propagate across steps. The PyPI h5py wheel still does not bundle the ROS3 driver (h5py#1884), so conda is still necessary. Switched to PyNWB's pattern: a dedicated environment-ros3.yml and setup-miniconda with activate-environment + environment-file + auto-activate-base: false. PyNWB uses this same setup on its windows-latest ROS3 job, which has been green for some time.

While in there I also did a small API consolidation: deprecated read_nwbfile_and_io (removal 2026-11-13) in favor of just read_nwbfile. The IO object remains accessible from the returned NWBFile via nwbfile.get_read_io(), so no functionality is lost. Internal callers switched to a private _read_nwbfile_and_io. git log shows the duality was originally a mypy workaround for a Union return type in 2024, not a deliberate API choice. This change restores the 2023 single-function design intent.

@codecov-commenter

Copy link
Copy Markdown

Codecov Report

❌ Patch coverage is 70.00000% with 3 lines in your changes missing coverage. Please review.
✅ Project coverage is 82.81%. Comparing base (56c728c) to head (301ebed).
⚠️ Report is 1 commits behind head on dev.

Files with missing lines Patch % Lines
src/nwbinspector/tools/_read_nwbfile.py 66.66% 3 Missing ⚠️
Additional details and impacted files

Impacted file tree graph

@@            Coverage Diff             @@
##              dev     #700      +/-   ##
==========================================
+ Coverage   80.01%   82.81%   +2.80%     
==========================================
  Files          48       48              
  Lines        1886     1897      +11     
==========================================
+ Hits         1509     1571      +62     
+ Misses        377      326      -51     
Files with missing lines Coverage Δ
src/nwbinspector/_nwb_inspection.py 85.90% <100.00%> (+2.45%) ⬆️
src/nwbinspector/tools/_read_nwbfile.py 79.36% <66.66%> (+15.98%) ⬆️

... and 3 files with indirect coverage changes

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

@h-mayorquin h-mayorquin requested a review from rly May 14, 2026 03:43

@rly rly left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

This all looks good to me. Thanks @h-mayorquin !

@h-mayorquin h-mayorquin merged commit 04e099d into dev May 19, 2026
25 checks passed
@h-mayorquin h-mayorquin deleted the improve_CI branch May 19, 2026 17:37
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.

3 participants