diff --git a/.github/workflows/ci.yml b/.github/workflows/ci.yml index d5df175..d3e3e9c 100644 --- a/.github/workflows/ci.yml +++ b/.github/workflows/ci.yml @@ -77,6 +77,34 @@ jobs: - name: make install run: make install + # **No `xvfb` here, and that is a recorded decision rather than an + # omission** -- so the 10 display-guarded tests + # (`@_needs_a_real_display` in `tests/integration/ + # test_interactive_window.py` and `test_playback_cli.py`) skip on + # Linux and run only on Windows. Without a `DISPLAY`, + # `_display_available()` short-circuits before GLFW is ever + # constructed (deliberately: GLFW hard-aborts the process rather + # than raising, see `tests/integration/CLAUDE.md`). + # + # **Tried and reverted 2026-09-11/12 by the Stage 8 exit audit.** + # Installing `xvfb` and running `xvfb-run -a make ci` does work -- + # Linux skips dropped from 29 to 18, matching a local run, so all + # 10 tests genuinely ran. It was reverted because it made the + # Linux job flaky: across three attempts on identical test code, + # two passed and one crashed an xdist worker outright, with no + # Python traceback (the hard process abort GLFW produces), and the + # crashed one then passed on a plain re-run. 8 workers creating + # software-GL contexts against one Xvfb display concurrently is the + # working hypothesis, never reproduced locally. + # + # **Serialising the display tests onto one worker + # (`--dist loadgroup`) is not the fix, and made things worse**: it + # hung the *Windows* job for 5h45m until GitHub's own 6-hour limit + # killed it, at 95% with ~6 tests outstanding and an orphaned + # python process at cleanup. One process creating ~15 GLFW windows + # in sequence is the failure that buys; 1-2 per process spread + # across workers is what has always worked. See + # `docs/planning/backlog.md` for the open item. - name: make ci env: # Runs with more test parallelism than a local `make test` diff --git a/.gitignore b/.gitignore index 6499b69..838a584 100644 --- a/.gitignore +++ b/.gitignore @@ -46,3 +46,16 @@ checkpoints/ # `replay.py`) -- README's own documented example uses `--cache cache` # from the repository root; same reasoning as `checkpoints/` above. cache/ + +# The same run output as `checkpoints/` above, but written straight into +# whatever directory `pyflow record`/`resume` was invoked from -- what +# `--output-dir .` produces, and what a user experimenting from the +# repository root gets. **Added 2026-09-11 by the Stage 8 exit audit**, +# which found 19 of these sitting untracked in the repository root from +# a 2026-09-07 manual run: the `checkpoints/` rule above only ever +# matched the default directory, never the files themselves. +checkpoint_*.pt + +# `pyflow play --cache`'s own materialized windows, for the same reason +# -- `window_{from}_{to}.pt` written to a cache directory given as `.`. +window_*.pt diff --git a/README.md b/README.md index 6dc47dd..681ecab 100644 --- a/README.md +++ b/README.md @@ -148,12 +148,25 @@ the roadmap rather than restated here: **Stage 8's own record, for anyone tracking how reliably this section stays current**: opened and closed in one day (2026-09-07), reopened -two days later, and reclosed the same day it was reopened -- each edit -to this paragraph has so far landed in the same change as the roadmap -event it describes, unlike the multi-day staleness windows the two -paragraphs below describe for Stages 7 and 8's own *earlier* drafts. -Don't read this as the pattern solved; read Stage 9's own eventual -entry here as the next real test of it. +two days later, reclosed the same day it was reopened, and audited at +its exit on 2026-09-11 -- each edit to this paragraph has so far landed +in the same change as the roadmap event it describes, unlike the +multi-day staleness windows the two paragraphs below describe for +Stages 7 and 8's own *earlier* drafts. Don't read this as the pattern +solved; read Stage 9's own eventual entry here as the next real test of +it. + +**And read the exit audit before trusting that record too far.** This +paragraph tracks whether *this section* stays current, which it has. +What the audit found is that three other places did not: three of Stage +8's nine Completion Criteria were marked Met against checks that did not +test what the criterion said, and three documents outside this file +still described the pre-reopening stage -- one of them a +`Checked-by: stage-boundary` document that all four reopening tasks +landed without touching. All six are fixed; `docs/planning/roadmap.md`'s +own **Exit audit, 2026-09-11** section is the record. **A section that +keeps itself current is not evidence that the repository around it +did.** **This sentence said "not yet started" for Stage 8 itself, twice, while that stage was still open** -- once for the same reason a fourth time as diff --git a/docs/architecture/CLAUDE.md b/docs/architecture/CLAUDE.md index a69bd2b..25ddc69 100644 --- a/docs/architecture/CLAUDE.md +++ b/docs/architecture/CLAUDE.md @@ -42,12 +42,20 @@ all three of that stage's pieces in real code: `src/pyflow/checkpoint.py`/`recording.py`/`simulation_run.py` (TASK-045, checkpointing), and `src/pyflow/replay.py`/`playback.py` (TASK-046/047, windowed replay and interactive playback, built the same -day). Stage 8 itself is complete, all five of its own Completion +day). Stage 8 itself is complete, all **nine** of its own Completion Criteria met -- see `docs/planning/roadmap.md`'s own Stage 8 Status section for the one real course-correction along the way (its Golden Demo moved from Heat Diffusion to Lid-Driven Cavity once TASK-047's own scope decision made the first choice incompatible). +**This paragraph said "all five" until 2026-09-11**, correct when +written on 2026-09-07 and falsified two days later when the stage was +reopened and Criteria 6-9 were added (TASK-048/049/050/051). Found by +that stage's own exit audit, not by anything that runs -- a criteria +*count* is exactly the decaying-number shape `docs/practices.md` +warns about under "A stage's documentation sweep is a grep, not a diff +review", restated here in a file no Stage 8 task ever opened. + **Section 3's anchor history is worth keeping, because it is why the subsection existed to be finished at all.** It used to say the subsection was "anchored to the specific roadmap task that will build it diff --git a/docs/architecture/sequences.md b/docs/architecture/sequences.md index 28707f0..0b37a72 100644 --- a/docs/architecture/sequences.md +++ b/docs/architecture/sequences.md @@ -384,6 +384,9 @@ sequenceDiagram alt frame_count % checkpoint_interval == 0, or final frame recording->>checkpoint: write_checkpoint(frame_count, config, state.fields) checkpoint->>Disk: checkpoint_{frame_count:08d}.pt + opt max_checkpoints_retained set (TASK-049) + recording->>Disk: _prune_checkpoints() -- keep frame 0 + newest N + end end end ``` @@ -443,6 +446,9 @@ sequenceDiagram alt frame_count % checkpoint_interval == 0, or final frame recording->>checkpoint: write_checkpoint(frame_count, config, state.fields) checkpoint->>Disk: checkpoint_{frame_count:08d}.pt + opt max_checkpoints_retained set (TASK-049) + recording->>Disk: _prune_checkpoints() -- keep frame 0 + newest N + end end end ``` @@ -472,6 +478,18 @@ nothing. Those are Stage 8's own second and third bullets, and both are built now (TASK-046/047, 2026-09-07): the rest of this subsection covers them. +**Retention prunes after every write, not once at the end** (TASK-049, +added to both diagrams above 2026-09-11) -- which is what makes +`max_checkpoints_retained` bound peak disk use *as a long run grows* +rather than only its final footprint. A 300-frame run at `interval=5` +with a cap of 3 writes 61 checkpoints and never holds more than 5 at +once. Opt-in: unset (the default) deletes nothing, so every config +written before TASK-049 produces exactly the files it always did. Frame +0 is excluded from the count itself rather than merely surviving by +being newest, so a capped recording always keeps a point to restart +from. `resume` prunes the whole `output_dir`, including checkpoints an +earlier `record` left there, not only the files it wrote itself. + ### Built today: windowed replay and interactive playback (`pyflow play`) ```mermaid @@ -487,6 +505,10 @@ sequenceDiagram playback->>replay: materialize_or_load_window(...) alt --cache given and an exact-range match exists replay->>Disk: read_materialized_window(cache_path) + else --cache given and a cached window is a strict superset (TASK-050) + replay->>replay: _find_superset_window(cache_dir, from_frame, to_frame) + replay->>Disk: read_materialized_window(superset_path) + replay->>replay: slice out [from_frame, to_frame] -- no re-simulation else materialize fresh replay->>replay: find_checkpoint_at_or_before(checkpoints_dir, from_frame) replay->>checkpoint: read_checkpoint() + restore_simulation_state() @@ -499,13 +521,23 @@ sequenceDiagram end replay-->>playback: MaterializedWindow (config, frames[]) playback->>Window: RenderWindow(config.rendering), build_vector_field_arrows(frames[0]) + opt config declares fields (TASK-051) + playback->>Window: field_visualization.panel_colors/build_panel_legend per panel + end + playback->>Window: scrub bar track + thumb (gfx.Line + gfx.Points) Window-->>Window: window.playback_state = PlaybackState() playback->>Window: canvas.add_event_handler(_on_key, "key_down") + playback->>Window: canvas.add_event_handler(pointer_down/move/up) -- scrub drag (TASK-048) loop each real draw - Window->>playback: on_frame() - playback->>playback: advance_playback_position() -- position += speed unless paused + alt a seek event arrived (TASK-048) + Window->>playback: _on_key ArrowLeft/Right -> seek_relative, Home/End -> seek_to + Window->>playback: pointer drag on the bar -> frame_index_from_fraction -> seek_to + else no input + Window->>playback: on_frame() + playback->>playback: advance_playback_position() -- position += speed unless paused + end opt materialized frame index changed - playback->>Window: remove old arrows, build_vector_field_arrows(frames[index]) + playback->>Window: rebuild arrows, panels and thumb from frames[index] end end ``` @@ -517,20 +549,43 @@ window` (what `play()` actually calls) is the only place a cache is read or written, and only when `--cache DIR` is given. Nothing is written to disk by a bare `pyflow play`. -**Scoped to solved-velocity-only rendering for this first cut** -- +**Arrows and declared-field panels, from the same materialized frame.** `playback.py` builds `gfx` arrows from `MaterializedWindow.frames[i]`'s `velocity.0`/`velocity.1` tensors the same way `bootstrap.py`'s own -`_add_solved_velocity_rendering` does from a live `SimulationState`, but -has no declared-field/scalar-colormap path yet -(`UnsupportedPlaybackConfigError` otherwise) -- see `src/pyflow/ -CLAUDE.md`'s own `playback.py` entry for the full reasoning and the -scene-rebuild-cost measurements that shaped the speed mechanism -(`position += speed`, not more draws per second). - -**Every subsection in this section is now built.** `sequences.md`'s own -Maintenance note, below, no longer names a task to re-read this file -for -- update it again the next time Stage 8 gains a fourth piece, or -whenever any task named in this section is touched. +`_add_solved_velocity_rendering` does from a live `SimulationState`, and +-- since TASK-051 -- colours any declared field's panels from that same +frame, through the `rendering/field_visualization.py` helpers +`bootstrap.py`'s own live path calls rather than a second copy of them. +A config with **no** solved velocity at all is still rejected +(`UnsupportedPlaybackConfigError`); see `src/pyflow/CLAUDE.md`'s own +`playback.py` entry for the full reasoning and the scene-rebuild-cost +measurements that shaped the speed mechanism (`position += speed`, not +more draws per second). + +**Seeking is bounded by the window loaded at launch.** `seek_relative`/ +`seek_to` clamp to `[0, max_index]` of the materialized window, so +Home/End reach that window's own edges and not the recording's -- +reaching anything outside it still needs a fresh `pyflow play` with +different `--from-frame`/`--to-frame`. A stated Stage 8 scope boundary, +not an oversight: `docs/planning/roadmap.md`'s own Completion Criterion +6 records it, and `pyflow play --help` tells a user so directly. + +**Every subsection in this section is built.** Seven Stage 8 tasks are +covered here now: TASK-045 (recording, `resume`), TASK-049 (retention), +TASK-046 (windowed replay) and TASK-050 (subset cache reuse) above, +TASK-047 (playback) and TASK-048 (live scrub) and TASK-051 (declared +fields in playback) in this subsection. + +**This subsection described only TASK-046/047 until 2026-09-11**, and +one of its paragraphs ("scoped to solved-velocity-only rendering for +this first cut... no declared-field/scalar-colormap path yet") had been +false since TASK-051 landed on 2026-09-09. It was found by that stage's +own exit audit rather than by the re-read this file's own closing note +below asked for -- the note said to "update it again the next time +Stage 8 gains a fourth piece", Stage 8 then gained four pieces, and +nothing re-read it. **A `Checked-by: stage-boundary` declaration is a +promise about the stage boundary, not about the tasks in between**, +which is exactly how four tasks landed without touching this file. --- @@ -653,3 +708,30 @@ playback are both real, built sequences in Section 3 now, not Planned in substance under a different heading; that section's own closing note says so and names no further task, since Stage 8 has nothing left undrafted to anchor to. + +**And then the mechanism failed a second time, in the one way the +paragraphs above did not anticipate: no anchor existed to fail.** +Stage 8 was reopened on 2026-09-09 and gained four more tasks +(TASK-048/049/050/051 -- live scrub, checkpoint retention, subset cache +reuse, combined declared-field playback). All four landed without this +file being touched, and one of Section 3's own paragraphs was left +saying playback had "no declared-field/scalar-colormap path yet" two +days after TASK-051 built exactly that. Found 2026-09-11 by the Stage 8 +exit audit; Section 3 now covers all seven tasks. + +**Why the existing lesson did not reach it.** Every mechanism described +above keys off a *named task* in this file -- "grep this file's own +TASK-NNN mentions the next time any named task is touched". TASK-048-051 +were new tasks this file had never named, so there was nothing to grep +and nothing to notice. The paragraph that should have caught them was +the closing note's own "update it again the next time Stage 8 gains a +fourth piece", which is a standing instruction addressed to nobody in +particular and fired for no one. + +**So the rule this file actually needs is the one its own header already +declares:** `Checked-by: stage-boundary` means this document is re-read +when a stage opens or closes, and a stage that *reopens* is a stage +boundary twice over. The reopening on 2026-09-09 was the moment to +re-read this file, and the audit two days later was the second. Neither +a task anchor nor a "next time" note substitutes for that, because both +depend on a reader who already knows to look here. diff --git a/docs/planning/backlog.md b/docs/planning/backlog.md index 29d4505..bd27213 100644 --- a/docs/planning/backlog.md +++ b/docs/planning/backlog.md @@ -2427,6 +2427,54 @@ here.): actually built (recording, replay, playback's own first cut) and does not track the reopening's own progress. +- [ ] **Run the display-guarded tests on Linux CI.** Raised 2026-09-12 by + the Stage 8 exit audit, which found the gap, tried two fixes, and + reverted both. The 10 tests carrying `@_needs_a_real_display` + (`tests/integration/test_interactive_window.py`, + `test_playback_cli.py`) skip on `ubuntu-latest` and run only on + `windows-latest`, so **a green two-platform matrix proves + live-window behaviour on one platform** -- including every check + Stage 8's own Completion Criterion 6 rests on. + + **Not blocking**: the tests do run, on Windows, every CI run, and + they are the platform the maintainer develops on. What is lost is + the second platform's confirmation, which for GLFW/wgpu behaviour + is exactly where the two have diverged before + (`tests/integration/CLAUDE.md`'s own 2026-08-19 SIGABRT entry). + + **Both obvious approaches are already known to fail, with + measurements** -- read `tests/integration/CLAUDE.md` and + `.github/workflows/ci.yml`'s own comment before attempting this, + rather than rediscovering them: + + 1. **`xvfb` works and is flaky.** `xvfb-run -a make ci` plus an + `xvfb` apt package dropped Linux skips from 29 to 18, matching a + local run -- so all 10 tests genuinely ran. Across three + attempts on *identical* test code, two passed and one crashed an + xdist worker with no Python traceback (GLFW's hard process + abort), and that attempt then passed on a plain re-run. Roughly + a third of attempts, on a gating check. + 2. **Serialising them onto one worker is worse.** `--dist + loadgroup` plus an `xdist_group("display")` mark hung the + *Windows* job for 5h45m until GitHub's own 6-hour limit killed + it -- 95% complete, ~6 tests outstanding, an orphaned python + process at cleanup. It passed `make ci` locally in 183s, so + nothing short of CI could have caught it. + + **The unblock condition is a hypothesis worth testing, not a + design**: both failures point the same way from opposite + directions -- approach 1 raised *concurrent* window creation across + processes, approach 2 raised *sequential* window creation within + one process, and each broke a different platform. What has always + worked is 1-2 GLFW windows per process, spread across workers. So + the likely route is **reducing the number of windows rather than + redistributing them**, starting with the two + `*_rerenders_the_field_in_real_pixels` tests, which open two each + (a main window plus an independent reference window) and are the + highest count in the suite. Whether that alone takes the Linux + flake rate to zero is unmeasured; the flake was never reproducible + locally, so CI is the only place any of this can be judged, which + is what makes it a task rather than a quick fix. --- # Part III — Audit history diff --git a/docs/planning/roadmap.md b/docs/planning/roadmap.md index 7ddfcf3..465f431 100644 --- a/docs/planning/roadmap.md +++ b/docs/planning/roadmap.md @@ -306,7 +306,20 @@ This paragraph previously said `make install` and `make test` were still expected to fail, pending `uv.lock` and a test suite (B2/C1) -- stale since 2026-08-16 and corrected 2026-08-19. Both now succeed: `uv.lock` is committed (B2) and `make test` runs the suite with coverage -(C1a/C1b): **1200 tests as of 2026-09-09**, up from 1199 the same day +(C1a/C1b): **1209 tests as of 2026-09-11**, up from 1200 on 2026-09-09 +(the Stage 8 exit audit, closing the gap between three Completion +Criteria and what actually checked them: 4 in +`tests/integration/test_import_order.py` (Criterion 1's own "imports +neither `rendering` nor anything that transitively imports it", which +that criterion's own text admitted had only ever been "checked by hand +at implementation time"), 1 in `tests/unit/test_recording.py` and 1 in +`tests/integration/test_record_cli.py` (Criterion 8's own +"multi-hundred-frame" run, and the peak-disk bound that makes retention +mean anything on a long run), 2 in +`tests/integration/test_playback_cli.py` (Criterion 6's own "each +change `window.renderer.snapshot()`'s content", which neither scrub +test had ever called), and 1 more there for `pyflow play --help`'s own +controls block), 1200 itself up from 1199 on 2026-09-09 (`_add_declared_field_transport` also drawing the solved velocity as arrows when `config.simulation.velocity_solved` is set, alongside its declared fields' own panels -- the live `pyflow run` counterpart to @@ -11346,6 +11359,22 @@ them, which had not been drafted yet when these were written. a different `pyflow play` invocation, exactly as today. Extending scrub to seek beyond the loaded window is real, deferred future work, not built here. + - **Standing exclusion, reaffirmed at the 2026-09-11 exit audit and + recorded here rather than left to a task's Design decisions.** + This stage's own Goal promises playback "paused, **scrubbed to any + point**, and watched at a different speed" -- and "any point" + still means any point *in the loaded window*, not in the + recording. The gap is deliberate and the stage closes over it + knowingly, which is a different thing from the 2026-09-09 + reopening, where the same Goal clause had no criterion at all and + nobody had noticed. **Closing a stage over a Goal clause it only + partly satisfies is a decision, so it is written where a decision + goes**: the maintainer was asked directly at the exit audit and + chose to defer rather than build it. If it is built later it needs + a new task and a new criterion, not a quiet widening of this one. + A user meets the boundary in `pyflow play --help`'s own controls + block ("Seeking covers the loaded window only"), which is the one + place it stops reading as a bug. 7. **A config combining a solved velocity field with one or more declared fields plays back correctly, not only a solved-velocity-only config.** TASK-047's own stated scope boundary @@ -11445,14 +11474,14 @@ what shipped rather than against the criteria that were meant to operationalise | Criterion | Verdict | |-----------|---------| -| 1. Recording never opens a rendering window | **Met** -- TASK-045 | +| 1. Recording never opens a rendering window | **Met** -- TASK-045. Verdict unchanged, evidence changed: true all along, but gated only from 2026-09-11 (exit audit) | | 2. A recording's disk footprint is bounded | **Met** -- TASK-045 | | 3. Resuming reproduces the same trajectory, bit-identically | **Met** -- TASK-045, mutation-tested | | 4. A checkpoint file is self-contained | **Met** -- TASK-045 | | 5. Golden Demo runs end to end, both halves | **Met** -- TASK-045 (record), TASK-046/047 (playback), against Lid-Driven Cavity | -| 6. Live scrub, keyboard and mouse | **Met** -- TASK-048, verified against a real window | +| 6. Live scrub, keyboard and mouse | **Met, 2026-09-11** -- TASK-048 built it and proved it against `PlaybackState`; the criterion's own rendered-pixel clause was discharged by the exit audit, mutation-tested | | 7. Combined solved-velocity + declared-field playback | **Met** -- TASK-051, verified against a real Smoke Transport run | -| 8. Checkpoint retention, opt-in, frame 0 never pruned | **Met** -- TASK-049, mutation-tested | +| 8. Checkpoint retention, opt-in, frame 0 never pruned | **Met, 2026-09-11** -- TASK-049, mutation-tested; the criterion's own "multi-hundred-frame" clause discharged by the exit audit | | 9. Partial-overlap (subset) cache reuse | **Met** -- TASK-050, mutation-tested | **All nine criteria are met; the stage is complete again, reclosed @@ -11476,6 +11505,138 @@ directly) would have violated `field_visualization.py`'s own standing "owns no window" rule if moved as-is -- see that task's own Design decision 3. +### Exit audit, 2026-09-11 + +**Run at the maintainer's request, against a stage already marked +complete twice.** `make ci` was green throughout -- 1200 tests, 144 +scenarios, 99% coverage -- and three of the nine criteria were marked +Met against checks that did not test what the criterion said. None of +the three needed judgement to find: each is a sentence the criterion had +already written down, in a criterion somebody had already read. This is +`docs/practices.md`'s own "An exit audit reads each criterion to its +last sentence", recurring one stage after it was written down as a +standing rule, and now recurring on a stage whose *previous* audit is +the reason Criteria 6-9 exist at all. + +**What the three were, and what closed them.** + +1. **Criterion 6's rendered-pixel clause was checked by nothing.** The + criterion says "A keyboard seek and a mouse drag each change + `window.renderer.snapshot()`'s content". Neither + `test_arrow_and_home_end_keys_seek_playback_live` nor + `test_dragging_the_scrub_bar_seeks_without_panning_the_camera` calls + `snapshot()` at all; both assert on `PlaybackState.position`, and + the first one's own docstring says so. Closed by two new tests + (`test_keyboard_seeking_rerenders_the_field_in_real_pixels`, + `test_dragging_the_scrub_bar_rerenders_the_field_in_real_pixels`). +2. **Criterion 8 named a scale nothing had ever run.** "Checked + directly against a real multi-hundred-frame `pyflow record` run" -- + the CLI test used `--max-frames 20`, and TASK-049's own by-hand + verification used 25. The behaviour was right; the evidence was two + orders of magnitude short of the sentence. Closed by a real + 300-frame CLI run and, separately, by pinning the peak on-disk count + during the run, which is the claim `_advance_and_checkpoint`'s own + docstring makes ("as the recording grows, not only once it + finishes") and which the final file set cannot prove either way. +3. **Criterion 1 rested on a by-hand check, and said so.** Its own text + admits "the stronger claim was checked by hand at implementation + time", and names `test_import_order.py` while noting that test "does + not by itself prove the absence of a `rendering` import". It was + true -- re-verified by hand during this audit -- and one added import + would have silently falsified it. Closed by a fresh-subprocess gate + over the transitive closure, for all four headless modules. + +**Every one of the three fixes was mutation-tested before being trusted +green**, and the Criterion 6 pair took three attempts to get teeth, +which is the most useful thing this audit produced: + +- *"The pixels changed after seeking"* proves nothing -- the scrub thumb + moves with the index, so any seek repaints something whether or not + the field was rebuilt. +- *"A seek-reached frame matches an autoplay-reached frame at the same + index"* -- the obvious repair -- **also passed against a deliberately + frozen field**, because both sides of that comparison come from the + same run and were equally frozen. A reference computed by the run + under test cannot detect a fault common to the whole run. +- What works: an independent reference (a separate `play()` window + launched *at* the target frame, where that frame is index 0 and is + drawn by the initial scene build, never by the seek path) compared + over the field region only (cropping away the scrub bar). Both halves + then fail correctly -- against a frozen field, and against an + off-by-one field, checked separately. + +The second bullet is the transferable lesson and is recorded as a rule +in `docs/practices.md`: **a test whose expected value is produced by +the same run it is checking proves consistency, not correctness.** + +**Documentation the stage invalidated and never updated.** Three +findings, all in files no Stage 8 task opened -- the shape +`docs/practices.md`'s "A stage's documentation sweep is a grep, not a +diff review" already names: + +- `docs/architecture/CLAUDE.md` said Stage 8 was complete with "all + five" criteria met, written 2026-09-07 and falsified two days later by + the reopening that added four more. +- `docs/repository-manifest.md`'s `sequences.md` row said "Deterministic + replay and playback (TASK-046/047) are still unbuilt" -- stale from + the day it was written, since both landed 2026-09-07 alongside the + TASK-045 the same sentence credits. +- `docs/architecture/sequences.md`, a `Checked-by: stage-boundary` + document, described only TASK-045/046/047. All four reopening tasks + landed without it being touched, and one paragraph still said playback + had "no declared-field/scalar-colormap path yet" two days after + TASK-051 built exactly that. Its own closing note had asked to "update + it again the next time Stage 8 gains a fourth piece"; Stage 8 gained + four pieces and nobody re-read it. See that file's own Maintenance + section for why neither a task anchor nor a "next time" note reaches a + task the file has never named. + +**Two smaller things fixed on the way past.** `pyflow play --help` +documented none of the controls the subcommand exists for -- pause, +speed, scrub -- so `README.md` was the only place to find them; and 19 +stray `checkpoint_*.pt` files from a 2026-09-07 manual run sat untracked +in the repository root, which `.gitignore`'s `checkpoints/` rule never +matched. + +**A third was attempted and reverted, which is the more useful +record.** Linux CI has no `DISPLAY` and no `xvfb`, so all 10 +display-guarded tests -- every check Criterion 6 rests on -- skip there +and run only on Windows, making a green two-platform matrix prove +live-window behaviour on one platform. **Two fixes were tried over +2026-09-11/12 and both were reverted, each having broken a different +platform:** + +- **`xvfb` worked, and was flaky.** Linux skips dropped from 29 to 18, + matching a local run, so all 10 genuinely ran. But across three + attempts on *identical* test code, two passed and one crashed an xdist + worker with no Python traceback (GLFW's hard process abort), and that + attempt then passed on a plain re-run -- roughly a third of attempts, + on a gating check. +- **Serialising them onto one worker (`--dist loadgroup`) was worse.** + It hung the *Windows* job for 5h45m until GitHub's own 6-hour limit + killed it, at 95% with ~6 tests outstanding. It had passed `make ci` + locally in 183s, so nothing short of CI could have caught it. + +**Both point the same way from opposite directions**: the first raised +concurrent window creation across processes, the second raised +sequential window creation within one process, and what has always +worked is 1-2 GLFW windows per process spread across workers. A real fix +has to reduce the window count rather than redistribute it, which is a +task and not a polish item -- `docs/planning/backlog.md` now carries it +with these measurements, and `tests/integration/CLAUDE.md` and +`.github/workflows/ci.yml` both say why the obvious approaches are +already known to fail. **Recorded as an open gap rather than left +looking closed**: Criterion 6 is met, and the evidence for it runs on +one of the two platforms CI covers. + +**What this audit deliberately did not do.** The Goal's own "scrubbed +to any point" is still satisfied only within the window one `pyflow +play` invocation loads -- see the standing exclusion recorded under +Completion Criterion 6 below. That is now stated in three places a user +or a future contributor will actually meet it (the criterion, `pyflow +play --help`, and `sequences.md`), rather than only in a task's Design +decisions, which is where it was hiding when this audit started. + --- ## TASK-045 — Periodic Checkpointing (Headless Recording) diff --git a/docs/planning/status.md b/docs/planning/status.md index 9e2533e..cd5c08b 100644 --- a/docs/planning/status.md +++ b/docs/planning/status.md @@ -46,7 +46,7 @@ pie showData ## Live repository facts - **49** `CLAUDE.md` files -- **1200** tests collected +- **1209** tests collected - **144** Gherkin scenarios (`tests/features/*.feature`) ## Stages diff --git a/docs/practices.md b/docs/practices.md index 5efe8d0..e552e6e 100644 --- a/docs/practices.md +++ b/docs/practices.md @@ -1493,6 +1493,55 @@ not reading the second half of three sentences" -- restated as a rule rather than as a stage's own observation, because describing it once did not stop it recurring at a higher count one stage later. +## A reference the run under test produced proves consistency, not correctness + +**Standing rule, 2026-09-11, from the Stage 8 exit audit.** That audit +had to write the same test three times, and the second version is the +one worth recording, because it looked right, passed, and proved +almost nothing. + +The criterion asked that a scrub seek "change `window.renderer. +snapshot()`'s content". The obvious test -- *did the pixels change after +seeking?* -- is worthless here: the scrub thumb moves with the frame +index, so any seek repaints something whether or not the field +underneath it was ever rebuilt. The obvious repair was to compare the +seek-reached frame against an **autoplay-reached** frame at the same +index, so the thumb is in the same place on both sides and only the +field can differ. + +**That version passed against a deliberately frozen field.** Both sides +of the comparison were rendered by the same run, so freezing the field +froze the reference too, and the two matched exactly as before. The test +was not checking that the field was right; it was checking that the +field was *the same on both paths*, which a global fault satisfies +trivially. + +The repair that works has two parts, and both were needed: + +1. **The reference comes from outside the run under test.** A separate + `play()` window launched *at* the target frame, where that frame is + index 0 and is therefore drawn by the initial scene build -- not by + the seek path, not by the advance path, not by anything the test is + trying to exercise. +2. **The comparison excludes whatever moves for a reason other than + the one being tested.** Here that is the scrub bar, cropped out by + hashing only the rows above the mesh's own bottom edge, so the + thumb cannot reintroduce the confound from the first version. + +**So, when writing a test whose expected value is computed rather than +written down: ask what produced the expected value, and whether the +fault you are hunting would have corrupted it too.** If the answer is +yes, the test can only ever prove the two paths agree. Mutation-testing +is what surfaces this and nothing else reliably does -- the frozen-field +mutation took under a minute to apply and was the only reason the +second version was not merged looking green and meaning nothing. + +This is a sharper form of the repository's existing mutation-testing +discipline. That discipline says *run the test against a broken +implementation before trusting it green*. This rule says what to do +with the answer when the test passes anyway: the problem is usually not +the assertion, it is where the expected value came from. + ## A gap recorded in a `CLAUDE.md` is not recorded against a criterion **Standing rule, 2026-08-29, from the Stage 5 exit audit.** That stage diff --git a/docs/repository-manifest.md b/docs/repository-manifest.md index f4af87f..4adae08 100644 --- a/docs/repository-manifest.md +++ b/docs/repository-manifest.md @@ -154,7 +154,7 @@ Not present, deferred consciously rather than overlooked: | overview.md | 🟩 | Top-level system map -- no KA entry; legitimate but unspecified | | rendering.md | 🟩 | Architecture of the adopted renderer -- no KA entry | | repository.md | 🟩 | Repository architecture -- no KA entry | -| sequences.md | 🟩 | Time-ordered runtime sequences (setup, timestep loop, data flow, rendering) as Mermaid diagrams -- no KA entry. Its last `Planned` subsection (checkpointing, Section 3) was built 2026-09-07 (TASK-045, Stage 8); Section 2's live-loop wiring went real earlier, 2026-08-28 (TASK-030). Deterministic replay and playback (TASK-046/047) are still unbuilt, but Section 3 now says so in prose rather than under a `Planned` heading. | +| sequences.md | 🟩 | Time-ordered runtime sequences (setup, timestep loop, data flow, rendering) as Mermaid diagrams -- no KA entry. Its last `Planned` subsection (checkpointing, Section 3) was built 2026-09-07 (TASK-045, Stage 8); Section 2's live-loop wiring went real earlier, 2026-08-28 (TASK-030). Deterministic replay and playback (TASK-046/047) were built the same day as TASK-045, and Section 3 covers all three; live scrub, checkpoint retention, subset cache reuse and combined declared-field playback (TASK-048/049/050/051) were added to it 2026-09-11. (This cell read "TASK-046/047 are still unbuilt" until 2026-09-11 -- stale from the day it was written, since both landed 2026-09-07 alongside the TASK-045 the same sentence credits; found by the Stage 8 exit audit.) | | compute-and-rendering-stack.md | 🟨 | Survey and compatibility matrix for array-library × renderer combinations; decision-support for the stack ADRs. Both questions it exists to support are decided: the class (A2b) via `ADR-004`, the instances (A2c, PyTorch + wgpu/pygfx) via `ADR-005`, both 2026-08-15. It remains the record of why, and of the options not taken. (This row read "not yet decided" for A2c until 2026-08-18 -- stale since the day it was written, since `ADR-005` landed the same day.) | `engine.md` and `icds.md` written 2026-08-17 (`docs/planning/backlog.md` diff --git a/src/pyflow/__main__.py b/src/pyflow/__main__.py index 221f312..ce30826 100644 --- a/src/pyflow/__main__.py +++ b/src/pyflow/__main__.py @@ -363,7 +363,25 @@ def main(argv: list[str] | None = None) -> None: "play", help="Open a window and render a checkpointed run between two " "frames, with live Space to pause/resume and +/- to change speed.", + # The controls block was **added 2026-09-11 by the Stage 8 exit + # audit**: pause, speed and scrub are what this subcommand is + # for, and `--help` listed none of them, so the only place a + # user could discover them was `README.md`. Seeking is bounded by + # the window `--from-frame`/`--to-frame` loaded, which the last + # line says outright rather than leaving a user to infer from + # Home/End not going any further. epilog=( + "window controls:\n" + " Space pause / resume\n" + " + / - double / halve playback speed\n" + " Left / Right step one frame back / forward\n" + " Home / End jump to the first / last loaded frame\n" + " drag scrub bar seek directly to any loaded frame\n" + " Escape / Enter close the window\n" + "\n" + " Seeking covers the loaded window only -- to reach frames\n" + " outside it, rerun with different --from-frame/--to-frame.\n" + "\n" "examples:\n" " pyflow play --checkpoints-dir checkpoints --to-frame 500\n" " pyflow play --checkpoints-dir checkpoints --from-frame 100 " diff --git a/tests/integration/CLAUDE.md b/tests/integration/CLAUDE.md index 4a83d48..5ce6bd1 100644 --- a/tests/integration/CLAUDE.md +++ b/tests/integration/CLAUDE.md @@ -73,3 +73,62 @@ short-circuits by making a monkeypatched `GlfwRenderCanvas` raise a bare `BaseException` that `except Exception` would not catch -- standing in for the real SIGABRT without actually crashing the test runner to prove it. + +**`test_import_order.py` gained a second, stronger check 2026-09-11** +(Stage 8 exit audit): `test_headless_module_never_reaches_the_renderer`. +The original test proves each module *imports cleanly* first, which is +the D4 circular-import bug it was written for -- it would pass just as +happily with `import pyflow.rendering` at the top of `recording.py`. +Stage 8's own Completion Criterion 1 needs the opposite claim (the +headless modules **cannot reach** the renderer, transitively), so the +new test reads `sys.modules` back out of the same fresh subprocess and +fails on any `pyflow.rendering`/`pygfx`/`rendercanvas`/`wgpu`/`glfw` +entry. **Add a module to `HEADLESS_MODULES` whenever a new module +belongs on the headless side**, the same standing obligation `MODULES` +above it already carries. + +**The display-guarded tests still run on Windows CI only, and that is a +recorded decision rather than an oversight.** The Stage 8 exit audit +found it (every check that stage's Completion Criterion 6 rests on is +display-guarded, so a green two-platform matrix was proving live-window +behaviour on one platform), tried to fix it, and reverted the fix. **Read +this before trying again, because both obvious approaches are already +known to fail:** + +1. **`xvfb` works and is flaky.** Installing it and running `xvfb-run -a + make ci` dropped Linux skips from 29 to 18, matching a local run, so + all 10 tests genuinely ran. Across three attempts on *identical* test + code, two passed and one crashed an xdist worker outright -- no Python + traceback, the hard process abort GLFW produces rather than an + exception -- and the crashed attempt then passed on a plain re-run. + Roughly a third of attempts, on a gating check. Working hypothesis, + never reproduced locally: 8 workers creating software-GL contexts + against one Xvfb display at once. +2. **Serialising the group onto one worker is worse, not better.** + `--dist loadgroup` plus an `xdist_group` mark hung the *Windows* job + for 5h45m until GitHub's own 6-hour limit killed it -- at 95%, ~6 + tests outstanding, an orphaned python process at cleanup. It also + passed `make ci` locally in 183s, so nothing short of CI would have + caught it. + +**The transferable fact, which both results point at from opposite +directions: what works is 1-2 GLFW windows per process, spread across +workers.** Approach 1 raised concurrent window creation across processes; +approach 2 raised sequential window creation within one process. Each +broke a different platform. A real fix probably has to reduce the number +of windows rather than redistribute them -- the two +`*_rerenders_the_field_in_real_pixels` tests below open two each, which +is the highest count here. See `docs/planning/backlog.md` for the open +item. + +**Comparing rendered pixels: never build the reference from the run +under test.** `test_playback_cli.py`'s two `*_rerenders_the_field_in_ +real_pixels` tests are the worked example, and their module comments +record the two versions that passed against a deliberately broken +renderer before the third one worked. The short form: a reference frame +captured during the same `play()` window is frozen by exactly the faults +you are hunting, so it can only prove the two code paths agree with each +other. Take the reference from a separate window launched at the target +frame, and crop out anything that moves for an unrelated reason (here, +the scrub bar). Full rule: `docs/practices.md`, "A reference the run +under test produced proves consistency, not correctness". diff --git a/tests/integration/test_import_order.py b/tests/integration/test_import_order.py index 1ceacc0..72e1a4e 100644 --- a/tests/integration/test_import_order.py +++ b/tests/integration/test_import_order.py @@ -49,3 +49,56 @@ def test_module_imports_cleanly_first(module: str) -> None: check=False, ) assert result.returncode == 0, result.stderr + + +# Stage 8 Completion Criterion 1 (`docs/planning/roadmap.md`): the +# headless-recording modules must not be able to reach the renderer at +# all -- not merely default to headless. TASK-046's own Acceptance +# Criteria state the same claim for `replay.py`. +# +# **Added 2026-09-11 by the Stage 8 exit audit.** Both criteria were +# true and neither was gated: Criterion 1 said in its own text that the +# claim "was checked by hand at implementation time", and +# `test_module_imports_cleanly_first` above proves only that each module +# imports without error -- it would pass just as happily with +# `import pyflow.rendering` at the top of `recording.py`. A by-hand +# check does not survive the next import somebody adds, which is exactly +# what this criterion exists to prevent. +HEADLESS_MODULES = [ + "pyflow.checkpoint", + "pyflow.recording", + "pyflow.replay", + "pyflow.simulation_run", +] + +# `pyflow.rendering` itself, plus the three third-party layers underneath +# it -- a module could pull in `pygfx`/`wgpu`/`glfw` directly without +# going through `pyflow.rendering`, and that would break this criterion +# just as thoroughly. +_RENDERING_PREFIXES = ("pyflow.rendering", "pygfx", "rendercanvas", "wgpu", "glfw") + + +@pytest.mark.parametrize("module", HEADLESS_MODULES) +def test_headless_module_never_reaches_the_renderer(module: str) -> None: + """A fresh subprocess, so `sys.modules` reflects this import alone. + + Checks the *transitive* closure, not the module's own import + statements: anything `recording.py` imports that itself imports + `rendering` would show up here, which is the form the criterion is + actually written in ("nor anything that transitively imports it"). + """ + probe = ( + f"import sys; import {module}; " + f"print([m for m in sys.modules if m.startswith({_RENDERING_PREFIXES!r})])" + ) + result = subprocess.run( + [sys.executable, "-c", probe], + capture_output=True, + text=True, + check=False, + ) + + assert result.returncode == 0, result.stderr + assert result.stdout.strip() == "[]", ( + f"{module} transitively imported a rendering module: {result.stdout.strip()}" + ) diff --git a/tests/integration/test_playback_cli.py b/tests/integration/test_playback_cli.py index 3a73023..f298850 100644 --- a/tests/integration/test_playback_cli.py +++ b/tests/integration/test_playback_cli.py @@ -76,6 +76,48 @@ def _record_cavity(output_dir: Path, *, max_frames: int, checkpoint_interval: in assert result.returncode == 0, result.stderr +def _record_cavity_without_stats( + tmp_path: Path, output_dir: Path, *, max_frames: int, checkpoint_interval: int +) -> None: + """Record the same cavity, but with `rendering.show_stats: false`. + + **Added 2026-09-11 by the Stage 8 exit audit**, for the pixel-level + scrub check below. The stats HUD rewrites the frame number into the + rendered image every single frame, so with it on, *any* two frames + at different positions hash differently whether or not the field + itself was ever rebuilt -- a full-frame hash would pass vacuously. + Turning it off leaves the field, the arrows and the scrub thumb as + the only things in the image, which is what the criterion is + actually about. + """ + config_text = Path("examples/golden-demos/lid_driven_cavity.yaml").read_text(encoding="utf-8") + config_file = tmp_path / "cavity_no_stats.yaml" + # `rendering:` is the last section in the golden demo, so appending + # one more key to it needs no YAML round trip. + config_file.write_text(config_text + " show_stats: false\n", encoding="utf-8") + + result = subprocess.run( + [ + sys.executable, + "-m", + "pyflow", + "record", + "--config", + str(config_file), + "--max-frames", + str(max_frames), + "--output-dir", + str(output_dir), + "--checkpoint-interval", + str(checkpoint_interval), + ], + capture_output=True, + text=True, + check=False, + ) + assert result.returncode == 0, result.stderr + + def test_play_renders_a_real_recorded_run_headlessly(tmp_path: Path) -> None: checkpoints_dir = tmp_path / "checkpoints" _record_cavity(checkpoints_dir, max_frames=20, checkpoint_interval=20) @@ -505,3 +547,270 @@ def _on_frame(window: RenderWindow) -> None: # `stop_propagation` kept `RenderWindow.run`'s pan handlers from ever # starting a pan for this gesture. assert found["camera_after"] == found["camera_before"] + + +# Stage 8 Completion Criterion 6's second bullet asks for the two scrub +# paths checked "against real rendered pixels, the same way Space's own +# pause already is". **The three helpers and two tests below were added +# 2026-09-11 by the Stage 8 exit audit**, which found that bullet marked +# Met against two tests (above) that assert only on `PlaybackState. +# position` and never call `snapshot()` at all. +# +# **Two false starts are recorded here because each one passed against a +# deliberately broken renderer, and the shape of the mistake is the +# useful part.** +# +# 1. *"The pixels changed after seeking"* proves nothing: the scrub +# thumb moves with the index, so any index change repaints something +# whether or not the field was rebuilt underneath it. +# 2. *"A seek-reached frame matches an autoplay-reached frame at the +# same index"* -- the obvious repair -- proves almost nothing either, +# and this one actually had to be mutation-tested to see it. Freezing +# the field (`_rebuild_arrows(0)`/`_rebuild_panels(0)`, thumb left +# working) leaves *both* sides of that comparison equally frozen, so +# they still match. A reference computed by the same run cannot +# detect a fault common to the whole run. +# +# So the reference comes from **a separate `play()` window launched at +# the target frame** (`_reference_field_hash`), where that frame is +# index 0 and therefore rendered by the initial scene build rather than +# by the seek path at all -- and the comparison is over the **field +# region only** (`_field_region_hash`), cropping away the scrub bar, +# which `playback.py` draws below `mesh_min_y` and which would otherwise +# reintroduce exactly the thumb-moved-so-pixels-changed confound from +# (1). Both tests were re-run against the same frozen-field mutation and +# confirmed to fail before being trusted green. +# +# One timing fact the helpers depend on: **`snapshot()` is one frame +# behind `position`.** `play()`'s own `_on_frame` advances the position, +# rebuilds the scene, and only then invokes this callback -- all before +# the canvas presents anything -- so the image read back while +# `position` reads N is the rendering of frame N-1. Established +# empirically by logging `(frame_count, position, hash)` against a plain +# autoplay run, not inferred from reading `playback.py`. +def _field_region_hash(window: RenderWindow, mesh_bounds: tuple[float, float, float, float]) -> str: + """Hash only the rows of `snapshot()` above the mesh's own bottom + edge -- the field and its arrows, never the scrub bar. + + `playback.py` puts the bar at `mesh_min_y - mesh_height * 0.08`, so + cropping at `mesh_min_y` excludes the bar and its thumb entirely + while keeping every pixel the field itself can touch. + """ + logical_width, logical_height = window.canvas.get_logical_size() + mesh_min_x, mesh_min_y, _mesh_max_x, _mesh_max_y = mesh_bounds + _screen_x, screen_y = _world_to_screen( + window.camera, logical_width, logical_height, mesh_min_x, mesh_min_y + ) + + image = np.asarray(window.renderer.snapshot()) + # `snapshot()` is in physical pixels and `_world_to_screen` returns + # logical ones, so convert through a fraction of the height rather + # than assuming the two are the same scale. + cut = int(round((screen_y / logical_height) * image.shape[0])) + cut = max(1, min(cut, image.shape[0])) + return _frame_hash(image[:cut]) + + +def _reference_field_hash( + checkpoints_dir: Path, frame: int, mesh_bounds: tuple[float, float, float, float] +) -> str: + """The field region of absolute `frame`, rendered by a *separate* + `play()` window that starts there. + + In that window `frame` is index 0, so it is drawn by the initial + scene build and never by a seek or an advance -- which is what makes + it an independent reference rather than a second reading of the same + possibly-broken path. + """ + captured: dict[str, str] = {} + + def _on_frame(window: RenderWindow) -> None: + if "hash" not in captured: + captured["hash"] = _field_region_hash(window, mesh_bounds) + window.canvas.submit_event({"event_type": "key_down", "key": "Escape"}) + + play( + checkpoints_dir, + from_frame=frame, + to_frame=frame + 1, + backend="glfw", + max_frames=_SCRUB_SAFETY_MAX_FRAMES, + on_frame=_on_frame, + ) + assert "hash" in captured, "reference window never rendered a frame" + return captured["hash"] + + +@_needs_a_real_display +def test_keyboard_seeking_rerenders_the_field_in_real_pixels(tmp_path: Path) -> None: + """ArrowLeft, through the real key wiring, against a real window.""" + checkpoints_dir = tmp_path / "checkpoints" + _record_cavity_without_stats(tmp_path, checkpoints_dir, max_frames=30, checkpoint_interval=30) + checkpoint = read_checkpoint(checkpoints_dir / "checkpoint_00000000.pt") + mesh_bounds = mesh_bounding_box(StructuredCartesianMesh.from_config(checkpoint.config.mesh)) + + step = _SCRUB_STEP_FRAMES + seen: dict[str, object] = {} + injected: set[str] = set() + + def _on_frame(window: RenderWindow) -> None: + assert window.playback_state is not None + index = int(window.playback_state.position) + frame = window.frame_count + + if frame == step and "pause" not in injected: + injected.add("pause") + window.canvas.submit_event({"event_type": "key_down", "key": " "}) + elif frame == 2 * step and "left" not in injected: + seen["paused_index"] = index + seen["paused_hash"] = _field_region_hash(window, mesh_bounds) + injected.add("left") + window.canvas.submit_event({"event_type": "key_down", "key": "ArrowLeft"}) + elif frame == 3 * step: + # One frame behind `position`, so the settled image here + # depicts `index`, which has not moved for a whole step. + seen["sought_index"] = index + seen["sought_hash"] = _field_region_hash(window, mesh_bounds) + window.canvas.submit_event({"event_type": "key_down", "key": "Escape"}) + + window = play( + checkpoints_dir, + from_frame=0, + to_frame=30, + backend="glfw", + max_frames=_SCRUB_SAFETY_MAX_FRAMES, + on_frame=_on_frame, + ) + + assert window.canvas.get_closed() + assert injected == {"pause", "left"}, injected + + paused_index = seen["paused_index"] + sought_index = seen["sought_index"] + assert isinstance(paused_index, int) and isinstance(sought_index, int) + assert sought_index == paused_index - 1, (paused_index, sought_index) + + # The weak half: the rendered field moved at all. Already stronger + # than a whole-frame hash, since the thumb is cropped out. + assert seen["sought_hash"] != seen["paused_hash"], ( + "ArrowLeft left the rendered field byte-identical" + ) + # The strong half: it is the genuine rendering of that frame, + # according to a window that never seeked. + assert seen["sought_hash"] == _reference_field_hash( + checkpoints_dir, sought_index, mesh_bounds + ), ( + f"ArrowLeft moved position to {sought_index} but rendered a different field " + "than a fresh window started at that frame" + ) + + +@_needs_a_real_display +def test_dragging_the_scrub_bar_rerenders_the_field_in_real_pixels(tmp_path: Path) -> None: + """The same check for the mouse path, through real pointer events.""" + checkpoints_dir = tmp_path / "checkpoints" + _record_cavity_without_stats(tmp_path, checkpoints_dir, max_frames=30, checkpoint_interval=30) + checkpoint = read_checkpoint(checkpoints_dir / "checkpoint_00000000.pt") + mesh_bounds = mesh_bounding_box(StructuredCartesianMesh.from_config(checkpoint.config.mesh)) + mesh_min_x, _mesh_min_y, mesh_max_x, _mesh_max_y = mesh_bounds + mesh_width = mesh_max_x - mesh_min_x + + step = _SCRUB_STEP_FRAMES + max_index = 30 + target_index = 21 + + seen: dict[str, object] = {} + injected: set[str] = set() + + def _on_frame(window: RenderWindow) -> None: + assert window.playback_state is not None + index = int(window.playback_state.position) + frame = window.frame_count + logical_width, logical_height = window.canvas.get_logical_size() + + if "bar_y" not in seen: + thumb = next(child for child in window.scene.children if isinstance(child, gfx.Points)) + _thumb_x, bar_y, _z = thumb.geometry.positions.data[0] + seen["bar_y"] = float(bar_y) + + bar_y = seen["bar_y"] + assert isinstance(bar_y, float) + target_x = mesh_min_x + (target_index / max_index) * mesh_width + + if frame == step and "pause" not in injected: + injected.add("pause") + window.canvas.submit_event({"event_type": "key_down", "key": " "}) + elif frame == 2 * step and "down" not in injected: + seen["paused_hash"] = _field_region_hash(window, mesh_bounds) + injected.add("down") + screen_x, screen_y = _world_to_screen( + window.camera, logical_width, logical_height, target_x, bar_y + ) + seen["screen_pos"] = (screen_x, screen_y) + window.canvas.submit_event( + {"event_type": "pointer_down", "x": screen_x, "y": screen_y, "button": 1} + ) + elif frame == 3 * step and "up" not in injected: + injected.add("up") + screen_pos = seen["screen_pos"] + assert isinstance(screen_pos, tuple) + screen_x, screen_y = screen_pos + window.canvas.submit_event( + {"event_type": "pointer_up", "x": screen_x, "y": screen_y, "button": 1} + ) + elif frame == 4 * step: + seen["dragged_index"] = index + seen["dragged_hash"] = _field_region_hash(window, mesh_bounds) + window.canvas.submit_event({"event_type": "key_down", "key": "Escape"}) + + window = play( + checkpoints_dir, + from_frame=0, + to_frame=max_index, + backend="glfw", + max_frames=_SCRUB_SAFETY_MAX_FRAMES, + on_frame=_on_frame, + ) + + assert window.canvas.get_closed() + assert injected == {"pause", "down", "up"}, injected + assert seen["dragged_index"] == target_index, seen["dragged_index"] + + assert seen["dragged_hash"] != seen["paused_hash"], ( + "the scrub drag left the rendered field byte-identical" + ) + assert seen["dragged_hash"] == _reference_field_hash( + checkpoints_dir, target_index, mesh_bounds + ), ( + f"the drag moved position to {target_index} but rendered a different field " + "than a fresh window started at that frame" + ) + + +def test_play_help_documents_every_window_control() -> None: + """`pyflow play --help` must name the controls the subcommand exists + for, and say where seeking stops. + + **Added 2026-09-11 by the Stage 8 exit audit**, which found pause, + speed and scrub -- everything TASK-047 and TASK-048 built -- absent + from `--help` entirely, discoverable only by reading `README.md`. + Root `CLAUDE.md`'s Feature Verification rule asks for exactly this + check ("confirm every new CLI flag actually appears in `--help`"); + the controls are not flags, but they are the same promise. + + The bounded-seek line is asserted too, because it is the one place a + user is told that Home/End stop at the loaded window rather than at + the recording's own ends -- Stage 8's own stated scope boundary, and + the thing most likely to read as a bug without it. + """ + result = subprocess.run( + [sys.executable, "-m", "pyflow", "play", "--help"], + capture_output=True, + text=True, + check=False, + ) + + assert result.returncode == 0, result.stderr + for control in ("Space", "+ / -", "Left / Right", "Home / End", "drag scrub bar"): + assert control in result.stdout, f"{control!r} missing from pyflow play --help" + assert "loaded window only" in result.stdout diff --git a/tests/integration/test_record_cli.py b/tests/integration/test_record_cli.py index ca25067..93e19e7 100644 --- a/tests/integration/test_record_cli.py +++ b/tests/integration/test_record_cli.py @@ -216,3 +216,48 @@ def test_resume_requires_checkpoint_or_config_and_max_frames() -> None: assert result.returncode != 0 assert "--checkpoint" in result.stderr + + +def test_record_prunes_a_multi_hundred_frame_run_through_the_real_cli(tmp_path: Path) -> None: + """Stage 8 Completion Criterion 8 asks for the retention policy + checked "against a real multi-hundred-frame `pyflow record` run, + not only against the pruning function in isolation". + + **Added 2026-09-11 by the Stage 8 exit audit**, which found that + clause marked Met against a 20-frame run here and a 25-frame one by + hand -- the substantive behaviour was right, but no run at the scale + the criterion names had ever happened. 300 frames at `interval=5` + writes 61 checkpoints, which is the scale at which "bounded total" + means something distinct from "bounded interval". + """ + output_dir = tmp_path / "checkpoints" + + result = subprocess.run( + [ + sys.executable, + "-m", + "pyflow", + "record", + "--config", + "examples/golden-demos/heat_diffusion.yaml", + "--max-frames", + "300", + "--output-dir", + str(output_dir), + "--checkpoint-interval", + "5", + "--max-checkpoints-retained", + "3", + ], + capture_output=True, + text=True, + check=False, + ) + + assert result.returncode == 0, result.stderr + # 61 written, 4 kept: frame 0 (never pruned) plus the newest three. + assert "recorded 61 checkpoint(s)" in result.stderr or "61 checkpoint(s)" in result.stdout + remaining = { + int(p.stem.removeprefix("checkpoint_")) for p in output_dir.glob("checkpoint_*.pt") + } + assert remaining == {0, 290, 295, 300} diff --git a/tests/unit/test_recording.py b/tests/unit/test_recording.py index 23a2ee3..bda4418 100644 --- a/tests/unit/test_recording.py +++ b/tests/unit/test_recording.py @@ -6,11 +6,12 @@ from __future__ import annotations from pathlib import Path +from typing import Any import pytest import torch -from pyflow.checkpoint import read_checkpoint +from pyflow.checkpoint import read_checkpoint, write_checkpoint from pyflow.recording import NothingToRecordError, NothingToResumeError, record, resume _DECLARED_FIELD_CONFIG = """\ @@ -378,3 +379,53 @@ def test_resume_rejects_both_checkpoint_path_and_config_path(tmp_path: Path) -> with pytest.raises(ValueError, match="checkpoint_path.*config_path"): resume(output_dir / "checkpoint_00000003.pt", config_path=config_file, max_frames=6) + + +def test_retention_bounds_disk_use_throughout_a_long_run_not_only_at_the_end( + tmp_path: Path, monkeypatch: pytest.MonkeyPatch +) -> None: + """`_advance_and_checkpoint`'s own docstring claims the cap bounds + disk usage "as the recording grows, not only once it finishes" -- + which the final file set cannot prove either way, since pruning once + at the very end leaves exactly the same files behind. + + So this watches the directory *during* the run, after every single + checkpoint write, and pins the peak. **Added 2026-09-11 by the Stage + 8 exit audit**, which found that claim asserted in a docstring and + checked nowhere: a 300-frame run at `interval=5` writes 61 + checkpoints, so an implementation that pruned only at the end would + hold 61 files at its peak and still pass every other retention test + in this module. + """ + peak = 0 + + def _counting_write_checkpoint(path: Path, **kwargs: Any) -> None: + nonlocal peak + write_checkpoint(path, **kwargs) + peak = max(peak, len(list(path.parent.glob("checkpoint_*.pt")))) + + # Patched by name on `pyflow.recording`, which is where `record` + # resolves it -- patching `pyflow.checkpoint.write_checkpoint` + # instead would leave `recording.py`'s own from-import untouched. + monkeypatch.setattr("pyflow.recording.write_checkpoint", _counting_write_checkpoint) + + config_file = tmp_path / "config.yaml" + config_file.write_text(_DECLARED_FIELD_CONFIG) + output_dir = tmp_path / "checkpoints" + + result = record( + config_file, + max_frames=300, + output_dir=output_dir, + checkpoint_interval=5, + max_checkpoints_retained=3, + ) + + # 61 checkpoints genuinely written (frame 0 plus every multiple of 5 + # up to 300) -- so the peak below is a real bound being enforced + # mid-run, not an artefact of few files ever existing. + assert len(result.checkpoint_frames) == 61 + # Frame 0 (never pruned) + the cap of 3 + the one just written before + # pruning runs = 5. Never the 61 an end-only prune would reach. + assert peak <= 5, f"disk use peaked at {peak} checkpoints, not bounded during the run" + assert _checkpoint_frames_on_disk(output_dir) == {0, 290, 295, 300}