From b030c3a57f13bdde93f7e4ea92edf89e9ae9c8db Mon Sep 17 00:00:00 2001 From: lmoresi Date: Sun, 9 Aug 2026 12:27:35 +1000 Subject: [PATCH 1/5] Make the pinned-band emptiness test collective, and create the label on every rank MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit 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 --- .../discretisation/discretisation_mesh.py | 17 ++++++++++++++--- 1 file changed, 14 insertions(+), 3 deletions(-) diff --git a/src/underworld3/discretisation/discretisation_mesh.py b/src/underworld3/discretisation/discretisation_mesh.py index 50262af9..a1ddb5b6 100644 --- a/src/underworld3/discretisation/discretisation_mesh.py +++ b/src/underworld3/discretisation/discretisation_mesh.py @@ -6829,9 +6829,15 @@ def label_interface_band(self, surface, offset=0.0, halo=1, name=None): grown.update(vv) pinned |= grown - if not pinned: - # An empty DMLabel is not merely useless: querying its strata is a - # hard crash, not an exception, so refuse rather than hand one back. + # COLLECTIVE emptiness test. A rank whose subdomain the surface never + # enters legitimately has an empty local band — only a GLOBALLY empty + # band is a user error. The previous rank-local raise here deadlocked + # np=4 (measured 2026-08-06, review of PR #488: a corner-confined + # surface left three ranks raising while the fourth entered the + # collective mover and hung to the 300 s timeout). + n_global = uw.mpi.comm.allreduce(len(pinned)) + if n_global == 0: + # Raised on EVERY rank, after the collective reduction. raise ValueError( f"no cell is cut by distance == {offset} on surface " f"{getattr(surface, 'name', surface)!r}, so there is no band to " @@ -6839,6 +6845,11 @@ def label_interface_band(self, surface, offset=0.0, halo=1, name=None): f"interface you meant (for a weak zone it is the HALF-WIDTH, not " f"zero).") + # Every rank creates the label, including ranks whose local band is + # empty: the downstream consumer (smoothing.graph._pinned_mask) is + # documented to tolerate a present-but-empty label, and a label that + # exists on some ranks only is the kind of asymmetry this method is + # not allowed to produce. name = name or f"PinnedBand_{getattr(surface, 'name', 'surface')}" if not dm.hasLabel(name): dm.createLabel(name) From befa326d4dc39f9ea7f4247400edc977ba0f82d9 Mon Sep 17 00:00:00 2001 From: lmoresi Date: Sun, 9 Aug 2026 12:27:55 +1000 Subject: [PATCH 2/5] Make the pinned band partition-independent: synchronise pinned vertices 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 --- .../discretisation/discretisation_mesh.py | 28 +++++++++++++++++++ .../ptest_0845_relax_pinned_band_parallel.py | 15 ++++++++-- 2 files changed, 41 insertions(+), 2 deletions(-) diff --git a/src/underworld3/discretisation/discretisation_mesh.py b/src/underworld3/discretisation/discretisation_mesh.py index a1ddb5b6..750766b4 100644 --- a/src/underworld3/discretisation/discretisation_mesh.py +++ b/src/underworld3/discretisation/discretisation_mesh.py @@ -6816,11 +6816,38 @@ def label_interface_band(self, surface, offset=0.0, halo=1, name=None): if vS <= p < vE]) for c in range(cS, cE)] + def _sync_across_ranks(pinned_set): + """A vertex pinned on ANY rank is pinned on EVERY rank holding a copy. + + The straddle test and the 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 is pinned there but not here. + If HERE is the owner, the mover moves it and the neighbour's pinned + copy follows 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. Matching by coordinate (the same rounded + key the parallel test uses) makes the set partition-independent. + """ + if uw.mpi.size == 1: + return pinned_set + local_xy = (coords[[v - vS for v in pinned_set]] + if pinned_set else numpy.empty((0, self.dim))) + global_keys = set() + for arr in uw.mpi.comm.allgather(local_xy): + for p in arr: + global_keys.add(tuple(numpy.round(p, 12))) + out = set(pinned_set) + for i in range(vE - vS): + if tuple(numpy.round(coords[i], 12)) in global_keys: + out.add(vS + i) + return out + pinned = set() for verts in cell_vertices: d = distance[verts - vS] if d.min() < offset < d.max(): pinned.update(int(v) for v in verts) + pinned = _sync_across_ranks(pinned) for _ring in range(halo): grown = set() for verts in cell_vertices: @@ -6828,6 +6855,7 @@ def label_interface_band(self, surface, offset=0.0, halo=1, name=None): if any(v in pinned for v in vv): grown.update(vv) pinned |= grown + pinned = _sync_across_ranks(pinned) # COLLECTIVE emptiness test. A rank whose subdomain the surface never # enters legitimately has an empty local band — only a GLOBALLY empty diff --git a/tests/parallel/ptest_0845_relax_pinned_band_parallel.py b/tests/parallel/ptest_0845_relax_pinned_band_parallel.py index 7461c4db..ed37c053 100644 --- a/tests/parallel/ptest_0845_relax_pinned_band_parallel.py +++ b/tests/parallel/ptest_0845_relax_pinned_band_parallel.py @@ -47,7 +47,15 @@ def _coords(mesh): def _pinned_indices(mesh, name): vS, _vE = mesh.dm.getDepthStratum(0) - iset = mesh.dm.getLabel(name).getStratumIS(1) + label = mesh.dm.getLabel(name) + # A rank the surface never enters has the label but NO strata, and + # getStratumIS on an empty DMLabel is a segfault, not an exception + # (#291) — the same tolerant pattern smoothing.graph._pinned_mask uses. + # At np=4 this fixture leaves one rank band-less, which is exactly the + # partition case the 2026-08-02 review found uncovered. + if label.getNumValues() == 0: + return np.zeros(0, dtype=np.int64) + iset = label.getStratumIS(1) if iset is None: return np.zeros(0, dtype=np.int64) return np.asarray(iset.getIndices(), dtype=np.int64) - vS @@ -93,7 +101,10 @@ def test_pinned_vertices_including_shared_ones_do_not_move(): after = _coords(mesh) moved = np.linalg.norm(after - before, axis=1) - assert uw.mpi.comm.allreduce(float(moved[idx].max()), op=MPI.MAX) == 0.0 + # A band-less rank (np=4 leaves one) has empty idx; max() of a zero-size + # array raises rank-locally and desyncs the collectives below. + assert uw.mpi.comm.allreduce( + float(moved[idx].max()) if len(idx) else 0.0, op=MPI.MAX) == 0.0 free = np.setdiff1d(np.arange(len(before)), idx) assert uw.mpi.comm.allreduce(float(moved[free].max()) if len(free) else 0.0, op=MPI.MAX) > 0.0, "the mover did nothing" From f839f03ba9b704ffc0e08aa99420e41759843293 Mon Sep 17 00:00:00 2001 From: lmoresi Date: Sun, 9 Aug 2026 12:28:04 +1000 Subject: [PATCH 3/5] An explicit GAMG choice opts out of the custom-P multigrid pickup, without crashing MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit 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 --- src/underworld3/utilities/custom_mg.py | 27 +++++++++++++++++++++++-- src/underworld3/utilities/rotated_bc.py | 7 +++++-- 2 files changed, 30 insertions(+), 4 deletions(-) diff --git a/src/underworld3/utilities/custom_mg.py b/src/underworld3/utilities/custom_mg.py index 9719e6e0..f9d02b81 100644 --- a/src/underworld3/utilities/custom_mg.py +++ b/src/underworld3/utilities/custom_mg.py @@ -1040,6 +1040,24 @@ def build_transfers(solver, field_id=None): if coarse is None: return None, None # nothing to inject + # An EXPLICIT preconditioner choice beats the opportunistic pickup. Before + # this guard, `solver.preconditioner = "gamg"` on an adapt child was + # silently clobbered back to the custom-P PCMG at solve time (measured: + # both arms of test_0842's fmg-vs-gamg comparison ran pc_type=mg), so a + # user could not opt out and any FMG-vs-GAMG comparison was vacuous. + # `_pc_user_override` is the same statement in the other spelling: the + # solver's option manager has latched "the user owns this block's pc_type" + # (they wrote a pc_type of their own into petsc_options), and an + # opportunistic pickup must stand down for exactly the same reason. + # "auto" (the default) still picks up the mesh-owned hierarchy. + # NOTE the arity: this function returns a 2-tuple, never bare None — a bare + # `return` here is what turned the gate into a TypeError at the call site + # when this hunk migrated from auto_inject_custom_mg (which returns nothing) + # during the #488 x #471 merge. + if (getattr(solver, "_preconditioner", "auto") == "gamg" + or getattr(solver, "_pc_user_override", False)): + return None, None + builder = getattr(solver.mesh, "_custom_mg_builder", "barycentric") # Retry with the RBF builder before abandoning geometric MG. The # barycentric builder has LOCAL support: it re-triangulates the coarse @@ -1098,8 +1116,13 @@ def auto_inject_custom_mg(solver, field_id=None): inject_custom_mg(solver) return - h, Ps = build_transfers(solver, field_id=field_id) - if h is None: + # build_transfers' contract is a 2-tuple, but a "no hierarchy" answer has + # been written as a bare `return` before (the #488 x #471 merge shipped + # exactly that inside the explicit-gamg gate): a None here must mean + # "nothing to inject", never a TypeError mid-solve. + 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 # Dimensional guard (checkable for the monolithic operator, field_id is None): diff --git a/src/underworld3/utilities/rotated_bc.py b/src/underworld3/utilities/rotated_bc.py index 3d521845..7d00a499 100644 --- a/src/underworld3/utilities/rotated_bc.py +++ b/src/underworld3/utilities/rotated_bc.py @@ -907,8 +907,11 @@ def _build_rotated_custom_Pl(solver, Q, normal_rows): free-slip silently lost its multigrid and solved on GAMG (#467). This is the ``adapt-on-top-faults`` workflow's own configuration.""" from underworld3.utilities import custom_mg - h, Ps = custom_mg.build_transfers(solver, field_id=0) - if h is None: + # 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 vel_is = solver._subdict["velocity"][0] vis = np.asarray(vel_is.getIndices()) From 58702006557114ca4082542fef0caf62cb5e3209 Mon Sep 17 00:00:00 2001 From: lmoresi Date: Sun, 9 Aug 2026 12:28:14 +1000 Subject: [PATCH 4/5] Disarm the #492 trap in test_0842 and make the fmg-vs-gamg comparison 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 --- tests/test_0842_nvb_3d_parallel_adapt.py | 39 +++++++++++++++++++++--- 1 file changed, 34 insertions(+), 5 deletions(-) diff --git a/tests/test_0842_nvb_3d_parallel_adapt.py b/tests/test_0842_nvb_3d_parallel_adapt.py index fe95708d..4fd05a2d 100644 --- a/tests/test_0842_nvb_3d_parallel_adapt.py +++ b/tests/test_0842_nvb_3d_parallel_adapt.py @@ -90,8 +90,15 @@ def test_poisson_fmg_on_3d_child_matches_gamg(): mesh = _base3() child = mesh.adapt(_ball_metric, max_levels=1) + # Deliberate ordering: create BOTH variables before any solver runs — + # creating a MeshVariable after a solve rebuilds mesh.dm and detonates + # issue #492 (the old DM is destroyed under the custom-MG coarse/fine + # links; that dangling reference is what segfaulted Linux CI downstream). + fields = {pc: uw.discretisation.MeshVariable(f"u_{pc}", child, 1, degree=1) + for pc in ("fmg", "gamg")} + def solve(pc): - u = uw.discretisation.MeshVariable(f"u_{pc}", child, 1, degree=1) + u = fields[pc] poisson = uw.systems.Poisson(child, u_Field=u) poisson.constitutive_model = uw.constitutive_models.DiffusionModel poisson.constitutive_model.Parameters.diffusivity = 1.0 @@ -101,15 +108,37 @@ def solve(pc): if pc == "gamg": poisson.preconditioner = "gamg" poisson.petsc_options["pc_type"] = "gamg" - poisson.petsc_options["ksp_rtol"] = 1e-8 + poisson.petsc_options["ksp_rtol"] = 1e-9 poisson.solve() - its = poisson.snes.getKSP().getIterationNumber() + ksp = poisson.snes.getKSP() + its = ksp.getIterationNumber() + # The comparison must be REAL. The explicit-gamg arm was once silently + # clobbered by the mesh-owned custom-P pickup, so both arms ran + # pc_type=mg and the fmg-vs-gamg comparison compared FMG to itself. + # Pin each arm's PC so that vacuous comparison can never return. + assert ksp.getPC().getType() == ("gamg" if pc == "gamg" else "mg") # exact linear solution T = z: also proves the Dirichlet facet - # labels survived the parallel transform + # labels survived the parallel transform. + # + # The bound is tight ON PURPOSE: it must catch a once-shipped defect + # whose signature was a TRUE-error stall at 1e-6, INSENSITIVE to + # ksp_rtol — the gmres-smoothed geometric bundle (a non-stationary + # preconditioner) under a plain left-preconditioned gmres outer, whose + # recurrence norm fell to 1e-11 while the true residual stalled + # (the geometric bundle now owns the pairing — multigrid_options puts + # ksp_type=fgmres in the bundle and _configure_pcmg applies it to the + # live KSP, #514/#515 — so the fmg arm's ksp_rtol is enforced in the + # true residual norm; + # measured err/nrm ~1e-12). The gamg arm still converges in the + # preconditioned norm, with a declared-reduction -> nodal-error + # constant of ~10 on this child, so the declared reduction is one + # order tighter than the bound: neither arm rides on its PC constant, + # and the O(1) failures this test exists for (a lost Dirichlet label, + # a wrong transfer) stay unmissable. err = np.linalg.norm( poisson.Unknowns.u.data[:, 0] - poisson.Unknowns.u.coords[:, 2]) nrm = np.linalg.norm(poisson.Unknowns.u.coords[:, 2]) + 1e-30 - assert err / nrm < 1e-8 + assert err / nrm < 1e-7 return its fmg_its = solve("fmg") From d251a5c891375f7c48f70c2bba76766d76db1da7 Mon Sep 17 00:00:00 2001 From: lmoresi Date: Sun, 9 Aug 2026 12:28:14 +1000 Subject: [PATCH 5/5] CI crashes print a Python stack: PYTHONFAULTHANDLER=1 in scripts/test.sh Underworld development team with AI support from Claude Code --- scripts/test.sh | 2 ++ 1 file changed, 2 insertions(+) diff --git a/scripts/test.sh b/scripts/test.sh index 377902ba..65d14158 100755 --- a/scripts/test.sh +++ b/scripts/test.sh @@ -46,6 +46,8 @@ if [ $PARALLEL_ONLY -eq 1 ] && [ $PARALLEL_RANKS -eq 0 ]; then fi export UW_NO_USAGE_METRICS=0 +# A hard crash must print a Python stack, not just "Segmentation fault". +export PYTHONFAULTHANDLER=1 PYTEST="pytest --config-file=tests/pytest.ini" # Run serial tests (unless --parallel-only specified)