DM-13358: Initial composites implementation #12

timj · 2018-02-05T22:12:13Z

No description provided.

pschella · 2018-02-06T15:25:42Z

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

+        elif cls._pytypeName == "dict":
+            pytype = dict
+        elif cls._pytypeName == "list":
+            pytype = list


Why do we have these as special cases? If we really need that, can't we use builtins somehow?

We need a special case because the else clause tries to import them and you can't import dict. I hadn't thought about using dir(builtins) and seeing if the item exists there and checking it's a type and not a builtin_function_or_method.

pschella · 2018-02-06T15:26:10Z

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

    def __init__(self, name, bases, dct):
        super().__init__(name, bases, dct)
        if hasattr(self, "name"):
            StorageClass.subclasses[self.name] = self

+    @property
+    def pytype(cls):  # noqa N805


Docstring here?

Thanks. I had misunderstood how numpydoc worked with properties. I hadn't realized that I put a mention in the class docs but then the actual docs in the property.

and this is in the metaclass lazy loading so I wasn't sure how to document the metaclass at all.

pschella · 2018-02-06T15:31:44Z

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


        Raises
        ------
        e : `KeyError`
            If a storage class has already been registered with
            storageClassName.
        """
-        newtype = makeNewStorageClass(storageClassName, pytype, components)
+        newtype = makeNewStorageClass(storageClassName, **classAttr)


Given that makeNewStorageClass already creates the class, I think I'd rather call it outside registerStorageClass and pass the newly created class to it, rather than forwarding its constructor arguments.

So having: def registerStorageClass(storageClass) (or perhaps the type, instead of the instance).

Definitely the type, since the mappingFactory returns the instance.

pschella · 2018-02-06T15:38:51Z

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

+            # Fill in other items
+            scItems["components"] = components
+
+            self.storageClassFactory.registerStorageClass(name, scItems)


Why not simply pass **info? After swapping out the components. Perhaps renaming it storageClassKwargs or something.

It's a real shame we aren't using underscores rather than camel case 😄

pschella · 2018-02-06T15:43:51Z

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

+                refType = scComps[comp].pytype
+
+        if refType and not isinstance(result, refType):
+            raise TypeError("Got type {} from formatter but expected {}".format(type(result), refType))


This check is necessary, but probably also insufficient. It fails if the component happens to be of the same type as the parent. How do we know if it actually read the component, rather than the parent?
One possible solution would be to do an (md5) hash or something, but this cannot be done in the Datastore unless it keeps a registry.

Yes, if the parent has composites that are of the same class then this check will not be sufficient, but that is conceptually quite hard to spot.

pschella · 2018-02-06T16:01:46Z

tests/examplePythonTypes.py

+            requested.remove(c)
+            sc = storageClass.components[c]
+            if hasattr(self, c):
+                components[c] = (getattr(self, c), sc)


I wonder if we can't make this dissasembler more general? Seems like extracting components based on attributes is probably almost always what we want.

Yes. daf_persistence has a generic disassembler that checks for standard get methods as well as properties.

pschella · 2018-02-06T16:02:29Z

tests/test_posixDatastore.py

@@ -1,7 +1,7 @@
 #
 # LSST Data Management System
 #
-# Copyright 2008-2018  AURA/LSST.
+# Copyright 2018  AURA/LSST.


Is this the new default?

A new file written from scratch can't have a copyright year from before it existed.

pschella · 2018-02-06T16:03:26Z

tests/test_posixDatastore.py

+        self.assertEqualMetrics(metrics, metricsOut)
+
+        # Get a component even though it was written without
+        summary = datastore.get("{}#{}".format(uri, "summary"), storageClass=storageClass)


Here it is ok I think.

pschella · 2018-02-06T16:04:40Z

tests/test_posixDatastore.py

        with self.assertRaises(ValueError):
            # invalid storage class
-            datastore.get(uri="file:///non_existing.fits", storageClass=object, parameters=None)
+            datastore.get(uri="file:///non_existing.json", storageClass=object, parameters=None)


We should be careful that we don't start relying on file extensions anywhere (e.g. object stores don't have them).

I'm not looking at file extensions anywhere at the moment. That get will fail regardless.

I do now support file extensions inside formatters because some I/O libraries get upset if you don't have .fits (for example). The hint is treated as a hint and you get back a URI with the right extension.

pschella · 2018-02-06T16:05:57Z

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

@@ -28,11 +28,51 @@

 class StorageClassMeta(type):

+    # Caches of imported code objects.


Do we still actually need the metaclass and its registry of subclasses? If not, we should remove it, and move its methods to regular StorageClass class methods, to reduce complexity.

You need a metaclass to support lazy loading of class attributes. The subclasses stuff is not used at the moment but I have wondered if it might be useful (will we ever need to locate a storage class that hasn't been put in the storage class registry). Can remove it easily enough once the code settles down.

The python type is now materialized on demand rather than during class creation. As part of this change the code is now more generic to allow more simpler adding of additional class attributes to StorageClass.

Added assembler and disassebler implemntations for a simple test class. Put and get work.

Uses builtins to look up the string.

Rather than hand-crafting.

Rather than being test code, the assembler and disassembler can now attempt to run based on knowing the component names and checking the composite to see if it has associated getters or setters.

Mainly so that we can access components in formatters.

The formatter should know the type that it is expected to create when a component is being requested from a composite.

Remove the example version and create a generic one that can attempt to create objects of the correct type.

This allows components of a composite to be written with paths that include the same root and provides a predictable scheme for constructing file names for components.

This allows components to be removed individually.

All the file-based formatters are doing the same thing so refactor with base class that knows about files in general and subclasses that know the specifics of serializing to FITS/JSON/Pickle.

Also allow for afw to be missing.

pschella · 2018-02-13T20:12:42Z

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

+    """
+    requested = set(storageClass.components.keys())
+    expItems = requested & EXPOSURE_COMPONENTS
+    expInfoItems = requested & EXPOSURE_INFO_COMPONENTS


