API: forbid overrides in BoundaryRegistry.register unless unsafe mutations are explicitly allowed.

Author: neutrinoceros

src/gpgi/_boundaries.py

@@ -1,6 +1,5 @@
 from __future__ import annotations
 
-import warnings
 from collections.abc import Callable
 from typing import TYPE_CHECKING, Any, Literal, cast
 
@@ -60,13 +59,59 @@ def _validate_recipe(recipe: BoundaryRecipeT) -> None:
             )
 
     def register(
-        self, key: str, recipe: BoundaryRecipeT, *, skip_validation: bool = False
+        self,
+        key: str,
+        recipe: BoundaryRecipeT,
+        *,
+        skip_validation: bool = False,
+        allow_unsafe_override: bool = False,
     ) -> None:
+        """
+        Register a new boundary function.
+
+        Parameters
+        ----------
+        key: str
+            A unique identifier (ideally a meaningful name) to associate with
+            the function.
+
+        recipe: Callable
+            A function matching the signature (order and names of arguments) of
+            gpgi's builtin boundary recipes.
+
+        skip_validation: bool, optional, keyword-only (default: False)
+            If set to True, signature validation is skipped.
+            This is meant to allow bypassing hypothetical bugs in the validation
+            routine.
+
+        allow_unsafe_override: bool, optional, keyword-only (default: False)
+            If set to True, registering a new function under an existing key
+            will not raise an exception. Note however that doing so is not
+            thread-safe.
+
+        Raises
+        ------
+        ValueError:
+        - if skip_validation==False and the signature of the recipe doesn't meet
+          the requirements.
+        - if allow_unsafe_override==False and a new function is being registered
+          under an already used key. Registering the same exact function under
+          multiple times either under the same key or another, unused key, is
+          always safe so it does not raise.
+        """
+        if key in self._registry:
+            if recipe is self._registry[key]:
+                return
+            elif not allow_unsafe_override:
+                raise ValueError(
+                    f"Another function is already registered with {key=!r}. "
+                    "If you meant to override the existing function, "
+                    "consider setting allow_unsafe_override=True"
+                )
+
         if not skip_validation:
             self._validate_recipe(recipe)
 
-        if key in self._registry:
-            warnings.warn(f"Overriding existing method {key!r}", stacklevel=2)
         self._registry[key] = recipe
 
     def __getitem__(self, key: str) -> BoundaryRecipeT:

tests/test_boundary_registry.py

@@ -0,0 +1,51 @@
+import pytest
+
+from gpgi._boundaries import BoundaryRegistry
+
+
+def test_boundary_register_overrides():
+    registry = BoundaryRegistry()
+
+    def test_recipe1(
+        same_side_active_layer,
+        same_side_ghost_layer,
+        opposite_side_active_layer,
+        opposite_side_ghost_layer,
+        weight_same_side_active_layer,
+        weight_same_side_ghost_layer,
+        weight_opposite_side_active_layer,
+        weight_opposite_side_ghost_layer,
+        side,
+        metadata,
+    ): ...
+    def test_recipe2(
+        same_side_active_layer,
+        same_side_ghost_layer,
+        opposite_side_active_layer,
+        opposite_side_ghost_layer,
+        weight_same_side_active_layer,
+        weight_same_side_ghost_layer,
+        weight_opposite_side_active_layer,
+        weight_opposite_side_ghost_layer,
+        side,
+        metadata,
+    ): ...
+
+    registry.register("test1", test_recipe1)
+    assert registry["test1"] is test_recipe1
+
+    # registering the same function a second time shouldn't raise
+    registry.register("test1", test_recipe1)
+
+    with pytest.raises(
+        ValueError,
+        match="Another function is already registered with key='test1'",
+    ):
+        registry.register("test1", test_recipe2)
+
+    # check that we raised in time to preserve state
+    assert registry["test1"] is test_recipe1
+
+    # if we explicitly allow unsafe mutations, this should not raise
+    registry.register("test1", test_recipe2, allow_unsafe_override=True)
+    assert registry["test1"] is test_recipe2

tests/test_deposit.py

@@ -396,54 +396,6 @@ def _my_recipe(a, b, c, d, e, f):
         ds.boundary_recipes.register("my", _my_recipe)
 
 
-def test_warn_register_override(capsys):
-    nx = ny = 64
-    nparticles = 100
-
-    prng = np.random.RandomState(0)
-    ds = gpgi.load(
-        geometry="cartesian",
-        grid={
-            "cell_edges": {
-                "x": np.linspace(-1, 1, nx),
-                "y": np.linspace(-1, 1, ny),
-            },
-        },
-        particles={
-            "coordinates": {
-                "x": 2 * (prng.normal(0.5, 0.25, nparticles) % 1 - 0.5),
-                "y": 2 * (prng.normal(0.5, 0.25, nparticles) % 1 - 0.5),
-            },
-            "fields": {
-                "mass": np.ones(nparticles),
-            },
-        },
-        metadata={"fac": 1},
-    )
-
-    def _my_recipe(
-        same_side_active_layer,
-        same_side_ghost_layer,
-        opposite_side_active_layer,
-        opposite_side_ghost_layer,
-        weight_same_side_active_layer,
-        weight_same_side_ghost_layer,
-        weight_opposite_side_active_layer,
-        weight_opposite_side_ghost_layer,
-        side,
-        metadata,
-    ):
-        print("gotcha")
-        return same_side_active_layer * metadata["fac"]
-
-    with pytest.warns(UserWarning, match="Overriding existing method 'open'"):
-        ds.boundary_recipes.register("open", _my_recipe)
-
-    ds.deposit("mass", method="tsc")
-    out, err = capsys.readouterr()
-    assert out == "gotcha\n" * 4
-
-
 def test_register_custom_boundary_recipe(sample_2D_dataset):
     def _my_recipe(
         same_side_active_layer,