From 9b31a8bc3c6baf7707083758e6f9aac2b8cd88e4 Mon Sep 17 00:00:00 2001 From: sotashimozono Date: Mon, 31 Aug 2026 12:13:58 +0000 Subject: [PATCH] feat(Preflight): refuse a campaign before it burns compute MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Ported out of FiniteTemperature.jl, where it was written after the same class of defect had cost a campaign four separate times. The shape is always the same: declared in one place, relied on in another, checked nowhere. [datavault] float_format = "auto" the config asked; the path builder ignored it beta_quench ⊆ beta_targets required in a comment a stage knowing its upstream's required in a comment discretisation the checkpoint directory carrying not carried at all the parameter distinguishing runs Each is decidable by comparing strings before any physics runs. What lands here is the part that is not specific to one study: Finding / PreflightReport / launchable / n_errors / n_warns / show a report grouped by layer, so a refusal names its own cause check_injective! distinct parameter points must get distinct paths check_opens! a vault that will not open is a finding, not a stack trace on_grid is this value a multiple of the step the loop takes representative_keys one key per point; expand() gives (point × sample) check_injective! is the one worth having. Under a content-blind float rendering, h = 0.002 and 0.004 both render "h0.00", so two points share a directory — and not only their observables. They share the STATUS MARKER, so the second point is reported complete without ever running. A 168-point sweep finished as 132, with no error anywhere. Pass status paths as well as data paths: checking only the data half catches the overwrite and misses the skipped work, which is worse. What stays with the study: which values must lie on which grid, and whether the artefact a later stage LOOKS for is the one an earlier stage WRITES. That last one is decisive and cannot be generic, because only the study knows what its stages hand each other — but it is always built the same way, and the docstring says so: generate the path from BOTH sides and compare the strings, rather than trusting a comment that says they agree. Tests carry a positive control per check. A gate that has only ever returned "ok" is indistinguishable from one that cannot fail. Two of them are worth naming: * the collision message is asserted DETERMINISTIC across repeats — Set iteration order is not, and a CI failure that names a different path each run is unreproducible. * representative_keys is shown against its own absence: counting over all keys reports n_samples-1 false collisions per point, and the test asserts the naive version fails where the correct one passes. Minor bump: additive, but it widens the public surface. Co-Authored-By: Claude Opus 5 (1M context) --- Project.toml | 2 +- README.md | 1 + src/ParallelManager.jl | 1 + src/Preflight.jl | 190 +++++++++++++++++++++++++++++++ test/preflight/test_preflight.jl | 153 +++++++++++++++++++++++++ 5 files changed, 346 insertions(+), 1 deletion(-) create mode 100644 src/Preflight.jl create mode 100644 test/preflight/test_preflight.jl diff --git a/Project.toml b/Project.toml index 7fe1df7..0c1b026 100644 --- a/Project.toml +++ b/Project.toml @@ -1,6 +1,6 @@ name = "ParallelManager" uuid = "be946ad2-3cb3-4b6e-8f7e-4a5ecc3c255b" -version = "0.4.5" +version = "0.5.0" authors = ["sota shimozono "] [deps] diff --git a/README.md b/README.md index 153eae2..a9f16e3 100644 --- a/README.md +++ b/README.md @@ -91,6 +91,7 @@ path builders that leak phase1's storage layout into phase2's code. | [`src/KeyLock.jl`](src/KeyLock.jl) | `mkdir` advisory lock + heartbeat + stale reclaim | | [`src/InitWorkers.jl`](src/InitWorkers.jl) | Unified `:auto` / `:sequential` / `:threads` / `:distributed` / `:slurm` bootstrap | | [`src/Run.jl`](src/Run.jl) | `run!(work_fn, vault, keys; opts)` facade that ties everything to `DataVault` | +| [`src/Preflight.jl`](src/Preflight.jl) | `check_injective!` / `check_opens!` / `on_grid` — refuse a campaign *before* it burns compute | Each module is one file, one concern. They can be used independently (e.g. `atomic_write` + `EventLog` without `run!`). diff --git a/src/ParallelManager.jl b/src/ParallelManager.jl index 2b8312f..c5eccc3 100644 --- a/src/ParallelManager.jl +++ b/src/ParallelManager.jl @@ -69,5 +69,6 @@ include("EventLog.jl") include("Manifest.jl") include("InitWorkers.jl") include("Run.jl") +include("Preflight.jl") end # module ParallelManager diff --git a/src/Preflight.jl b/src/Preflight.jl new file mode 100644 index 0000000..156ba30 --- /dev/null +++ b/src/Preflight.jl @@ -0,0 +1,190 @@ +# Preflight — answer "can this campaign run" before spending the compute. +# +# Ported out of FiniteTemperature.jl, where it was written after the same class +# of defect had cost a campaign four separate times. The shape is always the +# same: **declared in one place, relied on in another, checked nowhere.** +# +# [datavault] float_format = "auto" the config asked; the path builder ignored it +# beta_quench ⊆ beta_targets required in a comment +# the downstream stage knowing the required in a comment +# upstream discretisation +# the checkpoint directory carrying not carried at all +# the parameter that distinguishes runs +# +# Each is decidable by comparing strings before any physics runs. What lives +# here is the part that is not specific to any one study: +# +# * the report — `Finding`, `PreflightReport`, `launchable`, and a `show` that +# groups by layer so a refusal names its own cause +# * `check_injective!` — distinct parameter points must get distinct paths. +# This is THE check: a collision means one point silently overwrites another +# AND its `.done` marker completes work that never ran. +# * `check_opens!` — opening a vault can itself fail (a shared `project_name` +# with differing `path_keys` is a startup crash, not a warning), and that +# belongs in the report rather than as a stack trace out of the checker +# * `on_grid` / `representative_keys` — the two predicates every study repeats +# +# A study adds its own layers on top: which values must lie on which grid, and +# whether the artefact a later stage LOOKS for is the one an earlier stage +# WRITES. That last one is the decisive check and it cannot be generic, because +# only the study knows what its stages hand each other — but it is always built +# the same way: generate the path from BOTH sides and compare the strings, +# rather than trusting a comment that says they agree. + +using Printf +using ParamIO: DataKey +using DataVault: DataVault + +export Finding, PreflightReport, launchable, n_errors, n_warns +export check_injective!, check_opens!, on_grid, representative_keys + +""" + Finding(layer, severity, where, message) + +One reason a campaign cannot (or should not) start. + +`layer` is the study's own vocabulary — `:config`, `:grid`, `:injective`, +`:crossphase` in the original — and exists so that a refusal is attributable. +Layering that cannot distinguish two different causes is decoration; a study's +tests should assert that a deliberately broken config is rejected by the RIGHT +layer, not merely rejected. +""" +struct Finding + layer::Symbol + severity::Symbol # :error / :warn + where::String + message::String +end + +struct PreflightReport + findings::Vector{Finding} + layers::Vector{Symbol} # display order +end + +PreflightReport(fs::Vector{Finding}) = + PreflightReport(fs, unique(f.layer for f in fs)) + +Base.isempty(r::PreflightReport) = isempty(r.findings) +n_errors(r::PreflightReport) = count(f -> f.severity === :error, r.findings) +n_warns(r::PreflightReport) = count(f -> f.severity === :warn, r.findings) + +""" + launchable(r) -> Bool + +True when nothing at `:error` severity was found. Warnings do not block. + +Callers should gate on this and **exit non-zero**, not print a verdict for a +human to grep — see [`verify-by-exit-code`]: a grep over output reports a parse +error as success. +""" +launchable(r::PreflightReport) = n_errors(r) == 0 + +function Base.show(io::IO, r::PreflightReport) + if isempty(r) + println(io, "✅ preflight: nothing to report") + return nothing + end + for layer in r.layers + fs = filter(f -> f.layer === layer, r.findings) + isempty(fs) && continue + println(io, "\n── ", layer, " ──") + for f in fs + println(io, " ", f.severity === :error ? "✗" : "!", " [", f.where, "] ", f.message) + end + end + @printf( + io, "\n%s errors=%d warnings=%d\n", + launchable(r) ? "✅ launchable" : "❌ NOT launchable", n_errors(r), n_warns(r) + ) + return nothing +end + +# ── predicates every study repeats ────────────────────────────────────────── + +""" + on_grid(x, step; atol=1e-9) -> Bool + +Is `x` a multiple of `step`? + +A value that is not is one the evolution steps *past*: the artefact named after +it is never written, and the failure surfaces much later as a missing file in a +downstream stage, long after the compute is spent. +""" +on_grid(x::Real, step::Real; atol::Real=1e-9) = + isapprox(x / step, round(x / step); atol=atol) + +""" + representative_keys(keys) -> Vector{DataKey} + +One key per distinct parameter point. + +`ParamIO.expand` enumerates (point × sample), but a path is a property of the +point — the sample only varies the filename. Counting collisions over all keys +would therefore report `n_samples` false collisions for every real one. +""" +function representative_keys(keys) + seen = Set{Any}() + out = eltype(keys)[] + for k in keys + k.params in seen && continue + push!(seen, k.params) + push!(out, k) + end + return out +end + +# ── the generic checks ────────────────────────────────────────────────────── + +""" + check_injective!(findings, layer, label, name, paths) + +Assert that `paths` has no duplicates, i.e. that distinct parameter points map +to distinct files. + +This is the check worth having. Under a content-blind float rendering (`%.2f`), +`h = 0.002` and `0.004` both render `h0.00`, so two points share a directory — +and not only their observables. They share the **status marker**, which means +the second point is reported complete without ever running. A sweep of 168 +points then finishes as 132 with no error anywhere. + +Pass the status paths as well as the data paths. Checking only the data half +catches the overwrite and misses the skipped work, which is the worse of the two. +""" +function check_injective!( + findings::Vector{Finding}, layer::Symbol, label::AbstractString, + name::AbstractString, paths::AbstractVector{<:AbstractString}, +) + n, u = length(paths), length(Set(paths)) + if u < n + dupes = [p for p in Set(paths) if count(==(p), paths) > 1] + push!(findings, Finding(layer, :error, String(label), + "$name: $n parameter points → $u distinct paths. " * + "$(n - u) point(s) share a path with another, so one silently overwrites " * + "the other and its status marker completes work that never ran. " * + "First collision: $(first(sort(collect(dupes))))")) + end + return findings +end + +""" + check_opens!(findings, layer, label, open_fn) -> Union{Any,Nothing} + +Run `open_fn()` and, if it throws, record the failure instead of propagating it. + +Opening a vault is itself a thing that fails — most commonly a DataVault +`log.toml conflict` when two stages of one campaign share a `project_name` but +declare different `path_keys`. A driver that opens several vaults in a row dies +at startup on that, so it belongs in the report next to everything else rather +than as a stack trace out of the checker. +""" +function check_opens!( + findings::Vector{Finding}, layer::Symbol, label::AbstractString, open_fn::Function, +) + try + return open_fn() + catch e + push!(findings, Finding(layer, :error, String(label), + "could not open the vault: $(sprint(showerror, e))")) + return nothing + end +end diff --git a/test/preflight/test_preflight.jl b/test/preflight/test_preflight.jl new file mode 100644 index 0000000..9e3dce9 --- /dev/null +++ b/test/preflight/test_preflight.jl @@ -0,0 +1,153 @@ +""" +test_preflight.jl — the report machinery, and proof that each check can refuse. + +The second half is the load-bearing one. A gate that has only ever returned +"ok" is indistinguishable from a gate that cannot fail, so every check here is +given input it MUST reject, and the rejection is asserted to carry the right +layer — a layering that cannot distinguish two causes is decoration. + +`check_injective!` is the one worth this care. In the study this was ported out +of, a content-blind float rendering collapsed `h = 0.002` and `0.004` onto one +directory: 168 sweep points became 132, and because the *status marker* +collided too, the second point of each pair was reported complete without ever +running. No error anywhere. +""" + +using Test +using ParallelManager + +@testset "Preflight" begin + @testset "an empty report is launchable and says so" begin + r = PreflightReport(Finding[]) + @test isempty(r) + @test launchable(r) + @test n_errors(r) == 0 + @test n_warns(r) == 0 + @test occursin("nothing to report", sprint(show, r)) + end + + @testset "warnings do not block; errors do" begin + warn = Finding(:grid, :warn, "cfg", "a note") + err = Finding(:grid, :error, "cfg", "a blocker") + @test launchable(PreflightReport([warn])) + @test !launchable(PreflightReport([warn, err])) + @test n_warns(PreflightReport([warn, err])) == 1 + @test n_errors(PreflightReport([warn, err])) == 1 + end + + @testset "show groups by layer and states the verdict" begin + r = PreflightReport([ + Finding(:grid, :error, "a.toml", "off grid"), + Finding(:injective, :error, "b.toml", "collision"), + ]) + out = sprint(show, r) + @test occursin("grid", out) + @test occursin("injective", out) + @test occursin("NOT launchable", out) + @test occursin("errors=2", out) + end + + # ── on_grid ───────────────────────────────────────────────────────────── + + @testset "on_grid" begin + @test on_grid(0.4, 0.2) + @test on_grid(0.0, 0.2) + @test on_grid(20.0, 0.2) + @test !on_grid(0.5, 0.2) # steps 0.4 -> 0.6, never lands + @test !on_grid(1.5, 0.2) + # accumulated floats must still count as on-grid + t = 0.0 + for _ in 1:100 + t += 0.2 + end + @test on_grid(t, 0.2) + @test t != 20.0 # …and it is NOT exactly 20.0 + end + + # ── check_injective! ──────────────────────────────────────────────────── + + @testset "check_injective! is silent on distinct paths" begin + fs = Finding[] + check_injective!(fs, :injective, "cfg", "obs", ["a", "b", "c"]) + @test isempty(fs) + end + + @testset "check_injective! reports a collision, with the offender" begin + fs = Finding[] + check_injective!(fs, :injective, "cfg", "obs", ["a", "b", "a"]) + @test length(fs) == 1 + f = only(fs) + @test f.layer === :injective + @test f.severity === :error + @test f.where == "cfg" + @test occursin("3 parameter points → 2 distinct paths", f.message) + @test occursin("a", f.message) # names which path collided + # the consequence, not just the count — this is the half that makes work + # be SKIPPED rather than merely overwritten + @test occursin("never ran", f.message) + end + + @testset "check_injective! counts every duplicate, not just the first" begin + fs = Finding[] + check_injective!(fs, :injective, "cfg", "obs", ["a", "a", "b", "b", "c"]) + @test occursin("5 parameter points → 3 distinct paths", only(fs).message) + end + + @testset "the collision it names is deterministic" begin + # Set iteration order is not; the message must not vary run to run or a + # CI failure is unreproducible. + msgs = map(1:8) do _ + fs = Finding[] + check_injective!(fs, :injective, "cfg", "obs", ["z", "z", "a", "a"]) + only(fs).message + end + @test length(unique(msgs)) == 1 + end + + # ── check_opens! ──────────────────────────────────────────────────────── + + @testset "check_opens! passes the value through on success" begin + fs = Finding[] + v = check_opens!(fs, :config, "cfg", () -> 42) + @test v == 42 + @test isempty(fs) + end + + @testset "check_opens! records a failure instead of throwing" begin + fs = Finding[] + v = check_opens!(fs, :config, "cfg", () -> error("log.toml conflict")) + @test v === nothing + @test length(fs) == 1 + @test only(fs).layer === :config + @test occursin("could not open the vault", only(fs).message) + @test occursin("log.toml conflict", only(fs).message) + end + + # ── representative_keys ───────────────────────────────────────────────── + + @testset "representative_keys collapses samples, keeps points" begin + mk(n, s) = ParallelManager.DataKey(Dict{String,Any}("N" => n), s) + keys = [mk(4, 1), mk(4, 2), mk(4, 3), mk(8, 1), mk(8, 2)] + reps = representative_keys(keys) + @test length(reps) == 2 + @test Set(k.params["N"] for k in reps) == Set([4, 8]) + # order is the first-seen order, so a report reads the same twice + @test [k.params["N"] for k in reps] == [4, 8] + end + + @testset "why collapsing matters: samples are not collisions" begin + # Counting over all keys would report n_samples-1 false collisions per + # point, because a path is a property of the POINT. + mk(n, s) = ParallelManager.DataKey(Dict{String,Any}("N" => n), s) + keys = [mk(4, s) for s in 1:5] + dir_of(k) = "N$(k.params["N"])" # sample lives in the filename + fs = Finding[] + check_injective!(fs, :injective, "cfg", "dir", [dir_of(k) for k in keys]) + @test !isempty(fs) # naive: 5 -> 1, looks broken + fs2 = Finding[] + check_injective!( + fs2, :injective, "cfg", "dir", [dir_of(k) for k in representative_keys(keys)] + ) + @test isempty(fs2) # correct: one point, one dir + end +end