Summer 26 Housekeeping: tests, docs, CI, and bug fixes - #430
Open
animmosmith wants to merge 19 commits into
Open
Summer 26 Housekeeping: tests, docs, CI, and bug fixes#430animmosmith wants to merge 19 commits into
animmosmith wants to merge 19 commits into
Conversation
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.
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.
Fixes #405. get_j() used np.log's `where=` to skip non-positive number_distribution bins (to avoid log(0)/log(negative)), added in #314 specifically to "handle zero values in number_distribution[ind]". But `where=` without a matching `out=` leaves the skipped positions as uninitialized memory rather than excluding them, and that array was fed straight into np.polyfit for the Junge slope fit - a real, user-facing value returned by nc_vc_from_stats() and stored in image_stats['junge']. Excludes non-positive bins from the fit outright (properly completing the intent of #314's fix), and returns NaN when fewer than two bins remain in the 150-300um fitting range (e.g. an image with no particles in that range), rather than crashing or fitting garbage - matching how other undefined/no-data cases are already handled elsewhere in the codebase. The 150-300um range itself is untouched: it's a deliberate choice (per the existing comment) to fit the Junge slope only where LISST-100 and SilCam data are considered mutually valid, not an arbitrary restriction.
…s clearly Fixes #331 and #350. - process/process_realtime/merge_mfdata/process_file_list now all route logging through a multiprocessing.Queue drained by a single QueueListener, rather than each process/chunk opening its own FileHandler onto the same log file. Worker processes are named chunk-{c} so the merged log identifies which chunk each line came from. - AuxillaryData.load_auxillary_data failures are now wrapped in a new AuxillaryDataError naming the file and underlying cause, instead of being silently swallowed (or, for errors load_auxillary_data could actually raise, not caught at all). process_file_list stops the rest of that chunk on this error instead of retrying every remaining image against the same broken file. - Promoted the per-image exception traceback from DEBUG to ERROR so it's visible by default rather than only with verbose logging enabled.
Fixes #409. - "Build docs locally" was broken outright: the documented sphinx-apidoc command referenced docs/source and docs/build, neither of which exist. Verified against .readthedocs.yaml and confirmed the correct, working command is a single sphinx-build call (sphinx.ext.autosummary, already configured in docs/conf.py, generates the API docs automatically). - Fixed a broken markdown link (brackets/parens were swapped) and a Windows-style backslash path inside a bash command. - Fixed typos (regester, monatge, complie, awarenes, plesant, aknowledged) and inconsistent "PyOpia"/"PyOPIA" naming throughout. - Replaced the "Development targets" section, which had become stale and in places directly contradicted the current architecture (e.g. it said "no use of settings/config files," but the whole pipeline is config-driven), with a short, accurate "Design principles" section. - Clarified step 5's montage.png as an output file, not a command, and noted that init-project --example-data both downloads images and generates config.toml, so it doesn't appear to come from nowhere. - Expanded the Docker rationale beyond "for users who prefer not to install dependencies directly." Verified the full quick-start flow (init-project, process, merge-mfdata, make-montage) end-to-end with real example data. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
RTD now installs docs dependencies via `uv sync` using the classification extra plus a new `docs` dependency group, instead of pip-installing docs/requirements.txt. Verified with a real end-to-end `sphinx-build` using the resulting environment. Closes #399
…rrors process()/process_realtime() created the log queue listener before validating the config (missing output step, etc.), but only stopped it at the very end - so a validation error left the listener's background thread (and the multiprocessing.Queue feeding it) running forever. Wrap the listener's whole lifetime in try/finally so it's always stopped, regardless of where an exception occurs.
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
Real CI evidence: Windows CI runs on this stack were completing all tests successfully but then hanging for ~48 minutes before being killed by the 60-minute job timeout, with no further output after the test summary. Stopping the QueueListener alone isn't sufficient cleanup - the underlying multiprocessing.Queue keeps its own background feeder thread alive until explicitly closed, which per the multiprocessing docs can prevent a process from exiting cleanly. This wasn't visible in local verification (Linux/macOS) or in Ubuntu/macOS CI, but Windows' stricter process-exit/thread-cleanup semantics surfaced it as a full job timeout. Adds a shared stop_queue_logging() helper (listener.stop() + queue.close() + queue.join_thread()) used at all three call sites: process(), process_realtime(), and merge_mfdata().
README's "Build docs locally" section now syncs the docs dependency group via uv before building, matching the RTD config.
Fixes #415: print_steps() called steps_to_string(self.steps), but self.steps was never set anywhere - Pipeline only ever sets self.settings/self.stepnames in __init__. steps_to_string() also turned out to expect instantiated step objects (with a __dict__), not the raw settings dict, and is itself already marked deprecated - rather than resurrecting that, print_steps() now just pretty-prints the settings dict directly, which is simpler and shows the actually-configured arguments.
Closes #428: documents the release philosophy discussed in #425 - cut releases when work is finished and documented, rather than on a fixed schedule, and keep the Docker image in sync with each release. Closes #429: clarifies that AI coding tools are welcome as an aid, but whoever submits a PR is responsible for the code in it.
This was referenced Aug 20, 2026
Closes #420: the training notebook's split logic put every image not chosen for training into validation, which for this dataset (thousands of images, badly imbalanced across classes) made evaluation dwarf training itself - locally, a single epoch at the old defaults took ~40 minutes almost entirely on a one-time ~7,400-image validation pass. Capping validation to VAL_PER_CLASS (20, randomly sampled) instead of "everything left over" brings the full 30-epoch default run down to ~16 minutes, confirmed empirically, with no change to training itself. That's cheap enough to move the notebook from pytest.mark.training (never ran in CI) into the routine per-PR slow-notebook set, so a real, full training run now has to pass before merging - removing the scheduled-vs-pinned dilemma #420 was asking about, since there's no longer a separate schedule to maintain.
…on-torch extra CI just proved this: with the notebook's validation now bounded, it runs in routine CI alongside the rest of the notebook suite - and its own "!uv pip install torch timm scikit-learn" cell mutates the shared job venv outside pyproject.toml's constraints, silently bumping click past the <8.2.0 pin the project relies on to keep Typer working. That broke cli.ipynb's test on every OS immediately after the training notebook ran. timm and scikit-learn are now part of the classification-torch extra, next to the torch dependency that was already there, so `uv sync` resolves everything together respecting the project's real constraints. The notebook's install cell is now a no-op whenever those are already present (true for both routine CI and anyone who installed pyopia[classification-torch]) and only actually installs anything for someone running this notebook completely standalone without that extra.
Closes #429: adds AGENTS.md - operational guidance for AI coding agents (setup, testing, linting, conventions), distinct from the README's "Using AI tools" section, which is the human-facing policy statement. Also removes the training marker entirely rather than leaving it registered but unused: nothing has carried it since the DINOv2 notebook moved into the routine slow-notebook suite, and CI's `pytest -v -m "not training"` was already a no-op filter at that point. Simplifies CI's invocation to `pytest -v` accordingly.
Closes the licensing half of #420, which the earlier fix in this branch missed: DINOv2's pretrained weights started under a non-commercial license before later moving to Apache 2.0. timm was only floor-pinned (timm>=1.0.0), so uv sync (CI has no committed uv.lock) could silently resolve a newer timm release that remaps the "vit_small_patch14_dinov2.lvd142m" tag onto different weights/licensing, in both the classification-torch extra and the notebook's own standalone-install fallback. Pins both to timm==1.0.28 (the version already verified working throughout this branch's CI runs). Bumping it is now a deliberate, reviewed change rather than something that happens implicitly on every CI run.
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
1 participant
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.
Combined housekeeping PR for the Summer 26 Housekeeping milestone, following up on our meeting with Emlyn and Raymond: a single reviewable PR covering tests/docs/bugs, ahead of a new release.
Each bullet below links to the specific commit that makes that change, so you can review them individually rather than as one combined diff.
Bug fixes
52b92da)get_j()no longer feeds uninitialized memory into the Junge slope fit for zero-count bins. (ab815d2)Pipeline.print_steps()no longer references a non-existent attribute. (078d59c)6ef82c1)cb92b6f), and that themultiprocessing.Queueis fully closed after stopping the listener - prevents a Windows-specific CI hang (1aba960).Tests
slowmarker, and per-notebook parametrized tests (including docs/notebooks coverage). (8206754)f341b78) and raises their timeout, based on real flakiness/timing evidence from CI (eeeab7d).CI
VAL_PER_CLASS, randomly sampled, instead of every leftover image) - cut the full 30-epoch default run from 40+ minutes (dominated by a one-time, unbounded ~7,400-image validation pass) to a few minutes on GitHub's runners. Moves the notebook frompytest.mark.training(never ran in CI) into the routine per-PR notebook suite, so a real training run now has to pass before merging. (a75c651)timm/scikit-learndependencies into theclassification-torchextra, alongsidetorch, souv syncresolves everything together respecting the project's own constraints (e.g.click<8.2.0) instead of an ad-hoc!uv pip installmutating the shared CI job's environment - confirmed via a real CI run that this was breakingcli.ipynb's test in the same job. (444c9ae)trainingpytest marker entirely (nothing carries it once the above landed) rather than leaving dead config around; simplifies CI's invocation frompytest -v -m "not training"topytest -v. (bbb4a25)timm==1.0.28exactly (wastimm>=1.0.0) in bothpyproject.tomland the notebook's standalone-install fallback: this closes the licensing half of Decide how to handle DINOv2 weight drift: scheduled check vs. pinned version #420 that the validation-cap fix above didn't address - DINOv2's pretrained weights started under a non-commercial license before later moving to Apache 2.0, and an unpinnedtimm(nouv.lockis committed) could silently resolve a future release onto different weights/licensing terms on every CI run. (658bd20)Docs
42a9c13)68bb706)2674332)Dependencies
9772f87)Process
97bce17)97bce17) - and anAGENTS.md- agent-facing operational guidance: setup, testing, linting, conventions (bbb4a25).Test plan
flake8 pyopiacleanuv run pytest -m "not slow"- 36 passed