DM-13814: Revamp how datastores and composites work #26

timj · 2018-03-28T23:46:31Z

Following the new scheme, DatasetRefs are now the means by which a datastore get/put work. Complete reorganization of how that happens and composite disassembly now happens at the butler level. All tests now pass. Currently uses a transient in-memory store in Datastore for keeping track of formatter that was used for a dataset.

Still need to add tests for butler put/get.

pschella

Looks ok, modulo comments. But I'd like a second look at it by Jim

pschella · 2018-03-29T21:18:33Z

python/lsst/daf/butler/assemblers/exposureAssembler.py

+            Assembled exposure.
+        """
+        micomps = {}
+        hasMI = False


Prefer longer names: maskedImageComponents. Annoying to type, but easier to read.

pschella · 2018-03-29T21:19:10Z

python/lsst/daf/butler/assemblers/exposureAssembler.py

+            if c in components:
+                hasMI = True
+                v = components[c]
+            micomps[c] = v


Here too: c -> component, v -> value.

pschella · 2018-03-29T21:19:58Z

python/lsst/daf/butler/assemblers/exposureAssembler.py

+            if wcs is not None:
+                exposure.setWcs(wcs)
+
+        # Set other components


I was wondering if something in obs_base already knows how to create an exposure from its components so I hadn't finished populating it. Maybe @TallJimbo knows.

No, nothing like this already exists. The afw APIs probably ought to be updated someday, so you can just pass kwargs with components to the Exposure constructor.

pschella · 2018-03-29T21:20:20Z

python/lsst/daf/butler/assemblers/exposureAssembler.py

+            exposure = makeExposure(maskedImage, wcs=wcs)
+
+            if not isinstance(exposure, pytype):
+                raise RuntimeError("Internal inconsistency in Exposure assembly")


Not a very descriptive error message.

pschella · 2018-03-29T21:22:29Z

python/lsst/daf/butler/butler.py

+        # and also has components
+        if storageClass.assemblerClass.disassemble is not None and storageClass.components:
+            components = storageClass.assembler().disassemble(obj)
+            if components:  # Probably need to always have all components


I would think so.

This gets back to the "if a component is defined to be None, does it really exist". In this case I think disassembly must always return something unless it's a completely blank Exposure.

pschella · 2018-03-29T21:36:12Z

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

+            if name == "config" or (isinstance(info, str) and info.endswith(".yaml")):
+                # This seems to be a location of another file so process that
+                self.addFromConfig(sconfig["config"])
+                continue


This looks like duplicate from Config.

I don't know. I wrote that from scratch. Does Config know that a storageClasses entry in a config can recursively load another config file if it sees the key config?

No, but it does have some check for a YAML file argument to its constructor that then builds it.

I don't want to create a storageClass called extraStorageClassesHere.yaml and I thought that you'd want the option of specifying multiple sources of extra definitions.

pschella · 2018-03-29T21:38:54Z

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

+    def __call__(cls, *args, **kwargs):  # noqa N805
+        if cls not in cls._instances:
+            cls._instances[cls] = super(Singleton, cls).__call__(*args, **kwargs)
+        return cls._instances[cls]


What does this do about multiple instances generated with different arguments? Now you confusingly get back an instance constructed with different arguments then you supplied.

you only ever get the first thing you constructed back. The **kwargs are there to allow this metaclass to be generally useful. I never call the StorageClassFactory constructor with arguments now because it no longer makes any sense because you don't know if you are the first one. I could remove the arguments from this definition if you want to force our uses of Singleton to only work if there are no arguments.

Yes, I think that would be safer.

pschella · 2018-03-29T21:45:16Z

python/lsst/daf/butler/datastores/posixDatastore.py

+            typeName = datasetType.name
+            template = self.templates.getTemplate(typeName)
+            location = self.locationFactory.fromPath(template.format(ref.dataId,
+                                                     datasetType=typeName) + "#predicted")


I assume we don't use #component anymore? So this won't clash?

Correct. I should remove that code from Location.

pschella · 2018-03-29T21:46:26Z

python/lsst/daf/butler/datastores/posixDatastore.py

+
+    @staticmethod
+    def calcChecksum(filename, algorithm="blake2b", block_size=8192):
+        """Calculate the checksum of the supplied file.


Is calc our convention for this?

I don't know. computeChecksum?

See rule 3-26. Abbreviations in names SHOULD be avoided in https://developer.lsst.io/cpp/style.html (which, given that it is about naming) I assume also applies to Python. compute is a specific example.

pschella · 2018-03-29T21:52:14Z

tests/test_butler.py

