Repository navigation
Conversation
One weekend campaign ran 27.6 h on a single `n = 4096` cell -- the serial `gram_residual` inside `svd_thin`'s check -- and finished with nothing to show for it: there was no per-cell limit, so nothing cut it off, and the run's CSV is written once at the end, so even the cells that had finished left no row. A cell that cannot finish and a cell that was never measured looked exactly alike. `run` and `verify` now take `--cell-timeout SECS` (`FBENCH_CELL_TIMEOUT`, default 3600, `0` disables). The clock covers one cell -- one `Op`, one CSV row -- is armed before it and disarmed after it, and is polled by the loops the harness owns: the timing loop, and the serial reference math a check runs (`shared::gram_residual`, `gram_adj_residual`, `mul`, `mul_adj_b`), which is where this cell spent its time. A cut cell is recorded as `status=timeout` with the limit in its note and the run continues, so it is a row and not a hole, and it is not confused with a wrong answer. A library call already in progress cannot be interrupted; the cell is cut at the next poll point. The check's cost is now in the record: `run` and `verify` write two appended columns, `timed_ms` (the timed calls, summed) and `check_ms` (the check that runs once after them, outside the clock). Appended, so the declared columns readers require are untouched. Measured at `n = 512` on the four `svd_thin` cells: 108, 98, 99 and 98 ms of timed region against 1667, 1672, 969 and 971 ms of check -- the check was 15x the measurement and no row said so. Red, before (release build, `--n 512,1024 --threads 1 --reps 1`): the four `n = 512` cells finish in ~5.8 s, the `n = 1024` check runs on, and an outer `timeout 60` kills the process (rc 124) with no CSV at all. Green, after, the same invocation with `--cell-timeout 20`: eight rows, the four `n = 512` cells `ok` with both times, the two `n = 1024` C-order cells `timeout 20`, and the two F-order cells after them still measured.
62b7e18 to
8c659f1
Compare
GPT-6.1-sol adversarial reviewCRITICAL: none Critical (must not merge)none Needs changes
Unverified claims
Checked and finenone Not checked: the full release/vendor matrix or publication pipeline end to end; timeout enforcement remains bypassable in the reproduced paths. Reviewed head: |
|
Review items addressed in
What I could not verify, and why: the harness does not build from the committed |
The committed `Cargo.lock` was a path-patched resolution -- `t4a-faer` and `t4a-faer-traits` carried no `source`, because it was written by a measured build that patched them to local checkouts -- so `cargo build --locked` refused it before compiling anything. Re-resolve it from the registry at the pinned fork's version (0.28.0). With a real resolution the harness still did not compile, but not because `backend.rs` is behind the API: the default features selected the pre-0.25 fork generation (`Par` is `Copy`, `Par::rayon`, workspace queries take a `Par`) against a crate that has the `ParShape` + team API, and that is the 21 errors. The newest-generation branches compile unchanged against `t4a-faer` 0.33.0, so the defaults now name the generation the locked crate has; a measured build reads it from the checkout it links and passes `--no-default-features` plus its own detection, which leaves its feature set byte-identical to before. CI gains one job, `harness`, that runs `cargo build --locked` against the registry arm -- the one configuration nobody records, because a recorded build links the checkouts by path. `--locked` is the point: the lock is what a reader gets, so a lock the harness cannot compile against fails there and not nowhere. tests/test_harness_build.py pins the three parts: the lock is a registry resolution, the defaults are the generation the locked fork has, and the measured build does not inherit them. Two documents stop claiming the harness does not exist and that CI has no build job.
GPT-6.1-sol adversarial reviewCRITICAL: none Critical (must not merge)none Needs changes
Unverified claims
Checked and fine
Not checked: full Cargo/vendor suites or real campaign reruns; the reproduction cases above are source-derived, not full-harness executions. Reviewed head: |
GPT-6.1-sol adversarial reviewCRITICAL: none Critical (must not merge)none Needs changes
Unverified claims
Checked and fine
Reviewed head: |
|
Review at
Each will carry an executed counterexample (the probes in the review are the shape I will use), and |
Closes part of #57: the per-cell limit (item 1) and the check's time in the record (item 3). Item 2 — whether the orthogonality check samples instead of running the full
O(n^3)product — is the maintainer's call and is not in this PR. The verification method is unchanged.What happened, and what this fixes
The weekend CPU campaign ran 27.6 h on one
n = 4096cell:fbench::shared::gram_residual, the serial triple loop insidesvd_thin's check. Two things made that worse than slow:pinned.shinvokes, so nothing cut it off andmaster.shwaited for aDONEthat never came;fbenchwrites its CSV once, at the end ofmain, so the cells that had already finished left no row either. "Not measured" and "hung" were the same thing in the run directory.1.
--cell-timeout SECS--cell-timeoutwins overFBENCH_CELL_TIMEOUT, which wins over the default;0disables it. The campaign needs no recorder change, and a host or pilot can lower it without editing the invocation the recorder composes.Op, one CSV row: armed before the cell, disarmed after it, so a cut ends that cell only and the loop continues. The rows after a cut are still measured (master.shis not the only thing that used to stop).shared::gram_residual,gram_adj_residual,mul,mul_adj_b.gram_residual's outer loop is exactly where the 27.6 h went.thread_local!(a cell runs on one thread from its firstprepareto its check) — no atomics, no watchdog thread, no signal, nothing to clean up on the measured cores.status=timeout, notetimeout 3600. Notfailed, because a cell lost to the limit and a cell caught giving a wrong answer are not the same event. Every existing reader treats anything other thanok/skippedas "no number", andtiming_evidencerefuses to publish a run that carries one, so a timeout cannot be published as a measurement. Inverifyit printsTIMEOUT <cell>instead ofMISMATCH <cell>.Limit of the mechanism, stated rather than hidden: a single library call already in progress cannot be interrupted. The cell is cut at the next poll point, and every loop that can grow without bound has one. A cell that blocks inside one call for an hour is cut an hour late, not never.
2.
timed_msandcheck_msTwo appended columns, in both
runandverify(appended after theprofilecolumn that #56 added, so nothing that already read the header moves): the timed calls summed, and the check that runs once afterwards outside the clock. Appended so the columnsbenchmarks/campaign.yamldeclares and theprofilecolumn (whichreport_run.read_rowsrequires andtiming_evidencereads) are untouched;python3 scripts/summarize_samples.pyreads the new files unchanged.Measured,
n = 512, 1T, the foursvd_thincells:timed_ms110, 99, 99, 99 againstcheck_ms1651, 1653, 968, 966. The check was 15x the measurement and no row said so.Red -> green, measured
Red, before this commit (release build,
fbench run --threads 1 --n 512,1024 --family svd_thin --dtype f64 --rows faer-fork --reps 1 --prime-ms 0), under an outertimeout 60:The four
n = 512cells finish in ~5.8 s; then = 1024check runs on; the process is killed; no CSV at all.Green, after, the same invocation with
--cell-timeout 20:rc 1 (a cell failed, nothing is publishable). The cut cells are rows, and the F-order
n = 1024cells after them are measured — that is the "cut one cell, keep going" requirement, end to end on the real pathological path at a size that costs a minute instead of a weekend.fbench verifyon the same grid writes the same two columns in its census CSV and printsTIMEOUTfor the two cut cells,all comparisons within toleranceabsent, rc 1 — which is what makes the recorder stop rather than publish.Checks run
cargo test --release -p fbench --features fork-par-shape,fbench/fork-team(patched to the localextern/checkouts): 10 passed, 0 failed — including two new tests: a fake cell whose check sleeps past the limit comes backtimeoutwith the following cellok, and the check's time is reported apart from the timed calls.cargo clippyon the same configuration: no new warnings.rustfmt --checkon the three touched files only (the rest of the tree is not rustfmt-clean and is left alone).python3 -m unittest discover -s tests -v(5 passed),python3 -m campaign_toolkit.declarations(ok).01a1cb8: the two new columns now follow theprofilecolumn fix(record_run): refuse a host the declared profile does not describe (#55) #56 added; all of the above is re-run at8c659f1.scripts/summarize_samples.pyon the new CSV (reports the timeout row as a failed measurement, as it always did for any non-okstatus) andreport_run.read_rows' declared-column check againstbenchmarks/campaign.yaml(no missing column; the new two are ignored extras).Design
docs/reviews/20261109-cell-timeout.md, and the harness contract in docs/results.md now names the flag, the status and the two columns.
Not in this PR: making the orthogonality check proportional (#57 item 2), and writing the CSV incrementally so a killed process still leaves the rows it finished — both are separate decisions.
The harness builds from the committed lock, and CI now says so (#59)
cargo build --lockeddid not work at this branch, and nothing builtfbenchin CI, so it had gone unnoticed. Two separate faults:t4a-faerandt4a-faer-traitscarried nosourceline — it was written by a measured build that patched them to local checkouts — so--lockedrefused it before compiling:error: cannot update the lock file ... because --locked was passed.>=0.24, <1.0picks 0.33.0, and the default features still selected the pre-0.25 generation (ParisCopy,Par::rayon, workspace queries take aPar). That, not the API, is the 21 errors: withfork-par-shape+fork-teamthe samebackend.rscompiles unchanged against 0.33.0.So: the lock is re-resolved from the registry at the pinned fork's version (0.28.0), the two generation features are the default (the registry arm has no checkout to read a generation from), and a measured build keeps reading the generation from its checkout —
scripts/build_for_faer_rev.shnow passes--no-default-featuresplus its own detection, which leavesBUILD_FEATURESbyte-identical to before (fbench/link-mkl,fbench/fork-par-shape,fbench/fork-teamfor the pinned checkout).CI gains one job,
harness, runningcargo build --locked(57 s, green here). It is not a measurement gate: it builds the one configuration nobody records.--lockedis the point — the lock is what a reader gets, so a lock the harness cannot compile against fails there and not nowhere.Red, measured locally with the lane's own command, before this commit:
Green, after:
cargo build --locked→ 0 errors;cargo test --locked→ 13 passed, 0 failed; the campaign build for the pinned revisions325e482/0539947f(MKL arm) → exit 0.Not measured: the hosted lane on a rotten lock (only the local runs above were made with that state), and any number — nothing here was timed.
review: gpt-6.1-sol — 1 IMPORTANT, verified pre-existing and left unfixed (
fbench/src/backend.rs:45,49: the pre-0.25 aliases spellPar<'static>, which 0.24 declares without a lifetime; that arm is not reachable from any lock, since resolving a pre-0.26 crate fails onfaer_fork/threadteam— report it as its own change), plus its one delete-list item applied (AGENTS.mdno longer says the harness is unwritten).