DM-14191: Merge butler configuration files with defaults #39

timj · 2018-05-09T19:59:24Z

Also rewrite doImport

* schema is now a top level item in config file * repository yaml correctly finds registry and datastore defaults on a per-class basis.

Python docs really don't want you to use __import__ and strongly prefer importlib. I think this implementation is arguably easier to understand and has a key advantage of not hiding ImportError from the caller (this is important when loading plugins for formatters and the like since you want to know why your formatter failed to import).

So now must use butler config.

It was not used anywhere.

and also validate that some keys are present.

This simplifies the constructor code since it no longer requires that you know the subset and you know that the subclass has the right bit of the config. For example, allow a SchemaConfig to be created from a butler config file and locate the "schema" component.

pschella · 2018-05-09T22:50:58Z

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

+        ----------
+        config : `Config`, optional
+            By default checks itself, but if ``config`` is given, this
+            config will be checked instead.


Better to make validate a public class method instead?

See my comment below on validation of ConfigSubset but would you prefer class attributes Config.requiredKeys and a public method in Config.validate which used requiredKeys but could be overridden? Why a class method and not an instance method?

pschella · 2018-05-09T22:53:45Z

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

-    pass
+class DatastoreConfig(ConfigSubset):
+    component = "datastore"
+    requiredKeys = ("cls",)


Isn't this check now duplicated in Butler(Config)?

ButlerConfig doesn't inherit from ConfigSubset so no. I could move the validation code in ConfigSubset.__init__() to Config.__init__() and always have it as an option if you set requiredKeys.

That sounds good.

pschella · 2018-05-09T22:55:49Z

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

            else:
                raise ValueError("Incompatible Registry configuration: {}".format(config))
-        cls = doImport(config['cls'])
+        cls = doImport(config['registry.cls'])


If config now is a RegistryConfig doesn't it have just the registry subset?

Ah, I see, because we want the full Butler config for it. Maybe then explicitly specify that?

Right. I think it's better for RegistryConfig to only have registry stuff and SchemaConfig to only have schema stuff. I'll change that.

TallJimbo

Looks good; mostly very minor comments. I would like to make sure we figure out where to put the StorageClass->Formatter mapping defaults so those don't have to be repeated in each repository (and that raises the question of which config values, if any, should always go in the repo config instead of being pulled from the defaults).

TallJimbo · 2018-05-09T23:12:47Z

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

+        # Find the butler configs
+        self.defaultsDir = None
+        if "DAF_BUTLER_DIR" in os.environ:
+            self.defaultsDir = os.path.join(os.environ["DAF_BUTLER_DIR"], "config")


utils.getPackageDir?

I wondered about that. utils would work fine but I do wonder about adding LSST dependencies for no real gain. We only use utils in tests and I would argue that we don't need that dependency. That leaves only lsst.log and sphgeom stopping us from putting this package directly on pypi for everyone to use without needing any non-standard dependencies. (I'm thinking about world domination here...)

I have some of the same thoughts about world domination (see RFC-430 and RFC-460 vis a vis getting that sphgeom dependency out on pypi). But I think using DAF_BUTLER_DIR here just avoids an explicit dependency on utils via an implicit dependency on EUPS and perhaps sconsUtils, and I think the utils version may be slightly easier to rip out and replace with something unencumbered later.

TallJimbo · 2018-05-09T23:13:17Z

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

+
+    Parameters
+    ----------
+    other : `str`


Might want to rename this argument to something more descriptive.

I called it that because that's what it was called in Config (which came from daf_persistence). I'll have a think.

TallJimbo · 2018-05-09T23:14:29Z

config/datastores/posixDatastore.yaml

+  cls: lsst.daf.butler.datastores.posixDatastore.PosixDatastore
+  records:
+    table: PosixDatastoreRecords
+  create: true


I'm a bit surprised to not see the default mapping from StorageClass to Formatter here. Those are specific for PosixDatastore, right?

I don't actually care if they go here or elsewhere, but we need an easy way to construct a Butler that knows about those defaults.

I wasn't entirely sure what defaults would mean since the formatters really do lock in the file format, but you are right that FITS is where we are so our default for "how to write an Exposure" should be in this file.