+
+        # Get a component
+        summary = butler.get("{}.{}".format(datasetTypeName, "summary"), dataId)
+        self.assertEqual(summary, metric.summary)


What about the other components?

ok. I'll use a for loop and getattr.

timj · 2018-03-30T01:18:19Z

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

+        return "DatasetType({}, {}, {})".format(self.name, self.storageClass.name, self.dataUnits)
+
+    @staticmethod
+    def nameWithComponent(datasetTypeName, componentName):


@pschella you might have an opinion on nameWithComponent, component and componentTypeName in DatasetType. They contain the logic for "calexp.wcs" naming convention so I wanted it to be in the one place in this class.

TallJimbo

Looks good. I had one structural comment about where disassemblers are associated with StorageClasses that I suspect is just a misunderstanding on my part, and a bunch of smaller things that I think are just minor cleanups.

I also think we're missing a piece of the lookup for components of deferred virtual composites in Registry (@pschella); there's an if statement in the confluence page pseudocode for that that doesn't seem to be implemented yet here.

TallJimbo · 2018-03-30T14:05:07Z

python/lsst/daf/butler/assemblers/exposureAssembler.py

+            if wcs is not None:
+                exposure.setWcs(wcs)
+
+        # Set other components


No, nothing like this already exists. The afw APIs probably ought to be updated someday, so you can just pass kwargs with components to the Exposure constructor.

TallJimbo · 2018-03-30T14:07:33Z

python/lsst/daf/butler/assemblers/exposureAssembler.py

+        if "apCorrMap" in components:
+            info.setApCorrMap(components.pop("calib"))
+        if "coaddInputs" in components:
+            info.setCoaddInputs(components.pop("coaddInputs"))


Not necessarily advocating this, but if you're interested, most of these ExposureInfo setters can take None (I think VisitInfo may be the only exception), so you could use a pattern like

info.setX(components.pop("x", None))

TallJimbo · 2018-03-30T14:18:03Z

python/lsst/daf/butler/butler.py

+        storageClass = datasetType.storageClass
+
+        # Check to see if this storage class has a disassembler
+        if storageClass.assemblerClass.disassemble is not None and storageClass.components:


Whether this disassemble method is None depends on the Butler config, right? How does that work if the storageClass instance is being retrieved from the Registry? (I'd have thought the assembler couldn't be attached to the storageClass for this reason, or if it was, it'd have to be attached by Butler itself, and then only if it knows it's the only Butler using that particular StorageClass instance).

That said, what we need from the config probably isn't the disassembler itself (since that should always be the same, which of course why it's nice to attach it to StorageClass intrinsically) - we just need a flag in the config that says whether we should use it, to use in this if statement.

