From 3c6fcb78f2665cc061ff61fcb3a0d95d78054fc1 Mon Sep 17 00:00:00 2001 From: Frank Buckley Date: Sun, 26 Jul 2026 16:59:48 +0100 Subject: [PATCH] fix(scripts): skip hidden output-transaction dirs when resolving fit targets MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit `_subdirs()` returned every subdirectory of the statistical-models output root, including the hidden staging siblings that `StatisticalFitContext.reset_output_dir` creates. Each run is staged in `.-.staging-XXXXXXXX` and promoted only after every fit stage succeeds, so a dotted directory is either a run in progress or an abandoned one. Publication also leaves a hidden `..backup-XXXX` sibling until it ages out — a *previous* fit, not the current one. Walking into those is wrong in three different ways depending on the caller: - `regenerate_key_findings.py` and `regenerate_itt_contrast_figures.py` write artefacts that are about to be discarded, or race a live fit. The contrast-figure script gates only on `config.json` and `trace.nc` existing, both of which a running fit writes well before it finishes. - `upload.py` is worse: `all` / `stat` push each resolved directory to blob storage under its own name, so an unfiltered walk publishes the half-written artefacts of a fit that was never accepted, under a label no report or comparison refers to, and records those URLs in `uploaded_urls.txt`. - `regenerate_mediation_calibration.py` is not actually reachable today — every branch of its `resolve_targets` filters on `d.name.startswith(f"{mid}-")`, which a leading-dot name fails. The filter is added anyway so the rule is a property of the helper rather than an accident of its caller. Each `_subdirs` now excludes dotted names and carries a docstring giving the caller-specific reason, matching the fix already made in `regenerate_psense.py`. The new test pins the exclusion for all four scripts: the staging case end to end through `resolve_targets`, the `.backup-` case, the single-model-id form, and `_subdirs` directly — the last because a script whose own id filter happens to exclude dotted names would otherwise pass without carrying the rule. Every assertion was verified to fail against the pre-fix code. Relates to #394 Co-Authored-By: Claude Opus 5 --- scripts/regenerate_itt_contrast_figures.py | 11 +- scripts/regenerate_key_findings.py | 11 +- scripts/regenerate_mediation_calibration.py | 12 +- scripts/upload.py | 12 +- tests/test_regenerate_key_findings.py | 165 ++++++++++++++++++++ 5 files changed, 207 insertions(+), 4 deletions(-) create mode 100644 tests/test_regenerate_key_findings.py diff --git a/scripts/regenerate_itt_contrast_figures.py b/scripts/regenerate_itt_contrast_figures.py index ccf9d653..b6387e18 100644 --- a/scripts/regenerate_itt_contrast_figures.py +++ b/scripts/regenerate_itt_contrast_figures.py @@ -77,9 +77,18 @@ def _subdirs(root: Path) -> list[Path]: + """Published fit directories, excluding in-flight output transactions. + + ``StatisticalFitContext.reset_output_dir`` stages each run in a *hidden* sibling + (``.-.staging-XXXX``) and promotes it only on success, so a dotted + directory is either a run in progress or an abandoned one. Redrawing figures into + it writes artefacts that are about to be discarded — or, worse, races a live fit. + """ if not root.is_dir(): return [] - return sorted(d for d in root.iterdir() if d.is_dir()) + return sorted( + d for d in root.iterdir() if d.is_dir() and not d.name.startswith(".") + ) def resolve_targets(target: str) -> list[Path]: diff --git a/scripts/regenerate_key_findings.py b/scripts/regenerate_key_findings.py index 44314b1e..2e795aa4 100644 --- a/scripts/regenerate_key_findings.py +++ b/scripts/regenerate_key_findings.py @@ -32,9 +32,18 @@ def _subdirs(root: Path) -> list[Path]: + """Published fit directories, excluding in-flight output transactions. + + ``StatisticalFitContext.reset_output_dir`` stages each run in a *hidden* sibling + (``.-.staging-XXXX``) and promotes it only on success, so a dotted + directory is either a run in progress or an abandoned one. Regenerating into it + writes artefacts that are about to be discarded — or, worse, races a live fit. + """ if not root.is_dir(): return [] - return sorted(d for d in root.iterdir() if d.is_dir()) + return sorted( + d for d in root.iterdir() if d.is_dir() and not d.name.startswith(".") + ) def resolve_targets(target: str) -> list[Path]: diff --git a/scripts/regenerate_mediation_calibration.py b/scripts/regenerate_mediation_calibration.py index 7d4f213a..0d27e9d0 100644 --- a/scripts/regenerate_mediation_calibration.py +++ b/scripts/regenerate_mediation_calibration.py @@ -44,9 +44,19 @@ def _subdirs(root: Path) -> list[Path]: + """Published fit directories, excluding in-flight output transactions. + + ``StatisticalFitContext.reset_output_dir`` stages each run in a *hidden* sibling + (``.-.staging-XXXX``) and promotes it only on success, so a dotted + directory is either a run in progress or an abandoned one. The model-id prefix + filter in ``resolve_targets`` already excludes those names incidentally; keeping + the rule here makes it a property of the helper rather than of its caller. + """ if not root.is_dir(): return [] - return sorted(d for d in root.iterdir() if d.is_dir()) + return sorted( + d for d in root.iterdir() if d.is_dir() and not d.name.startswith(".") + ) def resolve_targets(target: str) -> list[Path]: diff --git a/scripts/upload.py b/scripts/upload.py index 39f7e57b..7498e96d 100644 --- a/scripts/upload.py +++ b/scripts/upload.py @@ -31,9 +31,19 @@ def _subdirs(root: Path) -> list[Path]: + """Published fit directories, excluding in-flight output transactions. + + ``StatisticalFitContext.reset_output_dir`` stages each run in a *hidden* sibling + (``.-.staging-XXXX``) and promotes it only on success, so a dotted + directory is either a run in progress or an abandoned one. Uploading one would + publish the half-written artefacts of a fit that was never accepted, under a + label no report or comparison refers to. + """ if not root.is_dir(): return [] - return sorted(d for d in root.iterdir() if d.is_dir()) + return sorted( + d for d in root.iterdir() if d.is_dir() and not d.name.startswith(".") + ) def resolve_targets(target: str) -> list[tuple[str, Path]]: diff --git a/tests/test_regenerate_key_findings.py b/tests/test_regenerate_key_findings.py new file mode 100644 index 00000000..8703a0a5 --- /dev/null +++ b/tests/test_regenerate_key_findings.py @@ -0,0 +1,165 @@ +# Copyright (c) 2026 Down Syndrome Education International and contributors +# SPDX-License-Identifier: AGPL-3.0-or-later + +"""Target resolution for the scripts that walk the statistical-model output root. + +The regenerate backfills re-run a fit-time generator over already-published output +directories, and ``upload.py`` publishes them, so the one thing each must get right +before touching a file is *which* directories are published. +``StatisticalFitContext.reset_output_dir`` stages every run in a hidden sibling and +promotes it only after the last stage succeeds, so the output root routinely holds +dotted directories that are either in flight or abandoned. Walking into one writes +artefacts that are about to be discarded, races a live fit, or — for the upload +path — publishes the half-written output of a run that was never accepted. Hence +the shared exclusion pinned here for every script that walks that root. + +Scripts aren't on the import path in this repo, so the modules are loaded by file +path. ``regenerate_psense.py`` carries the same rule and pins it in its own tests. +""" + +from __future__ import annotations + +import importlib.util +from pathlib import Path + +import pytest + +_SCRIPTS = Path(__file__).resolve().parent.parent / "scripts" + +# The published directory each script would accept, paired with the staging sibling +# it must not: the mediation backfill narrows to the model ids it can calibrate, so +# a generic name would be filtered out for reasons unrelated to what is under test. +_WALKERS = [ + ("regenerate_key_findings", "lrp-rli-itt-010-reporting"), + ("regenerate_itt_contrast_figures", "lrp-rli-itt-010-reporting"), + ("regenerate_mediation_calibration", "lrp-rli-med-059-reporting"), + ("upload", "lrp-rli-itt-010-reporting"), +] + +# ``upload.py`` resolves to (label, path) pairs across both output roots, so it is +# covered by its own end-to-end test below rather than this shared signature. +_BACKFILLS = [entry for entry in _WALKERS if entry[0] != "upload"] + + +def _load(name: str): + spec = importlib.util.spec_from_file_location(name, _SCRIPTS / f"{name}.py") + assert spec is not None and spec.loader is not None + module = importlib.util.module_from_spec(spec) + spec.loader.exec_module(module) + return module + + +@pytest.fixture(scope="module") +def regen(): + return _load("regenerate_key_findings") + + +def test_targets_exclude_in_flight_output_transactions(regen, tmp_path, monkeypatch): + """Fits stage into a hidden ``.-.staging-XXXX`` sibling and are + promoted only on success. Regenerating into one writes artefacts that are about + to be discarded, or races a live fit.""" + from language_reading_predictors import paths as _paths + + root = tmp_path / "models" + (root / "lrp-rli-itt-010-reporting").mkdir(parents=True) + (root / ".lrp-rli-itt-010-reporting.staging-9hbfh_9x").mkdir(parents=True) + monkeypatch.setattr(_paths, "stat_models_dir", lambda: root) + + names = [d.name for d in regen.resolve_targets("all")] + assert names == ["lrp-rli-itt-010-reporting"] + + +def test_a_model_id_target_excludes_its_own_staging_directory( + regen, tmp_path, monkeypatch +): + """The single-model form matches on the ``-`` prefix, so it must not be + satisfied by a staging directory carrying that id — the run whose artefacts it + holds has not been published, and may still be writing them.""" + from language_reading_predictors import paths as _paths + + root = tmp_path / "models" + (root / "lrp-rli-itt-010-dev").mkdir(parents=True) + (root / "lrp-rli-itt-010-reporting").mkdir(parents=True) + (root / ".lrp-rli-itt-010-reporting.staging-9hbfh_9x").mkdir(parents=True) + (root / "lrp-rli-itt-011-reporting").mkdir(parents=True) + monkeypatch.setattr(_paths, "stat_models_dir", lambda: root) + + names = [d.name for d in regen.resolve_targets("lrp-rli-itt-010")] + assert names == ["lrp-rli-itt-010-dev", "lrp-rli-itt-010-reporting"] + + +def test_backup_directories_are_excluded_too(regen, tmp_path, monkeypatch): + """Publication keeps the superseded fit as a hidden ``..backup-XXXX`` until + it ages out. It is a *previous* fit, not the current one, so regenerating into it + would refresh artefacts no report reads.""" + from language_reading_predictors import paths as _paths + + root = tmp_path / "models" + (root / "lrp-rli-itt-010-reporting").mkdir(parents=True) + (root / ".lrp-rli-itt-010-reporting.backup-3kd0waz1").mkdir(parents=True) + monkeypatch.setattr(_paths, "stat_models_dir", lambda: root) + + names = [d.name for d in regen.resolve_targets("all")] + assert names == ["lrp-rli-itt-010-reporting"] + + +@pytest.mark.parametrize(("module_name", "published"), _WALKERS) +def test_every_output_root_walker_skips_hidden_transactions( + module_name, published, tmp_path +): + """The same output root is walked by several scripts, and the exclusion has to + hold in each — one that keeps the raw ``iterdir`` reintroduces the hazard on its + own. ``_subdirs`` is asserted directly because a script whose own id filter + happens to exclude dotted names would otherwise pass without carrying the rule.""" + module = _load(module_name) + root = tmp_path / "models" + (root / published).mkdir(parents=True) + (root / f".{published}.staging-9hbfh_9x").mkdir(parents=True) + + assert [d.name for d in module._subdirs(root)] == [published] + + +@pytest.mark.parametrize(("module_name", "published"), _BACKFILLS) +def test_backfill_targets_resolve_to_published_dirs_only( + module_name, published, tmp_path, monkeypatch +): + from language_reading_predictors import paths as _paths + + module = _load(module_name) + root = tmp_path / "models" + (root / published).mkdir(parents=True) + (root / f".{published}.staging-9hbfh_9x").mkdir(parents=True) + monkeypatch.setattr(_paths, "stat_models_dir", lambda: root) + + assert [d.name for d in module.resolve_targets("all")] == [published] + + +def test_upload_targets_exclude_in_flight_output_transactions(tmp_path, monkeypatch): + """``upload.py`` pushes each resolved directory to blob storage under its own + name, so an unfiltered walk publishes the half-written artefacts of a run that + was never promoted — under a label no report or comparison refers to.""" + from language_reading_predictors import paths as _paths + + upload = _load("upload") + stat_root = tmp_path / "statistical_models" / "models" + gb_root = tmp_path / "models" + (stat_root / "lrp-rli-itt-010-reporting").mkdir(parents=True) + (stat_root / ".lrp-rli-itt-010-reporting.staging-9hbfh_9x").mkdir(parents=True) + gb_root.mkdir(parents=True) + monkeypatch.setattr(_paths, "stat_models_dir", lambda: stat_root) + monkeypatch.setattr(_paths, "gb_models_dir", lambda: gb_root) + + assert [label for label, _ in upload.resolve_targets("all")] == [ + "lrp-rli-itt-010-reporting" + ] + assert [label for label, _ in upload.resolve_targets("lrp-rli-itt-010")] == [ + "lrp-rli-itt-010-reporting" + ] + + +@pytest.mark.parametrize(("module_name", "published"), _WALKERS) +def test_missing_output_root_resolves_to_no_targets(module_name, published, tmp_path): + """A run against an output root that was never created (a fresh checkout, or a + scratch disk that has been torn down) reports no targets rather than raising.""" + module = _load(module_name) + assert module._subdirs(tmp_path / "absent") == []