Merge pull request #699 from lsst/tickets/DM-35551

DM-35551: fix ConfigurableAction tests and make serialization deterministic

python/lsst/pipe/tasks/configurableActions/_configurableActionStructField.py

@@ -365,7 +365,7 @@ def save(self, outfile, instance):
         if actionStruct is None:
             return
 
-        for v in actionStruct:
+        for _, v in sorted(actionStruct.items()):
             outfile.write(u"{}={}()\n".format(v._name, _typeStr(v)))
             v._save(outfile)
 

python/lsst/pipe/tasks/configurableActions/tests.py

@@ -0,0 +1,68 @@
+# This file is part of pipe_tasks.
+#
+# Developed for the LSST Data Management System.
+# This product includes software developed by the LSST Project
+# (https://www.lsst.org).
+# See the COPYRIGHT file at the top-level directory of this distribution
+# for details of code ownership.
+#
+# This program is free software: you can redistribute it and/or modify
+# it under the terms of the GNU General Public License as published by
+# the Free Software Foundation, either version 3 of the License, or
+# (at your option) any later version.
+#
+# This program is distributed in the hope that it will be useful,
+# but WITHOUT ANY WARRANTY; without even the implied warranty of
+# MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
+# GNU General Public License for more details.
+#
+# You should have received a copy of the GNU General Public License
+# along with this program.  If not, see <https://www.gnu.org/licenses/>.
+from __future__ import annotations
+
+__all__ = (
+    "ActionTest1",
+    "ActionTest2",
+    "ActionTest3",
+    "TestConfig",
+)
+
+from lsst.pex.config import Config, Field
+from ._configurableAction import ConfigurableAction
+from ._configurableActionStructField import ConfigurableActionStructField
+from ._configurableActionField import ConfigurableActionField
+
+
+class ActionTest1(ConfigurableAction):
+    var = Field(doc="test field", dtype=int, default=0)
+
+    def __call__(self):
+        return self.var
+
+    def validate(self):
+        assert(self.var is not None)
+
+
+class ActionTest2(ConfigurableAction):
+    var = Field(doc="test field", dtype=int, default=1)
+
+    def __call__(self):
+        return self.var
+
+    def validate(self):
+        assert(self.var is not None)
+
+
+class ActionTest3(ConfigurableAction):
+    var = Field(doc="test field", dtype=int, default=3)
+
+    def __call__(self):
+        return self.var
+
+    def validate(self):
+        assert(self.var is not None)
+
+
+class TestConfig(Config):
+    actions = ConfigurableActionStructField(doc="Actions to be tested", default=None)
+    singleAction = ConfigurableActionField(doc="A configurable action", default=None)

python/lsst/pipe/tasks/dataFrameActions/_evalColumnExpression.py

@@ -131,7 +131,7 @@ def makeColumnExpressionAction(className: str, expr: str,
     names -= EXTRA_MATH.keys()
 
     fields: Mapping[str, ConfigurableActionField] = {}
-    for name in names:
+    for name in sorted(names):
         if exprDefaults is not None and (value := exprDefaults.get(name)) is not None:
             kwargs = {"default": value}
         else:

tests/test_configurableActions.py

@@ -23,50 +23,31 @@
 from io import StringIO
 from types import SimpleNamespace
 
-from lsst.pipe.tasks.configurableActions import (ConfigurableActionStructField, ConfigurableAction,
-                                                 ConfigurableActionField)
-from lsst.pex.config import Config, Field, FieldValidationError
+from lsst.pipe.tasks.configurableActions.tests import (
+    ActionTest1,
+    ActionTest2,
+    ActionTest3,
+    TestConfig,
+)
+from lsst.pipe.tasks.dataFrameActions import DivideColumns, SingleColumnAction
+
+from lsst.pex.config import FieldValidationError
 from lsst.pipe.base import Struct
 
 
-class ActionTest1(ConfigurableAction):
-    var = Field(doc="test field", dtype=int, default=0)
-
-    def __call__(self):
-        return self.var
-
-    def validate(self):
-        assert(self.var is not None)
-
-
-class ActionTest2(ConfigurableAction):
-    var = Field(doc="test field", dtype=int, default=1)
-
-    def __call__(self):
-        return self.var
-
-    def validate(self):
-        assert(self.var is not None)
-
-
-class ActionTest3(ConfigurableAction):
-    var = Field(doc="test field", dtype=int, default=3)
-
-    def __call__(self):
-        return self.var
-
-    def validate(self):
-        assert(self.var is not None)
-
-
 class ConfigurableActionsTestCase(unittest.TestCase):
     def _createConfig(self, default=None, singleDefault=None):
-        class TestConfig(Config):
-            actions = ConfigurableActionStructField(doc="Actions to be tested", default=default)
-            singleAction = ConfigurableActionField(doc="A configurable action", default=singleDefault)
-        return TestConfig
-
-    def testConfigInstatiation(self):
+        class NewTestConfig(TestConfig):
+            def setDefaults(self):
+                super().setDefaults()
+                if default is not None:
+                    for k, v in default.items():
+                        setattr(self.actions, k, v)
+                if singleDefault is not None:
+                    self.singleAction = singleDefault
+        return NewTestConfig
+
+    def testConfigInstantiation(self):
         # This will raise if there is an issue instantiating something
         configClass = self._createConfig()
         config = configClass()
@@ -231,17 +212,36 @@ def testSave(self):
         # This method will also test rename, as it is part of the
         # implementation in pex_config
         ioObject = StringIO()
-        configClass = self._createConfig(default={"test1": ActionTest1},
-                                         singleDefault=ActionTest1)
-        config = configClass()
+        config = TestConfig()
+        config.actions.test1 = ActionTest1
+        config.actions.test2 = ActionTest2
+        config.singleAction = DivideColumns(
+            colA=SingleColumnAction(column="a"),
+            colB=SingleColumnAction(column="b"),
+        )
 
         config.saveToStream(ioObject)
-        loadedConfig = configClass()
-        loadedConfig.loadFromStream(ioObject.read())
-        self.assertTrue(config.compare(loadedConfig))
+        string1 = ioObject.getvalue()
+        loadedConfig = TestConfig()
+        loadedConfig.loadFromStream(string1)
+        self.assertTrue(config.compare(loadedConfig), msg=f"{config} != {loadedConfig}")
         # Be sure that the fields are actually there
         self.assertEqual(loadedConfig.actions.test1.var, 0)
-        self.assertEqual(loadedConfig.singleAction.var, 0)
+        self.assertEqual(loadedConfig.singleAction.colA.column, "a")
+        self.assertEqual(loadedConfig.singleAction.colB.column, "b")
+        # Save an equivalent struct with fields originally ordered differently,
+        # check that the saved form is the same (via deterministic sorting).
+        config2 = TestConfig()
+        config2.actions.test2 = ActionTest2
+        config2.actions.test1 = ActionTest1
+        config2.singleAction = DivideColumns(
+            colB=SingleColumnAction(column="b"),
+            colA=SingleColumnAction(column="a"),
+        )
+        ioObject2 = StringIO()
+        config2.saveToStream(ioObject2)
+        self.maxDiff = None
+        self.assertEqual(string1, ioObject2.getvalue())
 
     def testToDict(self):
         """Test the toDict interface"""