(Or maybe I just don't understand how it works, of course).

storage classes are constructed via StorageClassFactory from config file. That's a singleton (as we discussed last week). The definition includes a definition of the assembler class. This is a name at class construction time. When you ask for an assembler instance from the storageclass it does a runtime import of the assembler (so you only need afw Exposure knowledge when you are actively wanting to deal with an Exposure). This is all hidden from registry and butler so registry is not involved other than asking the factory to convert the name into an instance when creating the DatasetType, and butler can just assume that the assembler will be there. You always have an assembler defined, but a disassemble method can be nulled out if you don't want to split up the in memory dataset into components.

Does that help?

That all makes sense if the disassemble method is either nulled out or not the original config file that defines the StorageClass, and can never change after that.

Let's say we're talking about a StorageClass that's define in the global configuration.

Can per-butler override configuration null the disassemble method?

StorageClassFactory is a singleton so you always retrieve the same StorageClass with a given name so if you have multiple butlers instantiated you do indeed end up with no way to enable or disable the disassembler per-butler. StorageClass doesn't have any instance data either so you can't override it by configuration. In the previous implementation this wasn't a problem because each butler got its own StorageClassFactory and the StorageClass definitions in the factory were allowed to be different even if they had the same fully qualified name. The switch to a singleton does require me to think some more about this.

Sounds good. I don't mind merging this as is; I don't think fixing it in the future will break anything structural.

TallJimbo · 2018-03-30T14:47:02Z

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

+    path : `str`
+        Path to Dataset, relative to `Datastore` root.
+    storageClass : `StorageClass`
+        `StorageClass` used when writing the file.


I think I understand how this works from our conversation yesterday, but this would be a good place to describe how/why/when it can differ from the StorageClass used to read the file.

TallJimbo · 2018-03-30T14:53:31Z

python/lsst/daf/butler/datastores/posixDatastore.py

+        try:
+            storedFileInfo = self.getStoredFileInfo(ref)
+        except KeyError:
+            return False


This currently returns False for virtual composites. I think that might be a problem; we definitely need a way to distinguish that state from "it used to exist, but has been deleted", which I don't think we have right now, but the solution doesn't have to be having this return True for virtual composites.

Good point. What does existence mean...? I was treating existence as "can I retrieve this thing" not "has it ever existed".

this is also a datastore.exists and not a butler.exists. If butler splits up an exposure and writes it as separate items datastore never sees the parent DatasetRef so can't understand that it does indeed have all the bits needed to make the composite. The only way this would really work is if butler could register the parent DatasetRef with the datastore after it's put all the components.

Keeping Datastore from knowing about these parents is probably a good idea. I think that just means we need a way for Registry to know that no Datastore entry should exist. That was going to be the assembler field in Dataset, and I think we can now just make that into a bool.

TallJimbo · 2018-03-30T15:05:23Z

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

@@ -106,13 +106,13 @@ class FileTemplate:
    def __init__(self, template):
        self.template = template

-    def format(self, dataUnits, datasetType=None, component=None):
+    def format(self, dataId, datasetType=None, component=None):


I think this may not need a component argument anymore, since that's packed into the datasetType.

In the future, it will probably need to have a run argument, as we'll need templates to be unique across runs as well.

Yes. Now that I've encoded the dot syntax into DatasetType I don't think a separate component is needed any more. I think I need to use the new DatasetType splitter here now.

Actually, I think it should just take a DatasetRef directly (it has all the information).

TallJimbo · 2018-03-30T15:11:37Z

python/lsst/daf/butler/datastores/posixDatastore.py

+        """
+
+        # if this has never been written then we have to guess
+        if ref.id is None:


This check only tells you whether some Datastore has written it, not whether this Datastore has, and I think that's what you want to be checking.

That said, I think users actually may want to specify (e.g. with a boolean arg) whether they're asking for a guess URI for something they haven't written yet or a URI for something that should exist, so they can get an exception when they don't get what they want.

When I wrote this I didn't have an exists method. I'll add an option to only retrieve things that actually exist.

TallJimbo · 2018-03-30T15:12:40Z

python/lsst/daf/butler/datastores/posixDatastore.py

            raise FileNotFoundError("No such file: {0}".format(location.uri))
-        os.remove(location.preferredPath())
+        os.remove(location.path)


Delete from StoredFileInfo, too?

TallJimbo · 2018-03-30T15:16:25Z

python/lsst/daf/butler/formatters/fileFormatter.py

+            # Concrete composite written as a single file (we hope)
+            try:
+                data = fileDescriptor.storageClass.assembler().getComponent(data, comp)
+            except AttributeError:


I'm surprised the expected exception is an AttributeError; is that a detail of the default implementation?

https://github.com/lsst/daf_butler/blob/master/python/lsst/daf/butler/core/composites.py#L205

You prefer KeyError? I throw AttributeError because I'm testing with hasattr in there.

I don't have a strong opinion; I just wanted to make sure this was being considered as part of the CompositeAssembler public API rather than being an accident (and I see now that the exception type is documented there as the one derived classes should raise, so I'm happy).

* Datastore APIs now take DatasetRefs * Assembly and disassembly is now done by butler. * Datastore now records path and formatter indexed by DatasetRef. - No longer uses a URI to record that information. * URIs can now be retrieved separately. * StorageClassFactory is now a Singleton. * DatasetType now has a StorageClass instance (not name) * StorageClasses can now be defined in multiple config files. * afw Exposure assembly much improved. * File templates now use DatasetRef * DatasetType now knows how to to represent components in the name. * Read Formatters now have access to the storage class used to write and the storage class requested to read the data. * Location no longer knows about components.

timj force-pushed the tickets/DM-13814 branch from 64204f2 to f92de54 Compare March 29, 2018 02:59

timj added 2 commits March 29, 2018 10:03

Add link to component on Jira

097a475

Fix some docstring problems

06b392f

timj force-pushed the tickets/DM-13814 branch 2 times, most recently from 3e1b954 to 1a5326c Compare March 29, 2018 19:47

pschella reviewed Mar 29, 2018

View reviewed changes

timj commented Mar 30, 2018

View reviewed changes

TallJimbo approved these changes Mar 30, 2018

View reviewed changes

timj force-pushed the tickets/DM-13814 branch from d4aadd7 to aed7723 Compare March 30, 2018 19:30

timj merged commit 551b585 into master Mar 30, 2018

ktlim deleted the tickets/DM-13814 branch August 25, 2018 06:49

DM-13814: Revamp how datastores and composites work #26

DM-13814: Revamp how datastores and composites work #26

Conversation

timj commented Mar 28, 2018

pschella 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

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

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