Streamline test suite with shared fixtures, markers, and per-notebook tests - #404
Streamline test suite with shared fixtures, markers, and per-notebook tests#404animmosmith wants to merge 7 commits into
Conversation
pipeline-holo.ipynb (an ordinary, non-training notebook) completed in ~2 minutes on Ubuntu and Windows in PR #404's CI run, but exceeded the 600s pytest-timeout default on macOS. The traceback showed it stuck waiting on the Jupyter kernel's socket, consistent with nbconvert/Jupyter-kernel execution being disproportionately slow on macOS CI runners specifically, not the underlying computation taking longer. Applies a 1800s override to all tests in this module rather than guessing a training-specific number, since any notebook could hit the same platform-specific slowdown.
Adds pytest-rerunfailures and applies @pytest.mark.flaky(reruns=2) to test_notebooks.py, scoped to that module only. The macOS timeout seen on PR #404 looks like CI infra flakiness (a stuck Jupyter kernel socket) rather than a reproducible bug, so a couple of automatic retries is a reasonable complement to the longer timeout. Not applied suite-wide, since retrying elsewhere could mask a real, reproducible failure as flakiness.
|
Added `.github/workflows/scheduled-training-check.yml` to this PR. What it does: runs the tests tagged `@pytest.mark.training` (currently just the DINOv2 classifier training notebook) on a weekly schedule (Mondays 06:00 UTC), plus on-demand via the "Run workflow" button (`workflow_dispatch`). These tests are intentionally excluded from the regular per-PR `build-and-test.yml` run, since they involve a real, uncapped multi-epoch training loop rather than a check of PyOPIA's own correctness — running them on every PR would be slow and mostly irrelevant to the change being reviewed. Why schedule it at all rather than just excluding it: without this, the only way we'd find out that notebook broke (e.g. an upstream `torch`/`timm` release changing the DINOv2 backbone's behavior) is a user stumbling into it and reporting back. We actually found a live example of exactly that risk while building this PR — `pipeline_step_by_step.ipynb` had a stale key reference that had silently gone unnoticed for roughly two years, since nothing ever re-ran it. A weekly check catches that kind of drift automatically instead. One thing to note: GitHub only evaluates the `schedule` cron trigger from the copy of the workflow file on the default branch (`main`) — so the weekly run won't actually start firing until this PR merges. Until then it can still be triggered manually from the Actions tab. |
In-place shape mutation (y.shape = ...) is deprecated as of NumPy 2.5. y is a freshly-created array here with no other references, so reassigning via reshape() is behavior-identical. Found via a DeprecationWarning in #404's CI.
There was a problem hiding this comment.
Given that DINOv2 was originally released under CC BY-NC 4.0 (non-commercial only) before being relicensed to Apache 2.0, I think we should be cautious about assuming the licences to pre-trained weights are stable. This workflow would quietly keep pulling and using new weights (potentially with a new licence) every Monday without anyone verifying.
Maybe we could consider if we can lock the weights version that gets downloaded by the DINOv2 notebook in the standard tests (but that would still remove the need for a scheduled test, with the benefit of more defined control over exactly which weights are used), and we would not capture the 'drift' effect from updated weights without it being an explicit PR where we update to newer weights. Perhaps that's anyway more robust: if it is possible to lock the weights version in that notebook downloads?
@nepstad any thoughts?
There was a problem hiding this comment.
Good point on the licensing risk. I've prepared the pin as a ready branch: 404-pin-dinov2-weights-version (403-streamline-test-suite-with-shared-fixtures...404-pin-dinov2-weights-version) - pins timm to 1.0.28 in the notebook's install cell, with a comment explaining why, so bumping it becomes a deliberate, reviewed change rather than an implicit weekly re-download. Verified the pinned version installs cleanly and loads the real backbone.
Haven't touched the scheduled workflow itself yet, since that decision (keep it for catching our own code going stale vs. drop it entirely) seems like it should follow from what you and @nepstad land on here - happy to fold this in once there's agreement, or push it as-is if that's easier to review directly.
Addresses #403 and folds in #237. - Add pytest markers (`slow`, `training`) registered in pyproject.toml, so `pytest -m "not slow"` gives a fast local loop and `pytest -m "not training"` (now used in CI) excludes tests that train a model from scratch. - Consolidate real-data download fixtures (example image, classifier model, training database, hologram) into session-scoped fixtures in pyopia/tests/conftest.py, shared across test_pipeline.py, test_classify.py, and test_cli.py instead of each downloading its own copy. The CLI-specific fixture copies into its own directory before duplicating a file for its chunking-minimum workaround, so it can't leak that duplicate into the fixtures other test files share. - Standardize on pytest's tmp_path/tmp_path_factory instead of manual tempfile.TemporaryDirectory() throughout. - Remove a flaky hardcoded wall-clock timing assertion in test_classify.py, and a stray `model/` directory it was leaving in the repo root. - Rewrite test_notebooks.py: each notebook is now its own parametrized test (test_notebook[<name>.ipynb]) instead of one monolithic function covering all of them, with markers reflecting real cost: - `slow`: real network/pipeline notebooks (existing + newly added docs/notebooks coverage per #237: montaging, stats, exploring_pipeline_data, pipeline_step_by_step, background_correction) - unmarked: cli.ipynb (no network dependency) and markdown-only notebooks (toml_config, processing_raw_data, big_datasets) - `slow` + `training`: the DINOv2 classifier training notebook, which runs 30 real training epochs with no CI-mode shortcut and is now excluded from routine CI entirely (`-m "not training"`) - docs/notebooks/STATSnc.ipynb intentionally not included: it loads a pre-existing stats file no notebook produces at that path in isolation Found and fixed two real, pre-existing bugs this new coverage caught: docs/notebooks/config.toml referenced a non-existent 'keras_model.h5', and pipeline_step_by_step.ipynb referenced stale 'imc'/default segment_source and roi_source keys that no longer match the current pipeline/ImagePrep implementation. - Add a Testing section to README.md documenting the markers, shared fixtures, and notebook CI policy. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
pipeline-holo.ipynb (an ordinary, non-training notebook) completed in ~2 minutes on Ubuntu and Windows in PR #404's CI run, but exceeded the 600s pytest-timeout default on macOS. The traceback showed it stuck waiting on the Jupyter kernel's socket, consistent with nbconvert/Jupyter-kernel execution being disproportionately slow on macOS CI runners specifically, not the underlying computation taking longer. Applies a 1800s override to all tests in this module rather than guessing a training-specific number, since any notebook could hit the same platform-specific slowdown.
Adds pytest-rerunfailures and applies @pytest.mark.flaky(reruns=2) to test_notebooks.py, scoped to that module only. The macOS timeout seen on PR #404 looks like CI infra flakiness (a stuck Jupyter kernel socket) rather than a reproducible bug, so a couple of automatic retries is a reasonable complement to the longer timeout. Not applied suite-wide, since retrying elsewhere could mask a real, reproducible failure as flakiness.
Runs weekly (plus manual workflow_dispatch) against tests tagged @pytest.mark.training - currently the DINOv2 classifier training notebook - which are excluded from routine per-PR CI since they run a real, uncapped multi-epoch training loop rather than checking PyOPIA's own correctness. This catches drift (e.g. an upstream torch/timm release change) between PRs instead of waiting for a user to report it. Note: the schedule only activates once this file is on the default branch; until this PR merges it can still be run manually via workflow_dispatch.
In-place shape mutation (y.shape = ...) is deprecated as of NumPy 2.5. y is a freshly-created array here with no other references, so reassigning via reshape() is behavior-identical. Found via a DeprecationWarning in #404's CI.
f42f79e to
a4c1f8e
Compare
Prompted by Emlyn's review comment on this PR about DINOv2's licensing history. Adds pip-licenses as a dev dependency and a CI job that fails the build if any dependency's license matches GPL/AGPL/LGPL/Commons Clause/Non-Commercial/SSPL/Business Source License. docutils needs an explicit exception since its PyPI metadata lists a compound license string that includes GPL from a couple of bundled non-code files, even though the actual code is BSD. Also declares PyOPIA's own license in packaging metadata (license = "BSD-3-Clause"), which was previously unset and reported as UNKNOWN by license-scanning tools. Closes #414
Summary
Fixes #403. Folds in #237.
Built on top of #394 (assumed merged) — this branch includes its
pyopia/tests/conftest.pyandtest_cli.py.slowandtrainingpytest markers inpyproject.toml.pytest -m "not slow"gives a fast local loop; CI now runs with-m "not training"so model-training notebooks never run in routine CI.conftest.py, shared acrosstest_pipeline.py,test_classify.py, andtest_cli.pyinstead of each downloading its own copy. The CLI-specific fixture copies into its own private directory before duplicating a file for its chunking-minimum workaround, so that duplicate can't leak into the fixtures other test files share.tmp_patheverywhere: replaces manualtempfile.TemporaryDirectory()with pytest's built-intmp_path/tmp_path_factorythroughout.test_classify.py: removes a hardcoded wall-clock timing assertion (we hit this exact flake earlier bumping keras) and a straymodel/directory it left in the repo root.test_notebooks.pyrewrite: each notebook is now its own parametrized test (test_notebook[<name>.ipynb]) instead of one monolithic function, with markers reflecting real cost:slow: real network/pipeline notebooks — existing ones plus new docs/notebooks coverage for Add docs/notebooks to test_notebooks #237 (montaging,stats,exploring_pipeline_data,pipeline_step_by_step,background_correction)cli.ipynb(no network dependency) and markdown-only notebooks (toml_config,processing_raw_data,big_datasets)slow+training: the DINOv2 classifier training notebook (30 real training epochs, no CI-mode shortcut) — now excluded from routine CI entirelydocs/notebooks/STATSnc.ipynbloads a pre-existingtest-STATS.ncfile that no notebook produces at that path in isolation; it's designed to be read against a user's own prior processing run, not executed standalone.docs/notebooks/config.tomlreferenced a non-existentkeras_model.h5, andpipeline_step_by_step.ipynbreferenced a staleimckey and relied on defaultsegment_source/roi_sourcevalues that no longer match the currentImagePrep/Segment/CalculateStatsimplementation.Test plan
pytest -m "not slow"— 34 passed in ~10spytest -m slowontest_pipeline.py/test_classify.py/test_cli.py— 10 passed in ~2 min, confirming shared fixtures work correctly across files-m trainingselects exactly the DINOv2 notebook, and-m "not training"(now CI's default) excludes only that one🤖 Generated with Claude Code