Should we do a sanity check for requested components that are not in the union of the supported sets?

pschella · 2018-02-13T20:21:32Z

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

+        cls = storageClass.pytype
+
+    # Should we check that the storage class components match or are a superset
+    # of the items described in the supplied components?


Yes, the former I would say.

Done. Has to be a superset though since you should be allowed to assemble an exposure from N of the M components if the pytype can support that.

pschella · 2018-02-13T20:22:41Z

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

+
+        if failed:
+            raise ValueError("Unhandled components during assembly ({})".format(failed))
+


Should we check that the decomposition returns the components again? Might be expensive to check though. So perhaps better done at the unittest level for each (dis)assembler.

I think asking the assembler to call the disassembler to compare components is going to be more than we need and we should have unittests for each assembler/disassembler that does that.

pschella · 2018-02-15T16:38:23Z

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

+
+    extension = None
+    """Default file extension to use for writing files. None means that no
+    modifications will be made to the supplied file extension."""


Document this in the class level attributes section?

That's what this is (meant) to do: https://developer.lsst.io/docs/py_docs.html?highlight=numpydoc#placement-of-constant-and-class-attribute-docstrings

pschella · 2018-02-15T16:42:26Z

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

+        return inMemoryDataset
+
+    def read(self, fileDescriptor):
+        """Read a `Exposure` from a FITS file.


Why is this specific to Exposure?

Because during the code refactoring I didn't notice that. Will fix.

pschella · 2018-02-16T19:46:52Z

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

+    """Metaclass used by `StorageClass`.
+
+    Implements lazy loading of class attributes, allowing datastores to
+    delay loading of external code until it is needed.


Still don't understand why this can't be handled in StorageClass itself and has to be in the metaclass.

It's a class attribute. You have to be able to lazy load the class type without instantiating a StorageClass instance. If you want it to be an instance attribute then I can switch it to being a standard @property lazy loader.

pschella · 2018-02-16T19:55:52Z

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

+    def disassembler(self):
+        """Function object to use to disassembler a type into components."""
+        return type(self).disassembler
+


I think it would be more elegant if e.g. SourceCatalog and the like were instances of StorageClass.
Then we won't need the metaclass. Additionally we could have assemble and disassemble be instance methods on a separate object that is passed to the StorageClass constructor (as discussed above). Right now the structure feels a bit forced with too many layers of inheritance.

