From 001ba8f58fb61248958abfa2a68b5767523503ac Mon Sep 17 00:00:00 2001 From: Louis Moresi Date: Thu, 15 Oct 2026 15:43:14 +1100 Subject: [PATCH 1/2] Ask an un-set-up field split for its blocks quietly (#827) A field split has block solvers only after PCSetUp, and asking earlier is a PETSc error whose ~38-line banner is printed before the exception reaches Python, so `except` cannot hide it. Two paths reached it: - the viscoelastic Stokes solve checked the velocity preconditioner before solving (the #827 reproducer); - the custom-multigrid "still installed?" test, run before each solve, on a solver rebuilt by a consistent_jacobian change. Every notch run with a non-nested FMG tail printed it. fieldsplit_sub_ksps(pc) returns the blocks, or [] when there are none yet, with the banner silenced for that one call by a push/pop of the "return" handler (it still raises; it does not print). Both sites use it, and the viscoelastic check moves to after the solve, as on the plain path, so it now inspects a set-up split instead of always returning early. test_1077 fails on the unfixed code (two cases) and passes with the fix. Underworld development team with AI support from Claude Code --- src/underworld3/systems/solvers.py | 25 +++--- src/underworld3/utilities/_petsc_tools.py | 24 ++++++ src/underworld3/utilities/custom_mg.py | 9 ++- tests/test_1077_quiet_unsetup_fieldsplit.py | 85 +++++++++++++++++++++ 4 files changed, 127 insertions(+), 16 deletions(-) create mode 100644 tests/test_1077_quiet_unsetup_fieldsplit.py diff --git a/src/underworld3/systems/solvers.py b/src/underworld3/systems/solvers.py index 605b11814..f986462da 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..540d8bee9 100644 --- a/src/underworld3/utilities/_petsc_tools.py +++ b/src/underworld3/utilities/_petsc_tools.py @@ -56,3 +56,27 @@ def requote(argument): import os as _os + + +def fieldsplit_sub_ksps(pc): + """The block solvers of a field-split ``pc``, or ``[]`` when it has none yet. + + The blocks exist only once ``PCSetUp`` has run, and asking an un-set-up split + for them is a PETSc error that PETSc prints as a full banner even when the + exception is caught -- so a healthy solve looks broken (#827). The banner is + silenced for this one call only: the ``"return"`` handler still raises, it just + does not print, and it is popped on every path. (The ``"ignore"`` handler is + different -- it turns errors into success -- and pushed broadly it broke 23 + unrelated tests.) + + Anything that is not a field split also has no blocks, so it gives ``[]``. + """ + if pc.getType() != "fieldsplit": + return [] + PETSc.Sys.pushErrorHandler("return") + try: + return list(pc.getFieldSplitSubKSP()) + except PETSc.Error: + return [] # not set up yet: no blocks to report + finally: + PETSc.Sys.popErrorHandler() diff --git a/src/underworld3/utilities/custom_mg.py b/src/underworld3/utilities/custom_mg.py index 44c50be6f..0ce2708a8 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..a8246c5c8 --- /dev/null +++ b/tests/test_1077_quiet_unsetup_fieldsplit.py @@ -0,0 +1,85 @@ +"""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 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_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" From a820ff0a64203e9294303c65143e123dfebef9fb Mon Sep 17 00:00:00 2001 From: Louis Moresi Date: Fri, 16 Oct 2026 01:43:40 +1100 Subject: [PATCH 2/2] Review fixes for the quiet sub-solver query (#827) - Test the plain-path site: a consistent_jacobian change rebuilds the solver, the warm solve meets its tolerance with no linear solve, and the post-solve check asked the un-set-up split for its blocks. Fails unfixed (banner). - The helper is private (_fieldsplit_sub_ksps): _petsc_tools is star-imported into the uw namespace. - Docstring: how it works, with Parameters and Returns. - Review record. Behaviour change, by design: a viscoelastic solve on a mesh without a hierarchy now warns that the velocity block fell back to GAMG and records the fallback, as a plain solve already did. Before, its pre-solve check always returned early. Underworld development team with AI support from Claude Code --- .../subksp-banner-827-adversarial-review.md | 70 +++++++++++++++++++ src/underworld3/systems/solvers.py | 4 +- src/underworld3/utilities/_petsc_tools.py | 33 +++++---- src/underworld3/utilities/custom_mg.py | 4 +- tests/test_1077_quiet_unsetup_fieldsplit.py | 27 ++++++- 5 files changed, 118 insertions(+), 20 deletions(-) create mode 100644 docs/reviews/2026-10/subksp-banner-827-adversarial-review.md 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 f986462da..11b6e9334 100644 --- a/src/underworld3/systems/solvers.py +++ b/src/underworld3/systems/solvers.py @@ -63,7 +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._petsc_tools import _fieldsplit_sub_ksps from underworld3.utilities._api_tools import ( uw_object, SymbolicProperty, @@ -2724,7 +2724,7 @@ def _check_velocity_preconditioner(self): # 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()) + blocks = _fieldsplit_sub_ksps(self.snes.getKSP().getPC()) if not blocks: return # not a split, or not set up installed = blocks[0].getPC().getType() diff --git a/src/underworld3/utilities/_petsc_tools.py b/src/underworld3/utilities/_petsc_tools.py index 540d8bee9..3f15b6848 100644 --- a/src/underworld3/utilities/_petsc_tools.py +++ b/src/underworld3/utilities/_petsc_tools.py @@ -58,18 +58,25 @@ def requote(argument): import os as _os -def fieldsplit_sub_ksps(pc): - """The block solvers of a field-split ``pc``, or ``[]`` when it has none yet. - - The blocks exist only once ``PCSetUp`` has run, and asking an un-set-up split - for them is a PETSc error that PETSc prints as a full banner even when the - exception is caught -- so a healthy solve looks broken (#827). The banner is - silenced for this one call only: the ``"return"`` handler still raises, it just - does not print, and it is popped on every path. (The ``"ignore"`` handler is - different -- it turns errors into success -- and pushed broadly it broke 23 - unrelated tests.) - - Anything that is not a field split also has no blocks, so it gives ``[]``. +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 [] @@ -77,6 +84,6 @@ def fieldsplit_sub_ksps(pc): try: return list(pc.getFieldSplitSubKSP()) except PETSc.Error: - return [] # not set up yet: no blocks to report + 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 0ce2708a8..e0b1fb659 100644 --- a/src/underworld3/utilities/custom_mg.py +++ b/src/underworld3/utilities/custom_mg.py @@ -46,7 +46,7 @@ from petsc4py import PETSc from underworld3.utilities import multigrid_options -from underworld3.utilities._petsc_tools import fieldsplit_sub_ksps +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", @@ -2222,7 +2222,7 @@ def _pcmg_still_installed(solver, h): elif pfx == "fieldsplit_velocity_": # 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()) + blocks = _fieldsplit_sub_ksps(solver.snes.getKSP().getPC()) if not blocks: return False pc = blocks[0].getPC() diff --git a/tests/test_1077_quiet_unsetup_fieldsplit.py b/tests/test_1077_quiet_unsetup_fieldsplit.py index a8246c5c8..9d35f1cad 100644 --- a/tests/test_1077_quiet_unsetup_fieldsplit.py +++ b/tests/test_1077_quiet_unsetup_fieldsplit.py @@ -6,6 +6,9 @@ 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). @@ -15,7 +18,7 @@ import sympy import underworld3 as uw -from underworld3.utilities._petsc_tools import fieldsplit_sub_ksps +from underworld3.utilities._petsc_tools import _fieldsplit_sub_ksps pytestmark = [pytest.mark.level_1, pytest.mark.tier_b] @@ -44,10 +47,10 @@ def test_helper_is_quiet_on_an_unsetup_split(capfd): stokes._setup_solver() pc = stokes.snes.getKSP().getPC() assert pc.getType() == "fieldsplit" - assert fieldsplit_sub_ksps(pc) == [] + assert _fieldsplit_sub_ksps(pc) == [] assert BANNER not in capfd.readouterr().err stokes.solve() - assert len(fieldsplit_sub_ksps(stokes.snes.getKSP().getPC())) == 2 + assert len(_fieldsplit_sub_ksps(stokes.snes.getKSP().getPC())) == 2 def test_viscoelastic_solve_prints_no_banner(capfd): @@ -62,6 +65,24 @@ def test_viscoelastic_solve_prints_no_banner(capfd): 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