DM-15614: Allow dynamic hierarchy delimiters for configs #84

timj · 2018-08-31T19:27:04Z

The delimiter for hierarchies no longer defaults to a .. The delimiter is now a class variable and can be overridden by starting the string with the delimiter you wish to use.

ktlim

Only got through nameTuples() for now. Still need to look at names() and test_config.py.

ktlim · 2018-09-05T23:17:27Z

python/lsst/daf/butler/core/config.py

    """Implements a datatype that is used by `Butler` for configuration
    parameters.

    It is essentially a `dict` with key/value pairs, including nested dicts
    (as values). In fact, it can be initialized with a `dict`. The only caveat
-    is that keys may **not** contain dots (``.``). This is explained next:
+    is that keys may **not** contain the delimiter. This is explained next:


I think this is backwards. You intend to allow any keys whatsoever; it's the accessor who must ensure that the delimiter does not appear in any keys. Instead, this reads like an admonishment to the key creator not to use an unknown delimiter.

ktlim · 2018-09-05T23:19:01Z

python/lsst/daf/butler/core/config.py

+    string, such that ``foo["a", "b", "c"]`` is equivalent to
+    ``foo[".a.b.c"]``.  Finally, the delimiter can be escaped if it should
+    not be used in part of the string: ``foo[r".a.b\.c"]`` results in a two
+    element hierarchy of ``a`` and ``b.c``.  For hard-coded strings it is


Can you escape an initial non-alphanumeric character? Looking at the code, the answer is no. Maybe you should be able to, or else you should make clear that you can't.

I don't think I want to support escaping of the delimiter at the start of the key. We can revisit that if it becomes an issue (I'm not convinced people will want to be escaping delimiters at all).

ktlim · 2018-09-05T23:19:42Z

python/lsst/daf/butler/core/config.py

+    as the delimeter resulting in a single key of ``a.b.c`` being accessed.
+    Using ``foo[":a:b:c"]`` is therefore equivalent to ``foo[".a.b.c"]``.
+    This requires that keys in `Config` instances do not themselves start with
+    non-alphanumeric characters.  If the hierarchy is already available in a


You can access non-alphanumeric-starting keys by selecting a different delimiter: ":.a.b.c" should work, no?

ktlim · 2018-09-05T23:20:09Z

python/lsst/daf/butler/core/config.py

@@ -140,6 +141,9 @@ class Config(_ConfigBase):
        If `None` is provided an empty `Config` will be created.
    """

+    _D = "→"
+    """Default internal delimiter to use for components in the hierarchy"""


Should say when this default is used.

ktlim · 2018-09-05T23:28:55Z

python/lsst/daf/butler/core/config.py

+            else:
+                return [key, ]
+            # Allow escaping of the delimiter: .a.foo\.bar -> a foo.bar
+            escaped = f"\\{d}"


Maybe fr"\{d}"? Or that may be too fancy. What you have is fine.

ktlim · 2018-09-05T23:38:52Z

python/lsst/daf/butler/core/config.py

+            temp = None
+            if escaped in key:
+                # Replace with a character that won't be in the string
+                temp = "\r"


