Skip to content

Give each autogenerated options prefix a unique index - #53

Merged
connorjward merged 2 commits into
mainfrom
pbrubeck/fix-autogenerated-prefix
Aug 27, 2026
Merged

connorjward merged 2 commits into
mainfrom
pbrubeck/fix-autogenerated-prefix

Conversation

@pbrubeck

Copy link
Copy Markdown
Contributor

AI-assisted: diagnosed and written with Claude Code. I have read and understood every change, and run the tests locally.

Goal

OptionsManager hands out a unique prefix to any object built without an
explicit options_prefix. Since #39 it does not: self.count += 1 reads the
class attribute and rebinds the result onto the instance, so
OptionsManager.count never leaves zero and every autogenerated prefix is
{default_prefix}0_.

>>> [OptionsManager({}, default_prefix="firedrake_").options_prefix for _ in range(3)]
['firedrake_0_', 'firedrake_0_', 'firedrake_0_']

Sharing a prefix means sharing a namespace in the global options database. A
manager built while another manager's options are inserted reads those options
back out as if a user had passed them on the command line, and the warning it
emits names them:

WARNING petsctools:options.py:524 Setting options using an autogenerated prefix 'firedrake_0_' is unsafe:
  firedrake_0_ksp_monitor
  firedrake_0_ksp_type
  firedrake_0_mat_type
  firedrake_0_mg_levels_pc_type
  firedrake_0_pc_type
  ...

After this PR the prefixes are firedrake_0_, firedrake_1_, firedrake_2_
again, and nothing is inherited.

What's in it

  • petsctools/options.py: increment the counter on OptionsManager rather
    than on self, so the class attribute actually advances.
  • tests/test_options.py: test_options_prefix now asserts that successive
    autogenerated prefixes differ. It fails on main and passes here.

OptionsManager.count keeps its meaning — the index the next autogenerated
prefix will use — so test_commandline_options, which reads it to predict that
prefix, is unaffected.

Why this matters downstream

Firedrake depends on petsctools @ git+https://github.com/firedrakeproject/petsctools.git@main,
unpinned. #39 merged at 11:32 UTC on 26 August; the next Firedrake CI run on
main, at 12:05 UTC, went red, and it has been red since:

Firedrake test before #39 after #39
macro/test_macro_multigrid.py::test_macro_multigrid_biharmonic[HCT, HCT-red] pass fail
multigrid/test_snes_adapt.py::test_snes_adapt_sequence_with_adaptive_multigrid[1, 2] pass fail
regression/test_appctx_cleanup.py::test_appctx_cleanup pass fail
regression/test_stress_elements.py::test_stress_displacement_convergence[conforming, high-order] pass fail
regression/test_bddc.py::test_bddc_cellwise_fdm[cube-nprocs=1-True-E-3] pass fail

All of them fail the same way. A solver runs, and inside its residual
assembly Firedrake builds a second, unrelated solver — the mass solve behind
Mesh.cell_sizes, say. That inner solver takes the same firedrake_0_ prefix,
inherits the outer solver's -pc_type mg, and then dies trying to coarsen a
problem that has no mesh hierarchy of its own:

petsc4py.PETSc.Error: error code 101
[0] SNESSolve() ... [0] PCSetUp_MG() ... [0] DMCoarsen()
TypeError: Incompatible function spaces in Action

Notes for review

Checked against Firedrake by putting this checkout ahead of the installed
petsctools on PYTHONPATH: each of the tests above fails on main and passes
with this commit. tests/test_options.py passes in full (18 tests) and ruff
is clean.

The remaining red tests on Firedrake maintest_io_backward_compat and an
intermittent xdist worker segfault in test_assemble_baseform — predate #39 and
are untouched by this.

@connorjward

Copy link
Copy Markdown
Collaborator

Thanks but already fixed in #52

@pbrubeck
pbrubeck deleted the pbrubeck/fix-autogenerated-prefix branch August 27, 2026 14:23
@pbrubeck
pbrubeck restored the pbrubeck/fix-autogenerated-prefix branch August 27, 2026 14:59
@pbrubeck pbrubeck reopened this Aug 27, 2026
pbrubeck and others added 2 commits August 27, 2026 16:01
`self.count += 1` reads the class attribute and rebinds the result onto
the instance, so `OptionsManager.count` stayed at zero and every manager
built without an explicit `options_prefix` was handed the same prefix,
`{default_prefix}0_`.

Sharing a prefix means sharing a namespace in the global options
database. A manager built while another manager's options are inserted
picks those options up as if they had been passed on the command line.
In Firedrake this makes any solver constructed inside a solve inherit
the outer solver's parameters: the mass solve behind
`Mesh.cell_sizes`, for instance, runs with the outer `-pc_type mg` and
then fails to coarsen a problem that has no hierarchy of its own.

Increment the counter on `OptionsManager` itself, so that one counter
serves the whole hierarchy. `type(self).count += 1` is not enough:
`OptionsManager` is used as a mixin -- Firedrake has
`NonlinearVariationalSolver` -> `LinearVariationalSolver` ->
`LinearSolver`, and `LinearEigensolver` -- and it reads the inherited
value but writes the incremented one onto the subclass, so each class
walks a counter of its own and prefixes repeat across the hierarchy:

    firedrake_2_ handed to both a LinearVariationalSolver and a
    NonlinearVariationalSolver

`test_options_prefix` builds two subclasses and the base class in turn
and checks the prefixes are distinct, which fails on either spelling of
the bug.

Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
@pbrubeck
pbrubeck force-pushed the pbrubeck/fix-autogenerated-prefix branch from 01e68d4 to a5e8ad3 Compare August 27, 2026 15:02
@pbrubeck

Copy link
Copy Markdown
Contributor Author

Rebased onto main and resolved the conflict, as two commits:

  1. Revert "Use class count, not instance (#52)" — clean revert, no conflict.
  2. Give each autogenerated options prefix a unique index — the fix, on OptionsManager itself.

Net effect against main is type(self).countOptionsManager.count, plus the test.

Why not type(self). It reads the inherited value but writes the incremented one onto the subclass, so each class walks a counter of its own and prefixes repeat across a hierarchy. OptionsManager is used as a mixin, so this is live for Firedrake — NonlinearVariationalSolverLinearVariationalSolverLinearSolver, and LinearEigensolver. Building three of each in turn against 729712d:

firedrake_1_ ['NVS']
firedrake_2_ ['LVS', 'NVS']                  <-- collision
firedrake_3_ ['LinearSolver', 'LVS', 'NVS']  <-- collision
firedrake_4_ ['LinearSolver', 'LVS']         <-- collision
firedrake_5_ ['LinearSolver']

The test now catches it. The version on the closed PR only built three managers of the same class, which is unique under type(self).count — it would have passed on main and guarded nothing. It now builds two subclasses and the base class in turn, and fails on either spelling of the bug.

Verified against Firedrake by putting this branch ahead of the installed petsctools on PYTHONPATH: test_macro_multigrid_biharmonic[HCT, HCT-red], test_appctx_cleanup, test_snes_adapt and test_stress_displacement_convergence all pass. tests/test_options.py passes in full (18) and ruff is clean.

AI-assisted: written with Claude Code.

@connorjward
connorjward merged commit f87b588 into main Aug 27, 2026
2 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants