Modify applying config overrides for flexibility

These changes allow greater flexibility when doing config overrieds
in areas such as not needing string escaping when working with the
command line, and using values from python statements in other
parts of pipeline config blocks.

python/lsst/pipe/base/config.py

@@ -26,6 +26,7 @@
 # -------------------------------
 #  Imports of standard modules --
 # -------------------------------
+from numbers import Number
 
 # -----------------------------
 #  Imports for other modules --
@@ -42,6 +43,18 @@
 # ------------------------
 
 
+class TemplateField(pexConfig.Field):
+    def _validateValue(self, value):
+        if value is None:
+            return
+
+        if not (isinstance(value, str) or isinstance(value, Number)):
+            raise TypeError(f"Value {value} is of incorrect type {pexConfig.config._typeStr(value)}."
+                            f" Expected type str or a number")
+        if self.check is not None and not self.check(value):
+            ValueError("Value {value} is not a valid value")
+
+
 class PipelineTaskConfigMeta(pexConfig.ConfigMeta):
     """Metaclass used in the creation of PipelineTaskConfig classes
 
@@ -85,9 +98,9 @@ def __new__(cls, name, bases, dct, **kwargs):
             if hasattr(connectionsClass, 'defaultTemplates'):
                 docString = "Template parameter used to format corresponding field template parameter"
                 for templateName, default in connectionsClass.defaultTemplates.items():
-                    configConnectionsNamespace[templateName] = pexConfig.Field(dtype=str,
-                                                                               doc=docString,
-                                                                               default=default)
+                    configConnectionsNamespace[templateName] = TemplateField(dtype=str,
+                                                                             doc=docString,
+                                                                             default=default)
             # add a reference to the connection class used to create this sub
             # config
             configConnectionsNamespace['ConnectionsClass'] = connectionsClass

python/lsst/pipe/base/configOverrides.py

@@ -25,15 +25,100 @@
 __all__ = ["ConfigOverrides"]
 
 import ast
+from operator import attrgetter
 
 import lsst.pex.exceptions as pexExceptions
 from lsst.utils import doImport
 
+import inspect
 from enum import Enum
 
 OverrideTypes = Enum("OverrideTypes", "Value File Python Instrument")
 
 
+class ConfigExpressionParser(ast.NodeVisitor):
+    """An expression parser that will be used to transform configuration
+    strings supplied from the command line or a pipeline into a python
+    object.
+
+    This is roughly equivalent to ast.literal_parser, but with the ability to
+    transform strings that are valid variable names into the value associated
+    with the name. Variables that should be considered valid are supplied to
+    the constructor as a dictionary that maps a string to its corresponding
+    value.
+
+    This class in an internal implementation detail, and should not be exposed
+    outside this module.
+
+    Parameters
+    ----------
+    namespace : `dict` of `str` to variable
+        This is a mapping of strings corresponding to variable names, to the
+        object that is associated with that name
+    """
+
+    def __init__(self, namespace):
+        self.variables = namespace
+
+    def visit_Name(self, node):
+        """This method gets called when the parser has determined a node
+        corresponds to a variable name.
+        """
+        # If the id (name) of the variable is in the dictionary of valid names,
+        # load and return the corresponding variable.
+        if node.id in self.variables:
+            return self.variables[node.id]
+        # If the node does not correspond to a valid variable, turn the name
+        # into a string, as the user likely intended it as such.
+        return f"{node.id}"
+
+    def visit_List(self, node):
+        """This method is visited if the node is a list. Constructs a list out
+        of the sub nodes.
+        """
+        return [self.visit(elm) for elm in node.elts]
+
+    def visit_Tuple(self, node):
+        """This method is visited if the node is a tuple. Constructs a list out
+        of the sub nodes, and then turns it into a tuple.
+        """
+        return tuple(self.visit_List(node))
+
+    def visit_Constant(self, node):
+        """This method is visited if the node is a constant
+        """
+        return node.value
+
+    def visit_Dict(self, node):
+        """This method is visited if the node is a dict. It builds a dict out
+        of the component nodes.
+        """
+        return {self.visit(key): self.visit(value) for key, value in zip(node.keys, node.values)}
+
+    def visit_Set(self, node):
+        """This method is visited if the node is a set. It builds a set out
+        of the component nodes.
+        """
+        return {self.visit(el) for el in node.elts}
+
+    def visit_UnaryOp(self, node):
+        """This method is visited if the node is a unary operator. Currently
+        The only operator we support is the negative (-) operator, all others
+        are passed to generic_visit method.
+        """
+        if isinstance(node.op, ast.USub):
+            value = self.visit(node.operand)
+            return -1*value
+        self.generic_visit(node)
+
+    def generic_visit(self, node):
+        """This method is called for all other node types. It will just raise
+        a value error, because this is a type of expression that we do not
+        support.
+        """
+        raise ValueError("Unable to parse string into literal expression")
+
+
 class ConfigOverrides:
     """Defines a set of overrides to be applied to a task config.
 
@@ -114,6 +199,15 @@ def addInstrumentOverride(self, instrument: str, task_name: str):
         instrument_lib = doImport(instrument)()
         self._overrides.append((OverrideTypes.Instrument, (instrument_lib, task_name)))
 
+    def _parser(self, value, configParser):
+        try:
+            value = configParser.visit(ast.parse(value, mode='eval').body)
+        except ValueError:
+            # eval failed, wrap exception with more user-friendly
+            # message
+            raise pexExceptions.RuntimeError(f"Unable to parse `{value}' into a Python object") from None
+        return value
+
     def applyTo(self, config):
         """Apply all overrides to a task configuration object.
 
@@ -125,35 +219,76 @@ def applyTo(self, config):
         ------
         `Exception` is raised if operations on configuration object fail.
         """
+        # Look up a stack of variables people may be using when setting
+        # configs. Create a dictionary that will be used akin to a namespace
+        # for the duration of this function.
+        vars = {}
+        # pull in the variables that are declared in module scope of the config
+        mod = inspect.getmodule(config)
+        vars.update({k: v for k, v in mod.__dict__.items() if not k.startswith("__")})
+        # put the supplied config in the variables dictionary
+        vars['config'] = config
+
+        # Create a parser for config expressions that may be strings
+        configParser = ConfigExpressionParser(namespace=vars)
+
         for otype, override in self._overrides:
             if otype is OverrideTypes.File:
                 config.load(override)
             elif otype is OverrideTypes.Value:
                 field, value = override
-                field = field.split('.')
-                # find object with attribute to set, throws if we name is wrong
-                obj = config
-                for attr in field[:-1]:
-                    obj = getattr(obj, attr)
-                # If input is a string and field type is not a string then we
-                # have to convert string to an expected type. Implementing
-                # full string parser is non-trivial so we take a shortcut here
-                # and `eval` the string and assign the resulting value to a
-                # field. Type erroes can happen during both `eval` and field
-                # assignment.
-                if isinstance(value, str) and obj._fields[field[-1]].dtype is not str:
-                    try:
-                        # use safer ast.literal_eval, it only supports literals
-                        value = ast.literal_eval(value)
-                    except Exception:
-                        # eval failed, wrap exception with more user-friendly
-                        # message
-                        raise pexExceptions.RuntimeError(f"Unable to parse `{value}' into a Python object")
-
-                # this can throw in case of type mismatch
-                setattr(obj, field[-1], value)
+                if isinstance(value, str):
+                    value = self._parser(value, configParser)
+                # checking for dicts and lists here is needed because {} and []
+                # are valid yaml syntax so they get converted before hitting
+                # this method, so we must parse the elements.
+                #
+                # The same override would remain a string if specified on the
+                # command line, and is handled above.
+                if isinstance(value, dict):
+                    new = {}
+                    for k, v in value.items():
+                        if isinstance(v, str):
+                            new[k] = self._parser(v, configParser)
+                        else:
+                            new[k] = v
+                    value = new
+                elif isinstance(value, list):
+                    new = []
+                    for v in value:
+                        if isinstance(v, str):
+                            new.append(self._parser(v, configParser))
+                        else:
+                            new.append(v)
+                    value = new
+                # The field might be a string corresponding to a attribute
+                # hierarchy, attempt to split off the last field which
+                # will then be set.
+                parent, *child = field.rsplit(".", maxsplit=1)
+                if child:
+                    # This branch means there was a hierarchy, get the
+                    # field to set, and look up the sub config for which
+                    # it is to be set
+                    finalField = child[0]
+                    tmpConfig = attrgetter(parent)(config)
+                else:
+                    # There is no hierarchy, the config is the base config
+                    # and the field is exactly what was passed in
+                    finalField = parent
+                    tmpConfig = config
+                # set the specified config
+                setattr(tmpConfig, finalField, value)
+
             elif otype is OverrideTypes.Python:
-                exec(override, None, {"config": config})
+                # exec python string with the context of all vars known. This
+                # both lets people use a var they know about (maybe a bit
+                # dangerous, but not really more so than arbitrary python exec,
+                # and there are so many other places to worry about security
+                # before we start changing this) and any imports that are done
+                # in a python block will be put into this scope. This means
+                # other config setting branches can make use of these
+                # variables.
+                exec(override, None, vars)
             elif otype is OverrideTypes.Instrument:
                 instrument, name = override
                 instrument.applyConfigOverrides(name, config)

python/lsst/pipe/base/tests/simpleQGraph.py

@@ -194,8 +194,8 @@ def makeSimplePipeline(nQuanta, instrument=None):
     # dependencies)
     for lvl in range(nQuanta):
         pipeline.addTask(AddTask, f"task{lvl}")
-        pipeline.addConfigOverride(f"task{lvl}", "connections.in_tmpl", f"{lvl}")
-        pipeline.addConfigOverride(f"task{lvl}", "connections.out_tmpl", f"{lvl+1}")
+        pipeline.addConfigOverride(f"task{lvl}", "connections.in_tmpl", lvl)
+        pipeline.addConfigOverride(f"task{lvl}", "connections.out_tmpl", lvl+1)
     if instrument:
         pipeline.addInstrument(instrument)
     return pipeline

tests/test_configOverrides.py

@@ -28,6 +28,9 @@
 import lsst.pex.config as pexConfig
 from lsst.pipe.base.configOverrides import ConfigOverrides
 
+# This is used in testSettingVar unit test
+TEST_CHOICE_VALUE = 1  # noqa: F841
+
 
 class ConfigTest(pexConfig.Config):
     fStr = pexConfig.Field(dtype=str, default="default", doc="")
@@ -100,10 +103,6 @@ def testSimpleValueBool(self):
         with self.assertRaises(pexConfig.FieldValidationError):
             self.checkSingleFieldOverride(field, "1")
 
-        # non-parseable input
-        with self.assertRaises(RuntimeError):
-            self.checkSingleFieldOverride(field, "value")
-
     def testSimpleValueInt(self):
         """Test for applying value override to a int field
         """
@@ -127,10 +126,6 @@ def testSimpleValueInt(self):
         with self.assertRaises(pexConfig.FieldValidationError):
             self.checkSingleFieldOverride(field, "[]")
 
-        # non-parseable input
-        with self.assertRaises(RuntimeError):
-            self.checkSingleFieldOverride(field, "value")
-
     def testSimpleValueFloat(self):
         """Test for applying value override to a float field
         """
@@ -151,10 +146,6 @@ def testSimpleValueFloat(self):
         with self.assertRaises(pexConfig.FieldValidationError):
             self.checkSingleFieldOverride(field, "(1, 1)")
 
-        # non-parseable input
-        with self.assertRaises(RuntimeError):
-            self.checkSingleFieldOverride(field, "value")
-
     def testListValueStr(self):
         """Test for applying value override to a list field
         """
@@ -172,10 +163,6 @@ def testListValueStr(self):
         with self.assertRaises(pexConfig.FieldValidationError):
             self.checkSingleFieldOverride(field, "['a', []]")
 
-        # non-parseable input
-        with self.assertRaises(RuntimeError):
-            self.checkSingleFieldOverride(field, "value")
-
     def testListValueBool(self):
         """Test for applying value override to a list field
         """
@@ -188,10 +175,8 @@ def testListValueBool(self):
         # supported string conversions
         self.checkSingleFieldOverride(field, "[True, False]", [True, False])
         self.checkSingleFieldOverride(field, "(True, False)", [True, False])
+        self.checkSingleFieldOverride(field, "['True', 'False']", [True, False])
 
-        # parseable but invalid input
-        with self.assertRaises(pexConfig.FieldValidationError):
-            self.checkSingleFieldOverride(field, "['True', 'False']")
         with self.assertRaises(pexConfig.FieldValidationError):
             self.checkSingleFieldOverride(field, "[1, 2]")
         with self.assertRaises(pexConfig.FieldValidationError):
@@ -211,10 +196,9 @@ def testListValueInt(self):
         # supported string conversions
         self.checkSingleFieldOverride(field, "[1, 2]", [1, 2])
         self.checkSingleFieldOverride(field, "(1, 2)", [1, 2])
+        self.checkSingleFieldOverride(field, "['1', '2']", [1, 2])
 
         # parseable but invalid input
-        with self.assertRaises(pexConfig.FieldValidationError):
-            self.checkSingleFieldOverride(field, "['1', '2']")
         with self.assertRaises(pexConfig.FieldValidationError):
             self.checkSingleFieldOverride(field, "[1.0, []]")
         with self.assertRaises(pexConfig.FieldValidationError):
@@ -255,9 +239,22 @@ def testChoiceValueInt(self):
         with self.assertRaises(pexConfig.FieldValidationError):
             self.checkSingleFieldOverride(field, [0, 1])
 
-        # non-parseable input
-        with self.assertRaises(RuntimeError):
-            self.checkSingleFieldOverride(field, "value")
+    def testSettingVar(self):
+        """Test setting a field with a string that represents a variable name
+        """
+        field = "fChoiceInt"
+
+        # verify loading variable
+        self.checkSingleFieldOverride(field, "TEST_CHOICE_VALUE", 1)
+
+        # Verify That python importing a variable works
+        config = ConfigTest()
+        overrides = ConfigOverrides()
+        overrides.addPythonOverride("from math import pi")
+        overrides.addValueOverride("fFloat", "pi")
+        overrides.applyTo(config)
+        from math import pi
+        self.assertEqual(config.fFloat, pi)
 
     def testDictValueInt(self):
         """Test for applying value override to a dict field
@@ -278,10 +275,6 @@ def testDictValueInt(self):
         with self.assertRaises(pexConfig.FieldValidationError):
             self.checkSingleFieldOverride(field, {"a": "b"})
 
-        # non-parseable input
-        with self.assertRaises(RuntimeError):
-            self.checkSingleFieldOverride(field, "{1: value}")
-
 
 class MyMemoryTestCase(lsst.utils.tests.MemoryTestCase):
     pass