This is hacky. Perhaps you should just use re.split(fr'(?<=[^\\])\{d}', key)? (The lookbehind should be safe because there should always be at least one non-delimiter character, but you should probably check for that, too: you wouldn't want to allow a key of "::a:b\:c".)

Oh, but I just realized you need to replace the quoted delimiters with the actual delimiter afterwards, which my re.split doesn't do. Sorry about that.

ktlim · 2018-09-05T23:42:45Z

python/lsst/daf/butler/core/config.py

+        # Override the split for the simple case where there is an exact
+        # match.  This allows `Config.items()` to work since `UserDict`
+        # accesses Config.data directly to obtain the keys and every top
+        # level key should always retrieve the top level values.
        if name in data:
            keys = (name,)


Seems kind of wasteful to do another in and not just return the value here, but I guess you don't want to repeat the Config and _D code at the end.

Yes, it only goes once around the loop anyhow.

ktlim · 2018-09-05T23:48:56Z

python/lsst/daf/butler/core/config.py

        return data

    def __setitem__(self, name, value):
-        keys = name.split(".")
+        keys = self._splitKeys(name)
        last = keys.pop()
        if isinstance(value, Config):
            value = copy.deepcopy(value.data)


Below this, I'm a little worried about the setdefault(key, {}). What if there's supposed to be a sequence in there? You'd start getting "3" keys instead of 3 indexes.

config[".a.b.1"] = 5 works so long as /a/b is a sequence. Adding support for sequences is fairly new and has some rough edges. You can't say config[".a.b.10.c"] = 4 if /a/b/ is not a sequence or if /a/b is a sequence but only has 5 elements. If we started to want to support that there'd have to be some more deep thinking and we'd have to consider a ConfigSequence object I think. I'd like to defer going down that road until we need to.

ktlim · 2018-09-05T23:53:43Z

python/lsst/daf/butler/core/config.py

            if isinstance(d, collections.Sequence):
                theseKeys = range(len(d))
            else:
                theseKeys = d.keys()
            for key in theseKeys:
                val = d[key]
-                levelKey = f"{base}.{key}" if base is not None else key
+                levelKey = base + (key,) if base is not None else (key,)


These tuples may include integers, not just strings. Do you need to stringify the numbers above?

I believe there are tests for this and it's not an issue (names() does for stringification).

ktlim

Still have to check the tests.

ktlim · 2018-09-06T01:52:37Z

python/lsst/daf/butler/core/config.py

        return keys

+    def names(self, topLevelOnly=False, delimiter=None):
+        """Get the delimited name of all the keys in the hierarchy.


Maybe "a delimited name" instead of "the delimited name" since there isn't one canonical name.

ktlim · 2018-09-06T01:52:44Z

python/lsst/daf/butler/core/config.py

+            If True, only the top level are returned.
+        delimiter : `str`, optional
+            Delimiter to use when forming the keys.  The delimiter must
+            not be present in any of the keys.  If `None` given the delimiter


Again, "a delimiter", not "the delimiter".

ktlim · 2018-09-06T01:52:48Z

python/lsst/daf/butler/core/config.py

+
+        Returns
+        -------
+        names : `list`


list of str?

ktlim · 2018-09-06T01:52:54Z

python/lsst/daf/butler/core/config.py

+
+        if delimiter is not None:
+            if delimiter in combined:
+                # For better error message we need to report the clashing key


I was hoping the backslash-quoting would allow you to use the provided delimiter even in this case, by quoting it.

Ah. Yes. You are right. I could escape the delimiter in the keys in this case.

ktlim · 2018-09-06T01:52:59Z

python/lsst/daf/butler/core/config.py

+                    for n in names:
+                        for s in n:
+                            if searchStr in s:
+                                return s


Better to return s or n?

Good point. n is more explicit.

ktlim · 2018-09-06T01:53:03Z

python/lsst/daf/butler/core/config.py

+            delimiter = self._D
+            ntries = 0
+            while delimiter in combined:
+                log.debug(f"Trying delimiter '{delimiter}'")


The message here isn't quite accurate; delimiter was already tried and found wanting.

Good point. I had meant to remove that debug message anyhow.

ktlim · 2018-09-06T01:53:08Z

python/lsst/daf/butler/core/config.py

@@ -586,7 +713,7 @@ def __init__(self, other=None, validate=True, mergeDefaults=True, searchPaths=No
        # component.component (since the included files can themselves
        # include the component name)
        if self.component is not None:
-            doubled = "{0}.{0}".format(self.component)
+            doubled = (self.component, ) * 2


Less tricky to say (self.component, self.component) ?

The delimiter is now set as Config.D in a single location. Furthermore, the code to process the string supplied by the caller when addressing a config hierarchy is now in a single location. The logic has been expanded to allow an iterable such as a list as well as a string. Additionally, if the first character of the string is not an alphanumeric character then that character is assumed to be the delimiter for the given string.

Either using Config.D as a delimiter when building strings dynamically, or by using a leading "." to allow the lookup to be as close as possible to the previous form.

Starting with the delimiter used means that the string will work even in the unlikely event that the default delimiter is changed.

If the string being used to access the config entry is being constructed from a variable, it is more efficient and safer to pass in the components as a tuple() rather than having to worry about a delimiter.

It was being used in four places and that seemed like three too many places.

This is a clearer name and was requested in an earlier ticket review but was not implemented at the time.

We weren't making use of this and it added metaclass complication.

The delimiter is not allowed to clash with any of the keys. If no delimiter is specified the default internal delimiter will be used but if that is present in a key then a new delimiter will be used.

No longer use delimiters in the code since it is always safer to be explicit with a tuple at the expense of two more characters.

ktlim · 2018-09-06T15:23:47Z

tests/test_config.py

@@ -98,30 +99,53 @@ def testBadConfig(self):
            with self.assertRaises(RuntimeError):
                Config(badArg)

+    def testBasics(self):
+        c = Config({1: 2, 3: 4, "key3": 6})


These tests seem pretty minimal; can't you assert the actual strings or at least more of them or perhaps do a round-trip of the repr?

I'll do a round trip. I didn't really want to get bogged down in exact stringification form.

ktlim · 2018-09-06T17:24:02Z

tests/test_config.py

+        names = c.names()
+        nameTuples = c.nameTuples()
+        self.assertEqual(len(names), len(nameTuples))
+        self.assertGreater(len(names), 5)


Why assertGreater here? Don't you know what the length should be?

Fixed to use Equal.

ktlim · 2018-09-06T17:24:57Z

tests/test_config.py

+        self.assertGreater(len(names), 5)
+        self.assertGreater(len(nameTuples), 5)
+
+        with self.assertRaises(ValueError):


This would go away if we can do the quoted-delimiter thing.

Now the delimiter in a key is escaped.

timj requested review from TallJimbo and ktlim August 31, 2018 19:27

timj force-pushed the tickets/DM-15614 branch 4 times, most recently from 6659e57 to 190d07d Compare September 4, 2018 20:58

ktlim reviewed Sep 6, 2018

View reviewed changes

timj added 22 commits September 6, 2018 10:16

Use new syntax for addressing config hierarchy

e2aaa74

Either using Config.D as a delimiter when building strings dynamically, or by using a leading "." to allow the lookup to be as close as possible to the previous form.

Switch from . to an arrow as default delimiter

a3461be

Improve docs for Config

1afc154

Modify Config.names to include delimiter definition

dddc2ee

Starting with the delimiter used means that the string will work even in the unlikely event that the default delimiter is changed.

Improve config tests to include "." in keys

55b13c4

Use lists for config accessor strings

df1fa3a

If the string being used to access the config entry is being constructed from a variable, it is more efficient and safer to pass in the components as a tuple() rather than having to worry about a delimiter.

Use a module variable for an important configuration key

4684269

It was being used in four places and that seemed like three too many places.

Rename doDisassembly to shouldBeDisassembled

717ffc5

This is a clearer name and was requested in an earlier ticket review but was not implemented at the time.

Remove the YAML base class

f321165

We weren't making use of this and it added metaclass complication.

Fix typos in docstring and document tuple option

e285e5a

Tweak repr to return the class and add test for it

87c0102

str(Config) now returns pretty print, not repr

41fcce4

Allow delimiter to be escaped in Config hierarchy key

b376bc5

Do not use a default delimiter at all in Config

c087ab4

Add method to return all names in tuple form

e6ffa43

Allow names() to be given a delimiter, else use clever delimiter

b5fe4d1

The delimiter is not allowed to clash with any of the keys. If no delimiter is specified the default internal delimiter will be used but if that is present in a key then a new delimiter will be used.

Ensure that child configs inherit parent internal delimiter

895e1ca

Fix escaping in some strings

5fbf1bc

Use tuple access for all config strings with delimiters

79a471b

No longer use delimiters in the code since it is always safer to be explicit with a tuple at the expense of two more characters.

Remove extraneous parentheses for accessers

c422d60

Only mention \r once in escaping code

b05e83d

timj force-pushed the tickets/DM-15614 branch from 6a330a4 to b05e83d Compare September 6, 2018 17:19

ktlim reviewed Sep 6, 2018

View reviewed changes

timj added 11 commits September 6, 2018 11:14

Improve some documentation based on review

48b3fe8

Rename _splitKeys to _splitIntoKeys

6b06427

Return full key path in error message for delimiter clash

45470e0

Use explicit doubling in tuple rather than using *2 to expand

65d8406

Add more explicit Config tests

fe98cf4

Allow externally specified delimiter to always work in names()

102ce37

Now the delimiter in a key is escaped.

Enhance tests for key splitting

e617c2c

Ensure that supplied delimiter is not alphanumeric

6dd795a

Wrap line that is too long

9d74dd4

Remove E251 ignore for flake8

966a67e

Use tuple syntax for config access in gen2convert

1585885

timj merged commit 389b180 into master Sep 7, 2018

timj deleted the tickets/DM-15614 branch September 7, 2018 15:20

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

DM-15614: Allow dynamic hierarchy delimiters for configs #84

DM-15614: Allow dynamic hierarchy delimiters for configs #84

timj commented Aug 31, 2018

ktlim left a comment

ktlim Sep 5, 2018

ktlim Sep 5, 2018

timj Sep 6, 2018

ktlim Sep 5, 2018

ktlim Sep 5, 2018

ktlim Sep 5, 2018

ktlim Sep 5, 2018

ktlim Sep 6, 2018

ktlim Sep 5, 2018

timj Sep 6, 2018

ktlim Sep 5, 2018

timj Sep 6, 2018

ktlim Sep 5, 2018

timj Sep 6, 2018

ktlim left a comment

ktlim Sep 6, 2018

ktlim Sep 6, 2018

ktlim Sep 6, 2018

ktlim Sep 6, 2018

timj Sep 6, 2018

ktlim Sep 6, 2018

timj Sep 6, 2018

ktlim Sep 6, 2018

timj Sep 6, 2018

ktlim Sep 6, 2018

ktlim Sep 6, 2018

timj Sep 6, 2018

ktlim Sep 6, 2018

timj Sep 6, 2018

ktlim Sep 6, 2018

DM-15614: Allow dynamic hierarchy delimiters for configs #84

DM-15614: Allow dynamic hierarchy delimiters for configs #84

Conversation

timj commented Aug 31, 2018

ktlim left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ktlim left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment