Refactor options manager

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

petsctools/options.py

@@ -3,19 +3,19 @@
 import weakref
 import contextlib
 import functools
-import itertools
 import warnings
 from functools import cached_property
 from typing import Any, Iterable
 
 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
@@ -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,76 +406,72 @@ 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:
             # Convert nested dicts
             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())
+        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 exit the
+                    # context manager
+                    to_delete.remove(key)
+
+        self.parameters = parameters
+        self.to_delete = to_delete
+        self.options_prefix = options_prefix
 
         self._setfromoptions = False
 
@@ -559,15 +556,18 @@ 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)
+            for k in self.to_delete:
                 del self.options_object[self.options_prefix + 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()
 
 

tests/test_options.py

@@ -157,3 +157,57 @@ def test_default_options():
     assert options2.parameters["opt2"] == "2"
     assert options2.parameters["opt3"] == "3"
     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
+
+    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
+        "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"
+
+    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"
+    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
+