They are instances of StorageClass (that's what Factory does). If you don't need StorageClass.pytype to work then this can be simplified.

pschella · 2018-02-16T20:03:21Z

tests/examplePythonTypes.py

+            Object contents in the form of a dict with keys corresponding
+            to object attributes.
+        """
+        return self.exportAsDict()


Why have both?

History. Although if this was production code and not a test class, it might be useful to have the public interface and the private one.

pschella · 2018-02-16T23:07:59Z

tests/test_posixDatastoreFits.py

+
+    def testConstructor(self):
+        datastore = PosixDatastore(config=self.configFile)
+        self.assertIsNotNone(datastore)


We should probably test also that the factory function returns the proper PosixDatastore instance.

This is your original test code so I haven't looked at it at all.

pschella · 2018-02-16T23:10:13Z

tests/test_posixDatastoreFits.py

+        # Simple check of WCS
+        bbox = lsst.afw.geom.Box2I(lsst.afw.geom.Point2I(0, 0),
+                                   lsst.afw.geom.Extent2I(9, 9))
+        self.assertWcsAlmostEqualOverBBox(wcs, exposure.getWcs(), bbox)


Test the actual component values?

This is testing the Wcs values. I don't necessarily know how to test every single Exposure component but I can default to seeing if == works.

None of the code is used because the tests currently use the standard formatters.

Significantly simplifies formatters because they can now rely on Assemblers doing the right thing for the given StorageClass. Since monolithic files vs separate files is no longer known to the YAML config file, for now use special Assembler classes with disabled disassembler methods to decide whether to write a composite as a single file.

This allows us to remove the metaclass

Most of the Assembler methods needed to know the storage class. Therefore StorageClass now returns an assembler with an instance of itself attached. This simplifies the interface.

pschella reviewed Feb 6, 2018

View reviewed changes

timj force-pushed the tickets/DM-13358 branch from eda4e70 to fb1446b Compare February 9, 2018 15:42

timj added 28 commits February 15, 2018 09:34

Remove unused code from test

cba14e6

Rename test file to indicate it involves FITS

ab5c9aa

Add example JSON formatter for testing

ac374ee

Switch StorageClass to lazy loading

f6c1d3b

The python type is now materialized on demand rather than during class creation. As part of this change the code is now more generic to allow more simpler adding of additional class attributes to StorageClass.

Proof of concept at composites

06ee0ac

Added assembler and disassebler implemntations for a simple test class. Put and get work.

Validate the returned type from a formatter

8ff8711

Improve error message from failed formatter read

05bde76

Add tests for components from non-composite write

287ab91

Add some missing docstrings

aa9a9fa

No longer create classes in StorageClassFactory

7be47d9

Support standard python types for StorageClasses

9f39472

Uses builtins to look up the string.

Form fragment URIs using urllib

067887b

Rather than hand-crafting.

Add method to validate type against storage class

8d3c211

Switch to generic assembler and disassembler

36269f5

Rather than being test code, the assembler and disassembler can now attempt to run based on knowing the component names and checking the composite to see if it has associated getters or setters.

Add storageClass to fileDescriptor

e60a29b

Mainly so that we can access components in formatters.

Remove object from class inheritance

8ce41b9

Allow genericAssembler to get override of python type

40e163a

Pass component type to formatter and set storageclass

6d43c6c

The formatter should know the type that it is expected to create when a component is being requested from a composite.

Create generic JSON formatter

246d6b1

Remove the example version and create a generic one that can attempt to create objects of the correct type.

Refactor to share some code between composite helpers

c0318df

Remove duplicate code that had moved earlier in routine

8418f5f

Use composite helper function

089d3e4

Add pickle formatter

2cf980c

Enable pickle and json formatters

28f24c3

Move composites logic shared by pickle and JSON to composites.py

cdde3de

Mark FITS and npy files as binaries

62065df

Fix logger name to remove "persistence"

2f09ab5

More robust handling of composites support code

29754b1

timj added 11 commits February 15, 2018 09:34

Support file paths involving fragments

d1d520b

This allows components of a composite to be written with paths that include the same root and provides a predictable scheme for constructing file names for components.

Use preferred path when trying to look for file to remove

74c0527

This allows components to be removed individually.

Add some disassembler composite support for Exposure

14d5aed

Refactor formatters to use shared code

741f78d

All the file-based formatters are doing the same thing so refactor with base class that knows about files in general and subclasses that know the specifics of serializing to FITS/JSON/Pickle.

Refactor afw FITS test to inherit from helper code

75138f6

Also allow for afw to be missing.

Add initial test for Exposure

b421586

npy files are binary

6b24329

Use YAML anchors to define Exposure and composite Exposure

74f6bee

Improve formatter fail error message

548ba03

Define proper Exposure component classes

571c1e9

Test all Exposure components

6ec10d6

timj force-pushed the tickets/DM-13358 branch from 0ffd68d to 6ec10d6 Compare February 15, 2018 16:34

pschella reviewed Feb 16, 2018

View reviewed changes

timj added 9 commits February 19, 2018 09:54

Add sanity check for Exposure components

8e3adfb

Raise if asking for component not known to StorageClass

8da84f2

Remove Exposure wording left over from refactoring

d2c8276

Fix typo

df94401

Remove example formatter file for now

079c41e

None of the code is used because the tests currently use the standard formatters.

Handle capitalization when there is a single letter component

f70a483

Check that supplied URI is a string

4cdd059

StorageClass lazy loading now done in instance

89e6cbb

This allows us to remove the metaclass

timj force-pushed the tickets/DM-13358 branch from 0995e37 to 89e6cbb Compare February 20, 2018 19:48

Update assembler interface to know its own storage class

75fb57f

Most of the Assembler methods needed to know the storage class. Therefore StorageClass now returns an assembler with an instance of itself attached. This simplifies the interface.

timj merged commit 75fb57f into master Feb 20, 2018

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

		@@ -28,11 +28,51 @@

		class StorageClassMeta(type):

		# Caches of imported code objects.


		if failed:
		raise ValueError("Unhandled components during assembly ({})".format(failed))

DM-13358: Initial composites implementation #12

DM-13358: Initial composites implementation #12

Conversation

timj commented Feb 5, 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

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

timj Feb 19, 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 Feb 19, 2018 •

edited