From a0fd39a2d8cda8a4dd1b5213e6863dceca1900ec Mon Sep 17 00:00:00 2001 From: Connor Ward Date: Fri, 19 Jun 2026 15:38:45 +0100 Subject: [PATCH 1/4] improving things --- petsctools/options.py | 94 +++++++++++++++++++------------------------ tests/test_options.py | 44 ++++++++++++++++++++ 2 files changed, 85 insertions(+), 53 deletions(-) diff --git a/petsctools/options.py b/petsctools/options.py index be92fd5..afcbe11 100644 --- a/petsctools/options.py +++ b/petsctools/options.py @@ -280,6 +280,7 @@ def get_default_options(default_options_set: DefaultOptionSet, return default_options +# TODO: Add note about how we 'freeze' options at instantiation class OptionsManager: """Class that helps with managing setting PETSc options. @@ -405,14 +406,13 @@ class OptionsManager: AppContextManager """ - count = itertools.count() + count = 0 def __init__(self, parameters: dict, options_prefix: str | None = None, default_prefix: str | None = None, default_options_set: DefaultOptionSet | None = None, appmngr: AppContextManager | None = None): - super().__init__() if parameters is None: parameters = {} else: @@ -420,61 +420,48 @@ def __init__(self, parameters: dict, parameters = flatten_parameters(parameters) # If no prefix is provided generate a default prefix - # and ignore any command line options if options_prefix is None: default_prefix = default_prefix or "petsctools_" default_prefix = _validate_prefix(default_prefix) - self.options_prefix = f"{default_prefix}{next(self.count)}_" - self.parameters = parameters - self.to_delete = set(parameters) - + options_prefix = f"{default_prefix}{self.count}_" + self.count += 1 + unsafe_prefix = True else: options_prefix = _validate_prefix(options_prefix) - self.options_prefix = options_prefix - - # Are we part of a solver set sharing defaults? - if default_options_set: - if options_prefix not in default_options_set.custom_prefixes: - raise ValueError( - f"The options_prefix {options_prefix} must be one" - f" of the custom_prefixes of the DefaultOptionSet" - f" {default_options_set.custom_prefixes}") - default_options = get_default_options( - default_options_set, self.options_object) - else: - default_options = {} - - # Note: we need to know which parameters to_delete - # so we need to exclude the relevant command line - # options when combining the parameters from the - # defaults and the source code. - - # Start building parameters from the defaults so - # that they will overwritten by any other source. - self.parameters = { - k: v - for k, v in default_options.items() - if options_prefix + k not in get_commandline_options() - } - - # Update using the parameters passed in the code but - # exclude those options from the dict that were passed - # on the commandline because those have global scope and are - # not under the control of the options manager. - self.parameters.update({ - k: v - for k, v in parameters.items() - if options_prefix + k not in get_commandline_options() - }) - self.to_delete = set(self.parameters) - - # Now update parameters from options, so that they're - # available to solver setup (for, e.g., matrix-free). - # Can't ask for the prefixed guy in the options object, - # since that does not DTRT for flag options. - for k, v in self.options_object.getAll().items(): - if k.startswith(self.options_prefix): - self.parameters[k[len(self.options_prefix):]] = v + unsafe_prefix = False + + # Are we part of a solver set sharing defaults? + if default_options_set: + if options_prefix not in default_options_set.custom_prefixes: + raise ValueError( + f"The options_prefix {options_prefix} must be one" + f" of the custom_prefixes of the DefaultOptionSet" + f" {default_options_set.custom_prefixes}") + default_options = get_default_options( + default_options_set, self.options_object) + else: + default_options = {} + + # Start building parameters from the defaults so + # that they will overwritten by any other source. + parameters = default_options | parameters + + # The parameters to drop from the global options when we leave the + # inserted_options context. This is everything except for options + # passed on the command line. + to_delete = set(parameters.keys()) + for k, v in self.options_object.getAll().items(): + if k.startswith(options_prefix): + if unsafe_prefix: + print("WARN") + parameters[k[len(options_prefix):]] = v + if k in to_delete: + # option is set globally, don't drop when we leave the context + to_delete.delete(k) + + self.parameters = parameters + self.to_delete = to_delete + self.options_prefix = options_prefix self._setfromoptions = False @@ -562,12 +549,13 @@ def inserted_options(self): for k in self.to_delete: if self.options_object.used(self.options_prefix + k): self._used_options.add(k) - del self.options_object[self.options_prefix + k] + del self.options_object[k] @functools.cached_property def options_object(self): from petsc4py import PETSc + # We can't pass the prefix here because that doesn't DTRT for flag options return PETSc.Options() diff --git a/tests/test_options.py b/tests/test_options.py index a4d5130..9009e9b 100644 --- a/tests/test_options.py +++ b/tests/test_options.py @@ -157,3 +157,47 @@ def test_default_options(): assert options2.parameters["opt2"] == "2" assert options2.parameters["opt3"] == "3" assert options2.parameters["opt4"] == "6" + + +@pytest.mark.parametrize("options_prefix", (None, "", "custom_")) +def test_commandline_options(options_prefix): + from petsc4py import PETSc + + if options_prefix is None: + true_prefix = f"petsctools_{petsctools.OptionsManager.count}_" + else: + true_prefix = options_prefix + + # Put some options in the database as though they were passed by a user on + # the command line + options = PETSc.Options() + options["opt1"] = "unused" + options[f"{true_prefix}opt2"] = "will_overwrite" + options[f"{true_prefix}opt3"] = "extra" + + default_params = { + # this will get ignored because we pass something on the command line instead + "opt2": "default_opt2", + # this will be inserted and popped from the database + "opt4": "default_opt4", + } + om = petsctools.OptionsManager(default_params, options_prefix=options_prefix) + + assert om.options_prefix == true_prefix + + with om.inserted_options(): + assert options["opt1"] == "unused" + assert options[f"{om.options_prefix}opt2"] == "will_overwrite" + assert options[f"{om.options_prefix}opt3"] == "extra" + assert options[f"{om.options_prefix}opt4"] == "default_opt4" + + # make sure the command line options are persistent + assert options["opt1"] == "unused" + assert options[f"{om.options_prefix}opt2"] == "will_overwrite" + assert options[f"{om.options_prefix}opt3"] == "extra" + assert f"{om.options_prefix}opt4" not in options + + # TODO + # make sure we warn on usage if prefix is None + # and the appctx too + From f259e93857642d14710fa4258c020277bf5a851c Mon Sep 17 00:00:00 2001 From: Connor Ward Date: Fri, 19 Jun 2026 16:04:48 +0100 Subject: [PATCH 2/4] And warn --- petsctools/options.py | 33 ++++++++++++++++++++++----------- tests/test_options.py | 9 +++++++-- 2 files changed, 29 insertions(+), 13 deletions(-) diff --git a/petsctools/options.py b/petsctools/options.py index afcbe11..d353920 100644 --- a/petsctools/options.py +++ b/petsctools/options.py @@ -10,12 +10,13 @@ import petsc4py +import petsctools.log +from petsctools.appctx import AppContextManager from petsctools.exceptions import ( PetscToolsException, PetscToolsWarning, PetscToolsNotInitialisedException, ) -from petsctools.appctx import AppContextManager _commandline_options = None @@ -450,14 +451,23 @@ def __init__(self, parameters: dict, # inserted_options context. This is everything except for options # passed on the command line. to_delete = set(parameters.keys()) - for k, v in self.options_object.getAll().items(): - if k.startswith(options_prefix): - if unsafe_prefix: - print("WARN") - parameters[k[len(options_prefix):]] = v - if k in to_delete: - # option is set globally, don't drop when we leave the context - to_delete.delete(k) + warned = False + for full_key, v in self.options_object.getAll().items(): + if full_key.startswith(options_prefix): + key = full_key[len(options_prefix):] + + if unsafe_prefix and not warned: + petsctools.log.warning( + "Setting options using an autogenerated prefix " + f"({options_prefix}) is unsafe" + ) + warned = True # only warn once + + parameters[key] = v + + if key in to_delete: + # option is set globally, don't drop when we leave the context + to_delete.remove(key) self.parameters = parameters self.to_delete = to_delete @@ -546,10 +556,11 @@ def inserted_options(self): else: yield finally: - for k in self.to_delete: + for k in self.parameters: if self.options_object.used(self.options_prefix + k): self._used_options.add(k) - del self.options_object[k] + for k in self.to_delete: + del self.options_object[self.options_prefix + k] @functools.cached_property def options_object(self): diff --git a/tests/test_options.py b/tests/test_options.py index 9009e9b..2cc0213 100644 --- a/tests/test_options.py +++ b/tests/test_options.py @@ -160,7 +160,7 @@ def test_default_options(): @pytest.mark.parametrize("options_prefix", (None, "", "custom_")) -def test_commandline_options(options_prefix): +def test_commandline_options(caplog, options_prefix): from petsc4py import PETSc if options_prefix is None: @@ -182,7 +182,6 @@ def test_commandline_options(options_prefix): "opt4": "default_opt4", } om = petsctools.OptionsManager(default_params, options_prefix=options_prefix) - assert om.options_prefix == true_prefix with om.inserted_options(): @@ -191,6 +190,12 @@ def test_commandline_options(options_prefix): assert options[f"{om.options_prefix}opt3"] == "extra" assert options[f"{om.options_prefix}opt4"] == "default_opt4" + if options_prefix is None: + assert len(caplog.records) == 1 + assert caplog.messages[0].startswith("Setting options using an autogenerated prefix") + else: + assert not caplog.records + # make sure the command line options are persistent assert options["opt1"] == "unused" assert options[f"{om.options_prefix}opt2"] == "will_overwrite" From 1d1e60523cc9106e55f88bb40b39fb225250e37b Mon Sep 17 00:00:00 2001 From: Connor Ward Date: Fri, 19 Jun 2026 16:20:18 +0100 Subject: [PATCH 3/4] linting --- petsctools/options.py | 7 ++++--- tests/test_options.py | 10 +++++++--- 2 files changed, 11 insertions(+), 6 deletions(-) diff --git a/petsctools/options.py b/petsctools/options.py index d353920..0ac151d 100644 --- a/petsctools/options.py +++ b/petsctools/options.py @@ -3,7 +3,6 @@ import weakref import contextlib import functools -import itertools import warnings from functools import cached_property from typing import Any, Iterable @@ -466,7 +465,8 @@ def __init__(self, parameters: dict, parameters[key] = v if key in to_delete: - # option is set globally, don't drop when we leave the context + # option is set globally, don't drop when we exit the + # context manager to_delete.remove(key) self.parameters = parameters @@ -566,7 +566,8 @@ def inserted_options(self): def options_object(self): from petsc4py import PETSc - # We can't pass the prefix here because that doesn't DTRT for flag options + # We can't pass the prefix here because that doesn't DTRT + # for flag options return PETSc.Options() diff --git a/tests/test_options.py b/tests/test_options.py index 2cc0213..8adabeb 100644 --- a/tests/test_options.py +++ b/tests/test_options.py @@ -176,12 +176,14 @@ def test_commandline_options(caplog, options_prefix): options[f"{true_prefix}opt3"] = "extra" default_params = { - # this will get ignored because we pass something on the command line instead + # this will get ignored because we pass something on the command line "opt2": "default_opt2", # this will be inserted and popped from the database "opt4": "default_opt4", } - om = petsctools.OptionsManager(default_params, options_prefix=options_prefix) + om = petsctools.OptionsManager( + default_params, options_prefix=options_prefix + ) assert om.options_prefix == true_prefix with om.inserted_options(): @@ -192,7 +194,9 @@ def test_commandline_options(caplog, options_prefix): if options_prefix is None: assert len(caplog.records) == 1 - assert caplog.messages[0].startswith("Setting options using an autogenerated prefix") + assert caplog.messages[0].startswith( + "Setting options using an autogenerated prefix" + ) else: assert not caplog.records From 9fe0e06b8b01dd1ed9bf831833dc225e07928097 Mon Sep 17 00:00:00 2001 From: Connor Ward Date: Fri, 19 Jun 2026 16:31:16 +0100 Subject: [PATCH 4/4] fixup --- petsctools/log.py | 10 ++++++++++ tests/test_options.py | 1 + 2 files changed, 11 insertions(+) create mode 100644 petsctools/log.py diff --git a/petsctools/log.py b/petsctools/log.py new file mode 100644 index 0000000..7a95853 --- /dev/null +++ b/petsctools/log.py @@ -0,0 +1,10 @@ +import logging + + +LOGGER = logging.getLogger("petsctools") + +debug = LOGGER.debug +info = LOGGER.info +warning = LOGGER.warning +error = LOGGER.error +critical = LOGGER.critical diff --git a/tests/test_options.py b/tests/test_options.py index 8adabeb..7b274ce 100644 --- a/tests/test_options.py +++ b/tests/test_options.py @@ -159,6 +159,7 @@ def test_default_options(): assert options2.parameters["opt4"] == "6" +@pytest.mark.skipnopetsc4py @pytest.mark.parametrize("options_prefix", (None, "", "custom_")) def test_commandline_options(caplog, options_prefix): from petsc4py import PETSc