Bound a solver grind: wall-clock guard inside PETSc's convergence tests, and a sub-solve work gauge - #442
Conversation
A hard viscoplastic Stokes solve does not fail, it grinds -- for hours, inside a single Newton step. Two measurements say why nothing outside PETSc can stop it: an iteration cap never fires because the grind happens within one outer Krylov iteration, so the counter it watches does not advance; and a Python signal.alarm never fires because Python runs signal handlers only between bytecodes while control sits in PETSc's C code (a 90s alarm measured unfired at 10 minutes). So the deadline lives in PETSc's own convergence tests. solver.guard( wall_per_step=...) restarts a budget at each outer KSP solve -- once per Newton step -- and checks it inside the fieldsplit sub-KSP tests, where iterations are frequent enough for seconds of granularity. A guard exit is a report, not an error: solve_report.deadline_expired distinguishes it from a real divergence. The reason returned matters and is easy to get wrong. PETSc's KSPCheckSolve deliberately exempts KSP_DIVERGED_ITS from marking the preconditioner failed, so a guard returning DIVERGED_MAX_IT from a sub-KSP does nothing at all -- measured, the outer solve absorbed 36 truncated velocity solves and reported CONVERGED. DIVERGED_BREAKDOWN marks the sub-PC failed and surfaces as DIVERGED_LINEAR_SOLVE. In parallel the expiry decision is reduced over the solver's communicator: wall clocks are not synchronised, and a convergence test returning different verdicts on different ranks deadlocks the next collective. ptest_0203 provokes this with deliberately skewed per-rank clocks -- it passes at np=2 and np=4, and deadlocks when the reduction is removed (confirmed at a 90s limit). Also adds the work axis the guard needs. solve_report.ksp_its counts OUTER Krylov iterations, which Eisenstat-Walker collapses to about one per Newton step; measured on a small box, outer 1 against 285 multigrid cycles in the velocity block. solve_report.sub records iterations and applications per fieldsplit block, collected by monitors so it cannot influence a solve. Cost of leaving it on: 624 callbacks in a 280ms solve, no measurable change in wall time. Known and documented rather than hidden: on a solver's first solve the fieldsplit blocks do not exist until PCSetUp runs part-way through, so one preconditioner application is uncovered and that solve's sub-counts are a lower bound (SubSolveReport.complete says so). guard() raises on the rotated free-slip path, which solves outside self.snes and cannot be reached. Underworld development team with AI support from Claude Code
…guard Two independent reviewers were given the change and told to break it. Four of the findings were real defects in the new code and are fixed here; two more were claims in the docs that measurement did not support. MAJOR - the deadline was disabled for the first preconditioner application of EVERY solve, not just the first. begin_solve() left the clock at infinity and only outer-KSP iteration 0 armed it -- but PETSc's default for GMRES is LEFT preconditioning, and KSPInitialResidual applies the preconditioner BEFORE iteration 0. Verified: for pc_side LEFT the sub-block monitors fire before the outer monitor. So one full velocity solve plus one pressure solve ran outside the budget on every solve. The clock now starts in begin_solve(). MAJOR - the premise that forced a hand-rolled convergence test was false. petsc4py DOES expose composition: KSP.addConvergenceTest(prepend=True) runs a test in FRONT of the native one, and returning ITERATING hands the decision back to KSPConvergedDefault with its options-configured context intact. Verified: a prepended always-ITERATING test gives identical iteration count and reason to an uninstrumented solve. _default_convergence is deleted, and with it four silently dropped KSPConvergedDefault behaviours (-ksp_converged_maxits, the non-zero-initial-guess residual convention, the norm-type early return, -ksp_min_it) and a bug reporting an infinite residual as CONVERGED_RTOL. It also makes unguard() exact: the test stays installed and goes inert, which IS the uninstrumented behaviour, rather than being swapped for a fresh default context. MAJOR - the instrumentation held petsc4py references to a destroyed SNES's KSP, PC and multigrid hierarchy until the next solve, so a rebuilding solver carried two hierarchies at once. That is the leak BUGFIX(#157) was written to close. It now releases them before the destroy. MAJOR - guard()'s rotated-free-slip refusal was order-dependent: arming first and adding the BC afterwards armed cleanly and then sat inert on a path that never reaches the instrumentation. Re-checked at solve time. MINOR - preonly and richardson outer KSPs are now skipped: both change what they DO when a monitor is attached (gated on ksp->numbermonitors in PETSc), and neither iterates, so instrumenting them was all cost and no information. MEASURED, NOT FIXED - a reviewer argued a retry or a continuation stage would be handed a fresh budget after an expiry. PETSc prevents this on its own: once the sub-preconditioner is marked failed the next snes.solve bails before iterating, so removing the expiry latch changed the cost by 34 ticks against 32. The latch is kept because that protection is implicit, but it is belt-and-braces and no test claims to prove otherwise. Tests reworked against the second reviewer's demonstration that several of them passed with the feature broken. Now covered: cost parity (not just answer parity) between a guarded and an unguarded solve; the per-Newton-step meaning of wall_per_step, on a nonlinear model, since one linear solve cannot distinguish per-step from per-solve; the first-solve lower-bound contract; arming before the first solve; both call orders of the rotated refusal. The suite is tier_b, not tier_a: Charter 8 reserves tier A for tests that have been through use in anger. The parallel test is registered in mpi_runner.sh -- it was unreachable by any runner before, so the hazard the design calls severe had no automated coverage. Underworld development team with AI support from Claude Code
Underworld development team with AI support from Claude Code
🔍 Architectural Review ChecklistThank you for submitting an architectural review! Reviewers should validate: Design & Architecture
Implementation
Testing & Validation
Documentation
Review Process: See CODE-REVIEW-PROCESS.md Approval: This PR merge = Review formally approved |
|
This is the adversarial review this branch was put through, posted in full per the standing rule that reviews are public. Two independent reviewers, each given one dimension and told to break the change rather than praise it. Everything below is fixed on the branch except where explicitly marked as not sustained by measurement. Adversarial review — wall-clock guard and sub-solve work gaugeBranch Method: two independent adversarial reviewers, each given one dimension and instructed to Verdict: not ready as submitted. Four major defects in the new code, two false claims MAJOR — the deadline was disabled for the first preconditioner application of every solve
So one complete velocity-block solve plus one pressure-block solve ran entirely outside Fixed: the clock starts in MAJOR — the premise that forced a hand-rolled convergence test was falseBoth the code and the design note asserted that "a custom convergence test replaces the Verified here before acting on it:
Every divergence the reviewer then listed was a self-inflicted consequence of Fixed: MAJOR — the instrumentation re-opened the leak
|
|
✅ Test Suite: success |
The problem
A viscoplastic Stokes solve near its conditioning floor does not fail. It grinds — for
hours, inside a single Newton step, with no error and no sign of stopping. Any study that
sweeps a parameter plane hits this immediately, because the interesting cells are exactly
the unreachable ones.
Two obvious defences were tried first and both were measured to not work:
An iteration cap does not bound wall time. The grind happens within one outer Krylov
iteration, so the counter any cap watches never advances. Measured: 3814 s inside one
outer iteration. Nor can caps be tuned around it — tight caps misclassify in both
directions (a continuation march settles at its starting parameter and reports success;
a step that would have converged in fifteen iterations is called unreachable at ten),
loose caps classify correctly and run past ten minutes a station. The number of iterations
a feasible step needs is not knowable in advance, which is exactly why a time budget has
to be an independent guard.
A Python timer does not fire.
signal.alarmschedules a handler, but Python runshandlers only between bytecodes, and during the grind control is inside PETSc's C code the
whole time. Measured: a 90 s alarm still unfired at 10 minutes.
So the only code that runs during a grind is PETSc's own, and that is where the deadline
has to live.
What this adds
A guard exit is a report, not an exception: the current iterate stays in the fields so a
continuation driver can revert or retry from it.
The clock starts at the beginning of each solve and restarts at each outer KSP
iteration 0, which is once per Newton step — so no SNES hook is needed. Starting it at
the beginning of the solve is not cosmetic: PETSc defaults GMRES to left preconditioning,
and
KSPInitialResidualapplies the preconditioner before iteration 0, so a clock armedonly at iteration 0 leaves one full velocity solve plus one pressure solve outside the
budget on every solve.
The deadline is checked inside the fieldsplit sub-KSP tests, which is where the
granularity is. In a representative solve the outer KSP performed 1 iteration while the
velocity block performed 285; only the sub-KSP tests can interrupt anything finer than a
whole Newton step.
The guard runs in front of PETSc's own test, not instead of it.
KSP.addConvergenceTest(..., prepend=True)composes with whatever native test the KSP isconfigured with, so returning
ITERATINGhands the decision straight back toKSPConvergedDefaultwith its options-configured context intact. Verified: a prependedalways-
ITERATINGtest gives identical iteration count and reason to an uninstrumentedsolve. That is also what makes
unguard()exact — PETSc cannot remove an added test, soit stays installed and goes inert, which is the uninstrumented behaviour.
The part that is easy to get wrong
PETSc's
KSPCheckSolvedeliberately exemptsKSP_DIVERGED_ITSfrom marking thepreconditioner failed — truncating an inner solve at its iteration cap is normal
behaviour, not an error. A guard that returns
DIVERGED_MAX_ITfrom a sub-KSP thereforedoes nothing at all. Measured: the outer solve absorbed 36 truncated velocity solves and
reported CONVERGED. The earlier prototype in
~/+Simulations/spiegelman_hardcaseusedexactly that reason, so its wall-time cap was weaker than it appeared.
DIVERGED_BREAKDOWNmarks the sub-preconditioner failed, which surfaces asDIVERGED_PC_FAILEDat the outer KSP andDIVERGED_LINEAR_SOLVEat the SNES.Parallel
Wall clocks are not synchronised across ranks, and a PETSc convergence test that returns
different verdicts on different ranks leaves the ranks on different code paths — the next
collective deadlocks. The expiry decision is therefore reduced over the solver's
communicator. That is well posed because Krylov iterations are globally synchronised, so
every rank reaches the same check the same number of times.
ptest_0203does not take this on trust. It provokes the hazard with deliberately skewedper-rank clocks (rank 0 fast, the others effectively frozen), so the ranks would reach
opposite verdicts if each decided alone. It passes at np=2 and np=4 — and deadlocks when
the reduction is removed, confirmed against a 90 s
mpirun --timeout.Extra reductions are paid only when a guard is armed. An unarmed solver installs no
convergence tests at all.
The work axis (needed before any of this means anything)
solve_report.ksp_itscounts outer Krylov iterations, which Eisenstat–Walker collapsesto about one per Newton step. It is not a measure of cost, and using it as one understates
a Stokes solve by two orders of magnitude — measured, outer 1 against 285 multigrid cycles
in the velocity block.
solve_report.subrecords iterations and applications per fieldsplit block. For thevelocity block under
pc_type=mgthat count is the multigrid cycle count. It iscollected by monitors, which observe and never decide, so it cannot change a solve.
Cost of leaving it always on: 624 monitor callbacks in a 280 ms solve, with no measurable
change in wall time (medians 282 ms instrumented against 284 ms not, run alternately;
run-to-run noise exceeds the difference).
Limitations, made honest rather than hidden
PCSetUp, which runspart-way through the first
KSPSolve. So that solve's sub-counts are a lower bound(measured: about half, because left-preconditioned GMRES applies the preconditioner once
in
KSPInitialResidualbefore the guard can attach).SubSolveReport.completeisFalseto say so. The deadline is unaffected — the clock runs from the start of thesolve. A driver wanting exact work should do one cheap solve before arming, which it
usually does anyway to pay for the JIT compile.
guard()raises there, matchingestimate_difficulty(): thatpath runs its own Krylov loop outside
self.snes, so a guard would attach and neverfire — protection in appearance only.
preonlyandrichardsonouter KSPs are skipped entirely: both change what theydo when a monitor is attached (they are gated on
ksp->numbermonitorsin PETSc), andneither iterates, so instrumenting them would be all cost and no information.
Review
Two independent adversarial reviewers were given this branch and told to break it. They
found four major defects in the new code — including a deadline that was disabled for
the first preconditioner application of every solve, and instrumentation that re-opened
the memory leak
BUGFIX(#157)closed — plus a test suite that one of them demonstratedwould pass with several parts of the feature broken. All are fixed; the full report is in
docs/reviews/2026-07/solver-wall-clock-guard-adversarial-review.mdand reproduced as acomment on this PR.
One reviewer finding is recorded as not sustained: they argued a retry or continuation
stage would be handed a fresh budget after an expiry. Measurement says PETSc prevents this
itself (34 clock ticks against 32 with the expiry latch removed), so the latch is kept as
belt-and-braces and no test claims to prove it.
Testing
tests/test_0203_solver_wallclock_guard.py— 10 tests, tier_b. Each verified to failwithout its feature by neutering the implementation: the deadline, the counting, and the
per-Newton-step re-arming each break a distinct set.
solve — a change confined to the sub-blocks moves the cost and not the answer.
wall_per_stepis exercised on a nonlinear model, because onelinear solve cannot distinguish per-step from per-solve.
ticking clock rather than a seconds budget. A budget in seconds cannot express "expire
during the block solves" on an unknown machine — too large and the solve finishes, too
small and it expires at the outer iteration before the blocks are reached. Both
real-budget versions were fixture-dependent and flaky before being replaced.
tests/parallel/ptest_0203_wallclock_guard_parallel.py— np=2 and np=4, registered intests/parallel/mpi_runner.sh. It hangs rather than fails if the reduction is dropped,so it must be run under a timeout.
level_1across all tiers: 966 passed. (Tier A alone is not a sufficient gate for achange that touches solver behaviour — that lesson came from this session.)
Design note
docs/developer/design/solver-wall-clock-guard.md.Underworld development team with AI support from Claude Code