Adapt-review fixes: collective pinned-band verdicts, partition-independent pinning, explicit-GAMG opt-out, the #492 CI disarm - #530
Merged
Conversation
…on every rank label_interface_band raised rank-locally when its own subdomain held no band cell. A rank the surface never enters legitimately has an empty local band — only a GLOBALLY empty band is a user error. With the rank-local raise, a corner-confined surface at np=4 left three ranks raising while the fourth entered the collective mover and hung to the 300 s timeout (measured 2026-08-06, review of PR #488). Now the pinned count is allreduced, the error is raised on every rank only when the band is globally empty, and the DMLabel is created on every rank — including band-less ones — so downstream consumers see a symmetric label. Underworld development team with AI support from Claude Code
…es by coordinate The straddle test and the halo-ring growth both walk rank-local cells, and cells are partitioned disjointly, so a shared vertex whose cut (or ring) cell lives on the neighbour rank was pinned there but not on the owner. The mover then moved the owned copy and the neighbour's pinned copy followed through the SF: measured at np=4 (review of PR #488, 2026-08-06), two pinned leaves moved 4.2e-3 and 1.9e-3 while np=2/3 passed on partition luck. The pinned set is now synchronised across ranks by rounded coordinate after the core band and after each halo ring. The parallel test gains two guards for the band-less rank np=4 creates: getStratumIS on a strata-less DMLabel is a segfault (#291), and max() of a zero-size moved array raises rank-locally and desyncs the collectives that follow. Underworld development team with AI support from Claude Code
…thout crashing build_transfers now stands down — returning (None, None), never bare None — when the solver carries an explicit preconditioner="gamg" or a latched _pc_user_override (the user wrote their own pc_type into petsc_options). Before this gate, the opportunistic mesh-owned pickup silently clobbered the explicit choice back to the custom-P PCMG at solve time: both arms of test_0842's fmg-vs-gamg comparison ran pc_type=mg, so a user could not opt out and the comparison was vacuous. Both call sites (auto_inject_custom_mg and the rotated free-slip _build_rotated_custom_Pl) guard the unpack so a "no hierarchy" answer degrades to the default preconditioner instead of a TypeError mid-solve — a bare return inside this gate is exactly what the #488 x #471 merge once shipped. Underworld development team with AI support from Claude Code
… real Both MeshVariables are created before any solver runs: creating one after a solve rebuilds mesh.dm under the custom-MG coarse/fine links (issue #492), and that dangling reference is what segfaulted Linux CI downstream of this test. Each arm now asserts the PC type it actually ran (mg vs gamg), so the comparison can never silently become FMG-vs-itself again. The tolerance pair (ksp_rtol 1e-9, nodal bound 1e-7) is chosen to catch the once-shipped true-residual stall signature; the geometric bundle now owns its fgmres outer pairing (#514/#515), so the fmg arm's rtol is enforced in the true residual norm. Underworld development team with AI support from Claude Code
Underworld development team with AI support from Claude Code
Contributor
There was a problem hiding this comment.
Pull request overview
This PR addresses several parallel correctness and solver-configuration defects uncovered in the adapt/relax and custom multigrid paths, with targeted hardening in both library code and regression tests to prevent hangs, unintended vertex motion, and CI-only crashes.
Changes:
- Make
Mesh.label_interface_band()collective-safe (global emptiness check) and partition-independent (synchronise pinned vertices across ranks). - Ensure explicit GAMG choices opt out of opportunistic custom-MG pickup, and harden call sites against “no hierarchy” results.
- Harden parallel/CI diagnostics and tests (
ptest_0845,test_0842,PYTHONFAULTHANDLER=1) to avoid segfault traps and improve crash visibility.
Reviewed changes
Copilot reviewed 6 out of 6 changed files in this pull request and generated 2 comments.
Show a summary per file
| File | Description |
|---|---|
src/underworld3/discretisation/discretisation_mesh.py |
Collective pinned-band emptiness check + cross-rank synchronisation of pinned vertices; always create the DMLabel on all ranks. |
src/underworld3/utilities/custom_mg.py |
Explicit GAMG / user-override opt-out for mesh-owned custom-MG pickup; harden injection path. |
src/underworld3/utilities/rotated_bc.py |
Guard rotated custom-MG build path so “no hierarchy” degrades safely instead of erroring. |
tests/test_0842_nvb_3d_parallel_adapt.py |
Hardened regression to avoid #492 DM rebuild trap; assert the PC type actually used; adjusted tolerances. |
tests/parallel/ptest_0845_relax_pinned_band_parallel.py |
Avoid segfault-prone DMLabel stratum queries on band-less ranks and avoid max() on empty selections. |
scripts/test.sh |
Enable PYTHONFAULTHANDLER=1 so CI crashes emit Python tracebacks. |
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
Comment on lines
+910
to
915
| # Same None discipline as auto_inject_custom_mg: the contract is a 2-tuple, | ||
| # but "no hierarchy" must degrade to the default preconditioner, never raise. | ||
| resolved = custom_mg.build_transfers(solver, field_id=0) | ||
| h, Ps = resolved if resolved is not None else (None, None) | ||
| if h is None or Ps is None: | ||
| return None |
Comment on lines
+1123
to
1126
| resolved = build_transfers(solver, field_id=field_id) | ||
| h, Ps = resolved if resolved is not None else (None, None) | ||
| if h is None or Ps is None: | ||
| return |
Member
Author
|
CI note for the record: the test job's 4 failures ( Underworld development team with AI support from Claude Code |
This was referenced Aug 12, 2026
lmoresi
added a commit
that referenced
this pull request
Aug 12, 2026
…le-field FMG unlock, and multiplier_schur_pc verified (#478 #483 #484 #486) (#534) * Make every preconditioner fallback readable: solver.pc_fallbacks records what degraded, where, and why (#484) Ten of the twelve places a solver silently degrades its preconditioner (the single-field FMG gate, missing hierarchy, transfer-build failures, guard skips, forced Galerkin, the rotated-path substitutions) left no queryable state — a warning at best, nothing at worst — so neither a user nor a test could ask "did I get what I asked for?". Every site now writes through one recorder (_record_pc_fallback) into a public read-only property, solver.pc_fallbacks, with a fixed reason vocabulary: unavailable / declined / build_failed / check_skipped / forced. The record is cleared whenever the preconditioner options re-resolve (the same staleness rule as _pc_resolved), and solve-time sites (custom_mg, rotated_bc) re-record each solve. Warnings are unchanged where they exist; the silent "auto" declines gain a record but deliberately NO new warning. rotated_bc mirrors its two degraded arms into the same record; its own ctx keys stay authoritative. Behaviourally inert: no option value, route, or default changes. tests/test_1022_pc_fallback_observability.py: every probe proven to fire and proven silent on a clean solve (empty record is the global negative control). Underworld development team with AI support from Claude Code * Honour user-set snes_rtol/ksp_atol on the saddle-point solvers, and derive tolerance margins through one documented mechanism (#483) The Stokes solve re-pushed snes_rtol and ksp_atol before every solve (_reassert_outer_tolerances), silently discarding a value the user set explicitly — the worst reachability middle ground: documented as settable, actually owned. The re-push now runs through the same recorded-ownership latch that made snes_max_it reachable (ruling D18), generalised to a dict (_resolve_owned_option/_push_owned_option, with _resolve_snes_max_it kept as a named delegate): the framework keeps asserting the tolerance-derived values until the user sets a key, after which their value is honoured across solves. A user who never touches the keys sees byte-identical behaviour (proved by the no-override arms). Second half: Stokes and Stokes_Constrained derived different option keys from `tolerance` through two unrelated hand-rolled setters. Both now apply a per-class table (_TOLERANCE_DERIVED_KEYS, the base table keeping its historical _INNER_RTOL_MARGIN name as an alias) through one base method (_derive_tolerance_margins), at SET time only. The difference in table contents is deliberate and now documented: the base class derives the inner fieldsplit margins; Constrained derives the outer ksp_rtol and the Eisenstat-Walker pins, because EW pinning owns its outer accuracy. Constrained.__init__'s duplicate EW writes route through the same helper. Both `tolerance` docstrings now state the ownership table (OWNED vs DERIVED-at-set-time keys, and the EW caveat on ksp_rtol). tests/test_1023_saddle_tolerance_ownership.py reads the LIVE PETSc objects after two solves (the second solve is the one that used to clobber), with no-override negative controls on both classes. Stakeholder tests that write these keys (0820, 1013, 1014x2) pass. Underworld development team with AI support from Claude Code * Honour explicit geometric multigrid on single-field solvers via injection-free custom-P transfers (#478) preconditioner="fmg" on a scalar/vector solver was declined to GAMG with a warning, because the NATIVE geometric-FMG path needs DMCreateInjection, which PETSc cannot reliably build for a single-field discretisation on a refined DMPlex (err62, #276). That gate locked geometric MG out of every scalar/vector solver even though the robust custom-P route (own prolongations + Galerkin RAP, no injection anywhere) was already in utilities.custom_mg. The explicit request is now honoured: the gate flags the reroute (_pc_single_field_geo_requested) and custom_mg.build_transfers grows a THIRD hierarchy source — requested-native, wrapping the mesh's own dm_hierarchy tail in _DMLevelView adapters — resolved after solver-set and mesh-owned so build_transfers stays the single "which hierarchy" owner (#471, composing with #530's explicit-gamg opt-out, which is untouched). Installation rides the existing auto_inject_custom_mg solve hooks and the shared multigrid_options bundle (#468/#515), so the options DB deliberately keeps GAMG as the safe degrade base until the live PC is configured at first solve. Build failure degrades to that base through the recorded barycentric -> RBF -> default ladder (#484). Contract line, stated in both docstrings: preconditioner="fmg" is a PREFERENCE (degrades readably); set_custom_fmg is a DEMAND (raises). "auto" is deliberately unchanged — flipping the single-field default is its own validation campaign per #478 — it keeps GAMG and records the decline as the migration probe. Found en route: a gmsh-imported BASE hierarchy level carries section-only coordinates (PetscContainer, no PetscFE), and DMCreateInterpolation from such a source silently returns a ZERO matrix; _DMLevelView therefore works on a clone and installs a P1 Lagrange coordinate FE when missing. tests/test_1020 becomes the unlock test (live PC "mg" over every hierarchy level on the exact #276 err62 geometries; degrade arm proven by monkeypatched builders; auto-unchanged arm; Stokes velocity block untouched; DB-vs-live honesty arm) and tests/parallel/ptest_1020_fmg_single_field_parallel.py proves route parity at np=2. Underworld development team with AI support from Claude Code * multiplier_schur_pc: prove where it is live, instrument where it is inert, correct the drifted docstrings (#486) The issue measured four identical convergence rows across two decades of viscosity contrast and asked "verify or remove". Tracing PETSc's fieldsplit.c settles it without ambiguity: the flag swaps only the Pmat (h,h) block, and under Stokes_Constrained's own defaults (selfp + diag_use_amat) selfp assembles the Schur preconditioner from AMAT sub-blocks — the swapped block is provably never read, which is exactly what the issue measured. The flag IS read under pc_fieldsplit_schur_precondition='a11' (Sp = the grouped [p,h] Pmat block) and under a monolithic direct factorisation of the Pmat. Verdict: keep + instrument, not remove. Instrumentation: at the hh_pc selection in _setup_solver, an explicit opt-in that cannot reach the PC (not a11, diag_use_amat set, not a direct solve) records reason='declined' in pc_fallbacks AND warns — an explicit opt-in silently doing nothing is exactly the #477 class. Docstrings: multiplier_schur_pc now states its two live regimes and the inert one; the Constrained saddle_preconditioner claim that selfp uses "the 1/viscosity mass from constitutive_model.K" was drifted (selfp reads the Amat; the 1/mu mass participates only under a11) — corrected, with a TODO(BUG) on whether selfp should see Pmat blocks at all (a design question, deliberately not changed here). tests/test_1024_multiplier_schur_pc.py: matrix-level oracle (assembled Schur pre differs flag-on/off under a11 — immune to "both converge in 4 iterations"), the inertness codified as an exact-equality control pinning the PETSc semantics, regime-aware probe silence, no-flag silence, and the setter re-registration contract. Underworld development team with AI support from Claude Code * Review response: a resolution-cached hierarchy yields to a new explicit choice; the inertness probe reads the flag's value Two defects from the #534 adversarial review, both violations of the bundle's own observability contract. (1) The explicit-fmg install cached its hierarchy in solver._custom_mg, which auto_inject re-installs unconditionally — so a later preconditioner="gamg" was unreachable with an EMPTY pc_fallbacks record. The cache is now marked auto_cached and the preconditioner setter drops it; a set_custom_fmg registration is a demand and is kept. Regression test with a verified negative control (fails with the marker disabled). (2) The multiplier_schur_pc inertness probe read hasName("pc_fieldsplit_diag_use_amat") — the key's presence — so diag_use_amat=false (the flag LIVE, Schur pre measured differing by rel-Frobenius 0.30) still warned "no effect" and recorded a decline. It now reads the bool value; regression test pins the flag=false silence. Underworld development team with AI support from Claude Code
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Parallel pinned-band and custom-MG opt-out fixes from the #488 review
These are the findings from the #488 adversarial re-review, extracted as an
independent branch after #510/#511 superseded that PR. Each is a measured
defect: the np=4 pinned-band hang, 4.2e-3 motion of a pinned vertex, a crash
on an explicit GAMG choice, the #492 rebuild trap behind the test_0842 CI
segfault, and bare "Segmentation fault" CI logs. Rebased cleanly onto
development at 606583e (#515–#526: place_sheet 3-D, parallel placement,
lifecycle, outcropping zones) — no conflicts; none of the five fixes gained a
development-side counterpart (
label_interface_bandthere still has noallreduce).
What changed
Collective pinned-band emptiness (
label_interface_band,discretisation_mesh.py). The band-emptiness check raised rank-locally, so asurface confined to one rank's corner at np=4 left three ranks raising while
the fourth entered the collective mover and hung to the 300 s timeout
(measured). The pinned count is now allreduced; the error raises on every
rank only when the band is globally empty; the DMLabel is created on every
rank, including band-less ones.
Partition-independent pinned band (same function). The straddle test and
halo-ring growth walk rank-local cells, so a shared vertex whose cut cell
lives on the neighbour rank was pinned there but not on the owner — the
mover moved it and the pinned copy followed through the SF (measured at
np=4: two pinned leaves moved 4.2e-3 and 1.9e-3; np=2/3 passed on partition
luck). The pinned set is now synchronised across ranks by rounded coordinate
after the core band and after each halo ring. The parallel test
(
ptest_0845) gains two guards for the band-less rank np=4 creates: theempty-DMLabel getStratumIS segfault (Stokes_Constrained segfaults at np>1 in the interior-multiplier section reduction #291) and the zero-size max()
collective desync.
Explicit GAMG opt-out for the custom-P pickup (
custom_mg.py,rotated_bc.py).solver.preconditioner = "gamg"(or a user-latchedpc_type) on an adapt child was silently clobbered back to the custom-P PCMG
at solve time — both arms of test_0842's fmg-vs-gamg comparison ran
pc_type=mg.
build_transfersnow stands down for an explicit choice,returning
(None, None)(never bareNone), and both call sites guard theunpack so "no hierarchy" degrades to the default preconditioner rather than
a TypeError mid-solve.
test_0842 hardened. Both MeshVariables are created before any solve —
creating one after a solve rebuilds mesh.dm under the custom-MG links
(issue Creating a MeshVariable destroys the previous mesh.dm; a held handle segfaults (use-after-free) #492), which is what segfaulted Linux CI downstream. Each arm asserts
the PC type it actually ran, so the comparison can never silently become
FMG-vs-itself again. Tolerances: ksp_rtol 1e-9 against a 1e-7 nodal bound,
chosen to catch the once-shipped true-residual stall (the geometric bundle
now owns its fgmres outer pairing, Geometric-MG bundle pairs a gmres smoother with a non-flexible outer KSP: converges by recurrence while the true residual stalls #514/Geometric-MG bundle owns its outer-KSP pairing: fgmres, applied to the live KSP #515).
PYTHONFAULTHANDLER=1inscripts/test.sh— a CI crash prints a Pythonstack instead of a bare "Segmentation fault".
Deliberately not included
tripwire, the 0844 self-built vertex blanket, the 0847 deterministic plotter
close, and Placed surfaces (2-D serial) + the reconciled reconnect.py #511's reconciled reconnect/place-surface work (untouched here).
Verification (post-rebase onto 606583e, worktree env rebuilt)
test_0842serial (2 passed) and np=2 (2 passed); explicit-gamg probeconfirms each arm on its own PC (mg its=3, err 7.5e-10 vs gamg its=23,
err 2.0e-9), no crash.
ptest_0845at np=4: 3 passed on all ranks (the partition-independence case).all ranks;
offset=5.0raises ValueError on 4/4 ranks past a final barrier.pytest tests/test_08*py -qone process: 551 passed, 11 skipped, 11 xfailed,0 failed.
pytest tests -m "level_1 and tier_a"(ignoring test_0050):580 passed, 17 skipped, 1 xfailed, 0 failed.
Underworld development team with AI support from Claude Code