diff --git a/docs/reviews/2026-10/subksp-banner-827-adversarial-review.md b/docs/reviews/2026-10/subksp-banner-827-adversarial-review.md new file mode 100644 index 000000000..df94e31f5 --- /dev/null +++ b/docs/reviews/2026-10/subksp-banner-827-adversarial-review.md @@ -0,0 +1,70 @@ +# Quiet sub-solver query (#827): adversarial review record + +Branch `bugfix/subksp-banner-827`, off `development`. One independent reviewer, +run against the installed build of the branch, with an older feature branch +carrying the same unguarded call sites as the unfixed reference. + +## What the branch does + +`_fieldsplit_sub_ksps(pc)` (`utilities/_petsc_tools.py`) returns a field split's +block solvers, or `[]` when the split is not set up, with PETSc's error banner +silenced for that one call (the `"return"` handler: the error is still raised, not +printed; popped on every path). Two call sites use it: + +| Site | Path that printed the banner | +|---|---| +| `SNES_Stokes._check_velocity_preconditioner` | the viscoelastic solve ran the check before solving (the #827 reproducer); a plain warm solve that converged without a linear solve after a `consistent_jacobian` change rebuilt the solver | +| `custom_mg._pcmg_still_installed` | the pre-solve "still installed?" test on a solver rebuilt by a `consistent_jacobian` change; every notch run with a non-nested FMG tail printed it | + +The viscoelastic check now runs after the solve, as on the plain path. + +## Findings and their state + +- **The plain-path site had no test** (medium). A loud helper in `systems.solvers` + passed every test. Fixed: `test_warm_solve_without_a_linear_solve_prints_no_banner`; + 12 banner lines unfixed, 0 fixed. +- **Viscoelastic solves now warn and record a fallback** (behaviour change, kept). + Before, the pre-solve check always returned early, so a viscoelastic solve on a + mesh without a hierarchy never reported running GAMG. It now warns (once per + process under the default filter) and records `velocity.fell_back_from_fmg`, as a + plain solve does. Separately and pre-existing: `stacklevel=3` resolves to + `memprobe.py`, so every solve shares one warning location. +- **The helper reached the `uw` namespace** through `from .utilities._petsc_tools + import *`. Fixed: renamed `_fieldsplit_sub_ksps`. +- **Second copy of the pattern** in `petsc_generic_snes_solvers.pyx` + (`_discard_preconditioner_setup`). Left: the solver core is not changed in a + bugfix; a follow-up can route it through the helper. +- **Docstring** told the debugging history and lacked Parameters/Returns. Fixed. + +## Attacks that failed + +- Handler semantics: on an un-set-up Schur split the helper returns `[]` silently; + a direct call afterwards still raises and prints, so the stack is restored; a + `KeyboardInterrupt` inside propagates with the stack restored; 1000 calls leave it + intact; after set-up it returns 2 blocks. +- Parallel (np 2 and 4): helper before/after set-up, the viscoelastic reproducer, + 3 viscoelastic steps, the custom-FMG rewire. Exit 0, no banner, no hang. Set-up + state follows `PCSetUp`, which every rank runs; the not-set-up error is a local + check, so no rank takes a different branch. +- `_pcmg_still_installed` returns `False` for an un-set-up split as before (it used + to raise and be caught); no extra re-installs. +- Other `getFieldSplitSubKSP` calls (`custom_mg.py` transfer install, + `rotated_bc.py`, `solver_health.py`, the solve-config dump) all follow a + `setUp`, run inside a solve, or are already guarded. +- No import cycle: `_petsc_tools` imports only petsc4py. +- Tests: all three regression tests fail on the unfixed code on the banner + assertion and pass fixed; `test_107*` is in `scripts/test.sh`; + `check_test_coverage.py` reports every file reachable. + +## Suites + +Related suites on the fixed build (multigrid, custom-MG hierarchy, rotated +free-slip, MG option bundle, restore/continuation, viscoelastic): 77 passed, no +banner. The reviewer's wider run (388 tests, `not tier_c`): 387 passed, 1 failed: +`test_0017_describe_and_render.py::test_every_family_describes_itself_at_the_class_level`, +order-dependent and unrelated. `test_1057_ddt_transport_plugin.py` defines a +solver subclass `TensorTransport` with no summary, and the class-level description +test enumerates every subclass, so it fails whenever test_1057 ran first in the +same process. + +Underworld development team with AI support from Claude Code diff --git a/src/underworld3/systems/solvers.py b/src/underworld3/systems/solvers.py index 605b11814..11b6e9334 100644 --- a/src/underworld3/systems/solvers.py +++ b/src/underworld3/systems/solvers.py @@ -63,6 +63,7 @@ from underworld3 import VarType import underworld3.timing as timing from underworld3.utilities import memprobe +from underworld3.utilities._petsc_tools import _fieldsplit_sub_ksps from underworld3.utilities._api_tools import ( uw_object, SymbolicProperty, @@ -2047,7 +2048,6 @@ def solve( self._setup_pointwise_functions(verbose) self._setup_discretisation(verbose) self._setup_solver(verbose) - self._check_velocity_preconditioner() self._stress_history_advance(timestep, verbose=verbose, evalf=evalf) @@ -2069,6 +2069,9 @@ def solve( print(f"Stokes solver - store stress and shift history", flush=True) self._stress_history_post_solve(timestep, verbose=verbose, evalf=evalf) + # After the solve, as on the plain path: before it the split is not set + # up and has no velocity block to inspect (#827). + self._check_velocity_preconditioner() else: # Plain Stokes — no stress history @@ -2715,20 +2718,16 @@ def _check_velocity_preconditioner(self): if getattr(self, "_pc_user_override", False): return # the user owns this block's pc_type # The rotated free-slip path builds its OWN fieldsplit KSP and leaves - # this one un-set-up, so asking it for sub-KSPs raises -- and PETSc - # prints a full error banner before the exception can be caught, which - # makes a healthy solve look broken. Skip it by the path rather than by - # catching. (Suppressing with pushErrorHandler was tried: it is global - # state and broke 23 unrelated tests.) + # this one un-set-up: there is nothing here to inspect. if getattr(self, "_rotated_freeslip_bcs", None): return - pc = self.snes.getKSP().getPC() - if pc.getType() != "fieldsplit": - return - try: - installed = pc.getFieldSplitSubKSP()[0].getPC().getType() - except Exception: - return # not set up: nothing to inspect + # A split that is not set up has no blocks -- after a rebuild, or a warm + # solve that converged before any linear solve -- and asking for them + # prints a PETSc banner even when caught (#827); the helper asks quietly. + blocks = _fieldsplit_sub_ksps(self.snes.getKSP().getPC()) + if not blocks: + return # not a split, or not set up + installed = blocks[0].getPC().getType() if installed == "mg": return diff --git a/src/underworld3/utilities/_petsc_tools.py b/src/underworld3/utilities/_petsc_tools.py index 344b8aff2..3f15b6848 100644 --- a/src/underworld3/utilities/_petsc_tools.py +++ b/src/underworld3/utilities/_petsc_tools.py @@ -56,3 +56,34 @@ def requote(argument): import os as _os + + +def _fieldsplit_sub_ksps(pc): + """The block solvers of a field-split preconditioner, asked for quietly. + + A split has its blocks only once ``PCSetUp`` has run. Asking earlier is a + PETSc error, and PETSc prints its banner before the exception reaches Python, + so catching it alone cannot keep a healthy solve's output clean (#827). The + ``"return"`` error handler is pushed for this one call: the error is still + raised, it is just not printed, and the handler is popped on every path. + + Parameters + ---------- + pc : petsc4py.PETSc.PC + Any preconditioner. + + Returns + ------- + list of petsc4py.PETSc.KSP + The split's block solvers, or ``[]`` when ``pc`` is not a field split or + PETSc declines to give its blocks (the split is not set up). + """ + if pc.getType() != "fieldsplit": + return [] + PETSc.Sys.pushErrorHandler("return") + try: + return list(pc.getFieldSplitSubKSP()) + except PETSc.Error: + return [] # no blocks to give: not set up + finally: + PETSc.Sys.popErrorHandler() diff --git a/src/underworld3/utilities/custom_mg.py b/src/underworld3/utilities/custom_mg.py index 44c50be6f..e0b1fb659 100644 --- a/src/underworld3/utilities/custom_mg.py +++ b/src/underworld3/utilities/custom_mg.py @@ -46,6 +46,7 @@ from petsc4py import PETSc from underworld3.utilities import multigrid_options +from underworld3.utilities._petsc_tools import _fieldsplit_sub_ksps __all__ = ["barycentric_prolongation", "rbf_prolongation", "inject_custom_mg", "CustomMGHierarchy", "set_custom_fmg", "sbr_refine", "sbr_refine_where", @@ -2219,10 +2220,12 @@ def _pcmg_still_installed(solver, h): if pfx == "": pc = solver.snes.getKSP().getPC() elif pfx == "fieldsplit_velocity_": - outer = solver.snes.getKSP().getPC() - if outer.getType() != "fieldsplit": + # quietly: an un-set-up split has no blocks, and asking prints a + # PETSc banner even when caught (#827) + blocks = _fieldsplit_sub_ksps(solver.snes.getKSP().getPC()) + if not blocks: return False - pc = outer.getFieldSplitSubKSP()[0].getPC() + pc = blocks[0].getPC() else: return False return (pc.getType() == "mg" diff --git a/tests/test_1077_quiet_unsetup_fieldsplit.py b/tests/test_1077_quiet_unsetup_fieldsplit.py new file mode 100644 index 000000000..9d35f1cad --- /dev/null +++ b/tests/test_1077_quiet_unsetup_fieldsplit.py @@ -0,0 +1,106 @@ +"""No PETSc error banner from asking an un-set-up field split for its blocks (#827). + +A field split has block solvers only once ``PCSetUp`` has run. Asking for them +earlier raises, and PETSc prints a ~38-line "Object is in wrong state" banner to +stderr before the exception reaches Python, so ``except`` cannot hide it and a +healthy solve looks broken. Three paths reached it: + +- a viscoelastic solve checked the velocity preconditioner before solving; +- the same check after a plain solve that converged without a linear solve, on + a solver rebuilt by a ``consistent_jacobian`` change, so the new split was + never set up; +- the custom-multigrid "still installed?" test, run before each solve, on a + solver rebuilt by a ``consistent_jacobian`` change (the notch runs' path). + +The banner is C-level output on file descriptor 2, so ``capfd`` sees it. +""" +import pytest +import sympy + +import underworld3 as uw +from underworld3.utilities._petsc_tools import _fieldsplit_sub_ksps + +pytestmark = [pytest.mark.level_1, pytest.mark.tier_b] + +BANNER = "PETSC ERROR" + + +def _stokes(cellSize=0.25, refinement=None): + mesh = uw.meshing.UnstructuredSimplexBox( + minCoords=(0.0, 0.0), maxCoords=(1.0, 1.0), cellSize=cellSize, + refinement=refinement) + v = uw.discretisation.MeshVariable("V", mesh, mesh.dim, degree=2) + p = uw.discretisation.MeshVariable("P", mesh, 1, degree=1) + stokes = uw.systems.Stokes(mesh, velocityField=v, pressureField=p) + stokes.add_dirichlet_bc((0.0, 0.0), "Bottom") + stokes.add_dirichlet_bc((1.0, 0.0), "Top") + return stokes + + +def test_helper_is_quiet_on_an_unsetup_split(capfd): + """The helper: [] before set-up, with nothing printed; the blocks after.""" + stokes = _stokes() + stokes.constitutive_model = uw.constitutive_models.ViscousFlowModel + stokes.constitutive_model.Parameters.shear_viscosity_0 = 1.0 + stokes._setup_pointwise_functions() + stokes._setup_discretisation() + stokes._setup_solver() + pc = stokes.snes.getKSP().getPC() + assert pc.getType() == "fieldsplit" + assert _fieldsplit_sub_ksps(pc) == [] + assert BANNER not in capfd.readouterr().err + stokes.solve() + assert len(_fieldsplit_sub_ksps(stokes.snes.getKSP().getPC())) == 2 + + +def test_viscoelastic_solve_prints_no_banner(capfd): + """The #827 reproducer.""" + stokes = _stokes() + stokes.constitutive_model = uw.constitutive_models.ViscoElasticPlasticFlowModel( + stokes.Unknowns, order=1) + stokes.constitutive_model.Parameters.shear_viscosity_0 = 1.0 + stokes.constitutive_model.Parameters.shear_modulus = 1.0 + stokes.constitutive_model.Parameters.dt_elastic = sympy.Rational(1, 10) + stokes.solve(timestep=0.1) + assert BANNER not in capfd.readouterr().err + + +def test_warm_solve_without_a_linear_solve_prints_no_banner(capfd): + """A tangent change rebuilds the solver; the warm solve that follows meets its + (loose) tolerance at once, so no linear solve sets the new split up, and the + post-solve velocity-preconditioner check asked it for its blocks.""" + stokes = _stokes() + stokes.constitutive_model = uw.constitutive_models.ViscoPlasticFlowModel + stokes.constitutive_model.Parameters.shear_viscosity_0 = 1.0 + stokes.constitutive_model.Parameters.yield_stress = 0.3 + stokes.consistent_jacobian = False + stokes.solve() + capfd.readouterr() + stokes.petsc_options["snes_atol"] = 1.0e6 + stokes.consistent_jacobian = True + stokes.solve(zero_init_guess=False) + assert stokes.snes.getIterationNumber() == 0 + assert BANNER not in capfd.readouterr().err + + +def test_custom_fmg_rewire_prints_no_banner(capfd): + """A custom multigrid on the velocity block (an independent, non-nested coarsest + mesh: the shape of the notch runs' gmsh-tail FMG), then a tangent change. The + change rebuilds the solver, and the next solve's "is the multigrid still + installed?" test asked the new, un-set-up split for its blocks.""" + stokes = _stokes(refinement=1) + stokes.constitutive_model = uw.constitutive_models.ViscoPlasticFlowModel + stokes.constitutive_model.Parameters.shear_viscosity_0 = 1.0 + stokes.constitutive_model.Parameters.yield_stress = 0.3 + fine = stokes.mesh + tail = uw.meshing.UnstructuredSimplexBox( + minCoords=(0.0, 0.0), maxCoords=(1.0, 1.0), cellSize=0.6) + stokes.set_custom_fmg([tail] + fine._coarse_level_meshes()[:-1], field_id=0) + stokes.consistent_jacobian = False + stokes.solve() + capfd.readouterr() + stokes.consistent_jacobian = True + stokes.solve(zero_init_guess=False) + assert stokes.snes.getConvergedReason() > 0 + assert BANNER not in capfd.readouterr().err + assert stokes.snes.getKSP().getPC().getFieldSplitSubKSP()[0].getPC().getType() == "mg"