fix(scripts): skip hidden output-transaction dirs when resolving fit targets - #443
Merged
Merged
Conversation
…targets
`_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 `.<model-id>-<config>.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 `.<name>.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 <noreply@anthropic.com>
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
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.
Note
Drafted by a LLM-based AI tool (Claude Code/Opus 5).
What
The
_subdirs()helper shared by four scripts returned every subdirectory of the statistical-models output root, including the hidden staging siblings thatStatisticalFitContext.reset_output_dircreates. Each run is staged in.<model-id>-<config>.staging-XXXXXXXXand 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.<name>.backup-XXXXsibling until it ages out — a previous fit, not the current one.Each
_subdirsnow excludes dotted names and carries a docstring giving the caller-specific reason, matching the fix already made inregenerate_psense.py(which is separate work-in-progress and not on this branch, so its own test file covers it there).Why it matters, per caller
regenerate_key_findings.py(the script this started from) andregenerate_itt_contrast_figures.pywrite artefacts that are about to be discarded, or race a live fit. The contrast-figure script gates only onconfig.jsonandtrace.ncexisting, both of which a running fit writes well before it finishes.upload.pyis the worst case, and was not in the original scope — it turned up in a sweep for the same pattern. Itsall/statbranch pushes 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.…staging-XXXXlabel no report or comparison refers to, and records those URLs inuploaded_urls.txt.regenerate_mediation_calibration.pyis not actually reachable today: every branch of itsresolve_targetsfilters ond.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 — flagging it explicitly because it is a behavioural no-op, and it is reasonable to ask for it to be dropped.Tests
New
tests/test_regenerate_key_findings.pypins the exclusion for all four scripts: the staging case end to end throughresolve_targets, the.backup-case, the single-model-id form, and_subdirsdirectly. The direct_subdirsassertion is there because a script whose own id filter happens to exclude dotted names (mediation, above) would otherwise pass without carrying the rule.Every assertion was verified to fail against the pre-fix code before the fix was restored — 5 failures for the three regenerate scripts, 2 more for
upload.py.Reviewer notes
regenerate_key_findings.py(the script the change started from and the bulk of its cases) but covers the sibling walkers too; the module docstring says so. Happy to split it per script if you would rather match thetest_regenerate_psense.pyone-file-per-script convention.scripts/andsrc/for rawiterdir()over an output root. The only other hits aretune_model.pyandbase_pipeline.py, both operating on their own directories rather than the statistical-models root, so neither is affected. The GB output root has no output transactions at all —base_pipelineclears in place — so the filter there is inert but harmless.Checks
ruff check src/(also ran overscripts/andtests/) — passnpm run format:check— passnpm run spellcheck— passpytestsuite — 1200 passedRelates to #394