A lot of those defaults currently live in the gen3 module of ci_hsc (on the gen3-middleware branch). I'd love to get those moved out of Python code into YAML. I'd also be happy to take care of making a ci_hsc branch compatible with this ticket once you're done with the daf_butler changes.

TallJimbo · 2018-05-09T23:15:40Z

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

+        # Look for class specific defaults
+        for section in ("datastore", "registry"):
+            k = f"{section}.cls"
+            print("Checking {}: {}".format(k, butlerConfig[k]))


Remove or use logging.

Yes. Sorry. Should have deleted it.

TallJimbo · 2018-05-09T23:19:52Z

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

+
+        # Default schema
+        schema = Config(os.path.join(self.defaultsDir, "registry", "default_schema.yaml"))
+        self.update(schema)


Should we be using RegistryConfig here? ("No" is a perfectly good answer; I'm just curious why not.)

Probably. Before I added ConfigSubset there wasn't any difference. Now there is so I'll revisit that.

Although, these configs have to fit in the global structure so should not have the schema stripped. The advantage of using SchemaConfig is you get the validation so I have to see if I can read the subset and bind it to the larger config hierarchy.

pschella · 2018-05-10T15:31:59Z

Since they are all defaults we may want to rename default_schema.yaml to something else.

pschella · 2018-05-10T15:34:09Z

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

+        # Find the butler configs
+        self.defaultsDir = None
+        if "DAF_BUTLER_DIR" in os.environ:
+            self.defaultsDir = os.path.join(os.environ["DAF_BUTLER_DIR"], "config")


I still wonder if having just one level of overrides is future proof enough. As opposed to a (colon-separated) $DAF_BUTLER_CONFIG_PATH.

I can do that. I thought we agreed that once we have the merging code in one place we can easily add it but since I'm there I'll add this other variable as well.

Do you envisage the $DAF_BUTLER_CONFIG_PATH containing a set of search paths that look identical to the on in daf_butler/config (so we look for storageClasses.yaml, etc)? And that the defaults should be filled in by reading butler configs first, then the path in reverse order?

I think we should hold off on this until we have a use case for it. I tend to think that having extra paths that aren't connected to either a repo or a software version could do more harm than good.

I implemented this last night... How about I push the change this morning as a new commit and you can take a look.

timj · 2018-05-10T21:58:03Z

Regarding default_schema.yaml I'm happy to delete the registry directory and call the file registry_schema.yaml.

* Each configuration subclass now knows its root component and name of defaults configuration file. * Search DAF_BUTLER_CONFIG_PATHS for files. * Registry.fromConfig can now take a separate schema config since Schema is in a different area of the model.

ktlim · 2018-05-14T23:26:59Z

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

+                # code.
+                # Should this be a log message? Are we using lsst.log?
+                # print("Checking path {} for {} ({})".format(pathDir, configClass, configFile))
+                if not os.path.isabs(configFile):


Doesn't this apply absolute paths over and over? Maybe that's not a problem.

Yes. It does. Hadn't thought of that. I don't think it's a problem as such, but it will slow things down.

Why not simply invert the loop order?

yes. since the configs read from the paths are subsets of the config I can loop over configs and then paths.

pschella · 2018-05-15T16:05:08Z

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

+    are constructed by reading first the global defaults, and then adding
+    in overrides from each entry in the colon-separated
+    ``$DAF_BUTLER_CONFIG_PATH`` in reverse order such that the entries ahead
+    in the list take priority. The registry and datastore configurations


Is this opposite behavior to shell $PATH? If so why?

It's not meant to be the opposite order. In $PATH things early in the path take precedence over things later in the path, so I thought I had to reverse the order when reading in the defaults so that earlier ones in the path override defaults appearing late in the path.

pschella · 2018-05-15T16:07:45Z

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

+        specializedConfigs = (DatastoreConfig, RegistryConfig)
+
+        # Look for class specific default files to be added to the file list
+        # These class definitions must come from the suppliedbutler config and


supplied butler.

pschella · 2018-05-15T16:08:44Z

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

+        # Look for class specific default files to be added to the file list
+        # These class definitions must come from the suppliedbutler config and
+        # are not defaulted.
+        for componentConfig in specializedConfigs:


Why not simply loop over all components and have those subclasses determine if they are special?

E.g. by having a component member.

Are you saying I should look through all the top level keys in the supplied butler config looking for .cls members, then ask the imported classes for their config files and associated config classes? (the latter they don't know but they could).

pschella · 2018-05-15T16:09:37Z

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

+        butlerConfig = Config(other)
+
+        # All the configs that can be associated with default files
+        configComponents = (SchemaConfig, StorageClassConfig, DatastoreConfig, RegistryConfig)


Perhaps make this a class member?

pschella · 2018-05-15T16:12:18Z

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

+                # code.
+                # Should this be a log message? Are we using lsst.log?
+                # print("Checking path {} for {} ({})".format(pathDir, configClass, configFile))
+                if not os.path.isabs(configFile):


Why not simply invert the loop order?

pschella · 2018-05-15T16:13:50Z

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

@@ -94,7 +94,7 @@ def __init__(self, other=None):
    def ppprint(self):
        """helper function for debugging, prints a config out in a readable way in the debugger.

-        use: pdb> print myConfigObject.pprint()
+        use: pdb> print myConfigObject.ppprint()


I didn't write the method...

pschella · 2018-05-15T16:15:14Z

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

+    """
+
+    def __init__(self, other=None):
+        super().__init__(other)


What if other is a ConfigSubset?

What am I missing?

>>> from lsst.daf.butler import ConfigSubset >>> x = ConfigSubset({"A": 5, "b": 6}) >>> x {'A': 5, 'b': 6} >>> y = ConfigSubset(x) >>> y {'A': 5, 'b': 6} >>> x["c"] = 3 >>> x {'A': 5, 'b': 6, 'c': 3} >>> y {'A': 5, 'b': 6}

pschella · 2018-05-15T16:20:22Z

tests/test_dataUnit.py

@@ -33,7 +34,8 @@ class DataUnitRegistryTestCase(lsst.utils.tests.TestCase):
    """
    def setUp(self):
        self.testDir = os.path.dirname(__file__)
-        self.schemaFile = os.path.join(self.testDir, "../config/registry/default_schema.yaml")
+        self.schemaFile = os.path.join(lsst.utils.getPackageDir("daf_butler"), "config",
+                                       SchemaConfig.defaultConfigFile)
        self.config = SchemaConfig(self.schemaFile)


Do we still need to specify the schemaFile? Or does calling SchemaConfig() load the default file?

At the moment, the only defaulting is in ButlerConfig. I can see how each subclass doing its own defaulting and getting the search path from a shared routine might be helpful (and more obvious).

pschella · 2018-05-15T16:21:07Z

tests/test_instrument.py

@@ -51,7 +52,7 @@ def setUp(self):
        self.configFile = os.path.join(self.testDir, "config/basic/butler.yaml")


Should we have a default file for Butler too perhaps?

If only for users to copy. But it might be useful to do butler = Butler() and simply get something working with a local SqliteRegistry and PosixDatastore in a default local subdirectory.

Perhaps even more useful if that then can write the default butler.yaml config in it (something like git init .) that you can subsequently modify.

But maybe the default constructor is not the best for that. And instead you want an explicit class method e.g. butler = Butler.makeDefault() (with an optional path).

I think I'd go with an explicit class method with a required path argument (which could accept ".", I suppose).

pschella · 2018-05-15T16:28:03Z

tests/test_posixDatastoreFits.py

        inputPosixDatastore = PosixDatastore(config=inputConfig, registry=self.registry)
        outputConfig = inputConfig.copy()
-        outputConfig['datastore.root'] = os.path.join(self.testDir, "./test_output_datastore")
+        outputConfig['root'] = os.path.join(self.testDir, "./test_output_datastore")


Are we now assuming that Datastore and Registry always have the same root? What if (for a particular datastore) there is no such thing?

I'm not sure I understand. This is a DatastoreConfig so by definition it's a datastore config not a registry config. Or are you asking a different question?

TallJimbo

Looks good. I've made some minor comments, but like @pschella, from this point I'm happy for you to merge when you think it's in good shape.
If you do want to merge before moving the StorageClass->Formatter lookup defaults to a non-test config file, I'll go ahead and add those on DM-14378 (there are a number of additional mappings I want to move to daf_butler from ci_hsc anyway, once I have a place to put them in daf_butler).

TallJimbo · 2018-05-15T17:49:44Z

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

@@ -94,7 +94,7 @@ def __init__(self, other=None):
    def ppprint(self):
        """helper function for debugging, prints a config out in a readable way in the debugger.

-        use: pdb> print myConfigObject.pprint()
+        use: pdb> print myConfigObject.ppprint()


I think pdb requires parentheses after print these days.

TallJimbo · 2018-05-15T17:50:14Z

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

+    the configuration supplied might contain more keys than you are interested
+    in and you only want one bit of it. For example, your config might contain
+    ``schema`` if it's part of a global config but it might be the contents
+    of ``schema``.


Missing the end of this sentence, I think.

TallJimbo · 2018-05-15T17:50:47Z

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

+
+    requiredKeys = ()
+    """Keys that are required to be specified in the configuration.
+    """


Are these relative to the top level or the component? (should be documented)

Relative to the Config root of this config (which does not include the component name).

TallJimbo · 2018-05-15T18:03:51Z

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

+                    # this checks a specific part of the tree
+                    # We may need to turn off validation since we do not
+                    # require that each defaults file found is fully
+                    # consistent.


I do think we'll need to defer validation until the full config has been loaded and merged.

TallJimbo · 2018-05-15T18:38:26Z

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

+    ModuleNotFoundError
+        No module in the supplied import string could be found.
+    ImportError
+        pythonType is found but can not be imported.


I think it might be best to catch and re-raise any of these as ImportError; it's much more likely that the caller will want to catch anything (without having a too-general except Exception block) than try to distinguish between them.

ModuleNotFoundError is a subclass of ImportError. I was pleased to discover that the simplification of doImport is only possible because we are using python 3.6 (previous versions did not have ModuleNotFoundError). I was trying to keep the interface looking like import. I suppose I could convert AttributeError to ImportError (although I thought I had managed to get import to throw that at one point, maybe via from).

Yeah, using a hierarchy of exceptions is the best scenario. If you can convert the AttributeError to ImportError, I do think that'd be best, even if that is a bit unlike import itself.

TallJimbo · 2018-05-15T18:40:21Z

python/lsst/daf/butler/registries/sqlRegistry.py

+    defaultConfigFile = None
+    """Path to configuration defaults. Relative to $DAF_BUTLER_DIR/config or
+    absolute path. Can be None if no defaults specified.
+    """


Do we not actually have a default for SqlRegistry configuration?

TallJimbo · 2018-05-15T19:26:18Z

Regarding default_schema.yaml I'm happy to delete the registry directory and call the file registry_schema.yaml.

Should we just call it schema.yaml, so overrides can use the same filename? (Overrides should be rare, but probably permitted in the same way StorareClass overrides are).

timj · 2018-05-15T19:27:52Z

Everything can be overridden using $DAF_BUTLER_CONFIG_PATH if they use the same file name as is in the butler config directory. I'm okay with schema.yaml.

This simplifies catch logic for doImport and most people do not need to understand AttributeError vs ImportError in this context.

Now each configuration determines its own defaults automatically. A ButlerConfig now simply asks each subconfig for a merge of the defaults with the externally supplied values.

Does not completely work yet because "run" can not be defaulted.

* Clearer statement on priority of search paths * Add explicit search path override to support tests * Merge defaults before overriding with external values

TallJimbo

Looks good; one question and one doc bug.

Also, could you rename registry_schema.yaml to just schema.yaml?

TallJimbo · 2018-05-17T18:01:42Z

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

+
+        # Select the part we need from it
+        if self.component is not None and self.component in externalConfig:
+            externalConfig.data = externalConfig.data[self.component]


Does this mean we merge the wrong externalConfig values when these conditions are not met, or does that mean we're assuming that externalConfig already corresponds to just the stuff under this component?

It means that if you give a config with a root of "datastore" and the config knows it's a datastore then your config will take the contents of the "datastore" part of the tree. If there is no "datastore" in the tree it assumes it's all meant for this config (and will then check for mandatory keys). ie you can give it a datastore.yaml that doesn't have the "datastore" part.

TallJimbo · 2018-05-17T18:02:38Z

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

+        """Search the supplied paths, merging the configuration values
+
+        The values read will override values currently stored in the object.
+        Every file found in the path will be read. The priority


Orphaned sentence.

timj added 10 commits May 9, 2018 12:58

Butler config can now merge with defaults

e929ce8

* schema is now a top level item in config file * repository yaml correctly finds registry and datastore defaults on a per-class basis.

Fix typo in docstring

611cf94

Provide config defaults

f365908

Move ButlerConfig to its own file

fee316f

Registry configuration needs schema and registry config entries

39dc7ef

So now must use butler config.

Remove StorageClassFactory.fromConfig

5b170a0

It was not used anywhere.

Add config subclass that can extract a sub component

d9b94b4

and also validate that some keys are present.

Use ConfigSubset

c88286f

This simplifies the constructor code since it no longer requires that you know the subset and you know that the subclass has the right bit of the config. For example, allow a SchemaConfig to be created from a butler config file and locate the "schema" component.

Simplify registry constructor

48ac511

timj force-pushed the tickets/DM-14191 branch from 991d8b0 to 48ac511 Compare May 9, 2018 21:27

pschella reviewed May 9, 2018

View reviewed changes

TallJimbo approved these changes May 9, 2018

View reviewed changes

pschella reviewed May 10, 2018

View reviewed changes

timj added 3 commits May 14, 2018 16:09

Rename registry defaults file

aa1472a

Update tests to use ButlerConfig and butler config directory

2612595

ktlim reviewed May 14, 2018

View reviewed changes

pschella reviewed May 15, 2018

View reviewed changes

TallJimbo approved these changes May 15, 2018

View reviewed changes

timj added 6 commits May 15, 2018 17:13

Prefer to throw ImportError rather than AttributeError

2fc0e3d

This simplifies catch logic for doImport and most people do not need to understand AttributeError vs ImportError in this context.

Rewrite config merging to use subset configs

3bcf273

Now each configuration determines its own defaults automatically. A ButlerConfig now simply asks each subconfig for a merge of the defaults with the externally supplied values.

Use defaulting to read schema

fdc02e9

Move some test configuration information to defaults

76b6020

Allow default butler config

86d9613

Does not completely work yet because "run" can not be defaulted.

Add parens around pdb example

76e603e

timj added 5 commits May 17, 2018 10:51

Add lots of tests for configuration merging

6f65bbc

Move merging discussion to Config class

f022d5b

Disable C++ check

4e4620d

If YAML file is empty create empty dict

e0fc9cf

Overhaul merging code to pass tests

0507933

* Clearer statement on priority of search paths * Add explicit search path override to support tests * Merge defaults before overriding with external values

TallJimbo approved these changes May 17, 2018

View reviewed changes

timj added 2 commits May 17, 2018 11:20

Rename registry_schema to schema config file

29cb101

Fix dangling sentence

bd82bd6

timj merged commit f0893da into master May 17, 2018

ktlim deleted the tickets/DM-14191 branch August 25, 2018 06:50

		@@ -51,7 +52,7 @@ def setUp(self):
		self.configFile = os.path.join(self.testDir, "config/basic/butler.yaml")

DM-14191: Merge butler configuration files with defaults #39

DM-14191: Merge butler configuration files with defaults #39

Conversation

timj commented May 9, 2018

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

TallJimbo 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

pschella commented May 10, 2018

pschella May 10, 2018 • edited

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

timj commented May 10, 2018

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

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

TallJimbo 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

TallJimbo commented May 15, 2018 • edited

timj commented May 15, 2018

TallJimbo 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

pschella May 10, 2018 •

edited

TallJimbo commented May 15, 2018 •

edited