DM-13349: Define storage classes in config yaml #11

timj · 2018-01-31T21:26:59Z

Moved the formatter factory registry to a new class MappingFactory.
Allow the factory registry to take classes as well as string definitions.
Replace StorageClass definitions in code with a new registry and ability to dynamically create them from strings.
Update posix data store to dynamically generate storage classes from the YAML.
Update tests to read storage classes from factory.

* Moved the formatter factory registry to a new class MappingFactory. * Allow the factory registry to take classes as well as string definitions. * Replace StorageClass definitions in code with a new registry and ability to dynamically create them from strings. * Update posix data store to dynamically generate storage classes from the YAML. * Update tests to read storage classes from factory.

timj · 2018-01-31T21:28:39Z

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

-                pass
-        typeName = self._registry[self._getName(storageClass)]
-        return self._getInstanceOf(typeName)
+        return self.getFromRegistry(storageClass, override=datasetType)


I can switch this to using composition if you prefer since the get and put methods do not have the same names as the base class implementation

Yes, that way we can also get rid of my other gripe (having refType specified) and move that to an argument (e.g. self.mappingFactory = MappingFactory(Formatter)).

Originally in python/lsst/daf/persistence/utils.py

timj · 2018-02-01T00:41:36Z

I've also relocated doImport from daf_persistence and added some tests to it.

We are using one of the functions and core daf_butler should not directly depend on daf_persistence. Transferring the whole file for now since we might use a function.

pschella · 2018-02-01T15:41:46Z

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

+    """
+
+    refType = None
+    """Type of instances expected to be returned from factory."""


Shouldn't this be documented in the attributes section of the class documentation instead?

I spent ten minutes reading numpydoc documentation and this seemed to be the way they wanted me to document class attributes. I admit it was somewhat unclear to me (and I'm about to delete that line anyhow).

Now that I read the numpydoc docs again I see the attributes section (as opposed to parameters for __init__).

pschella · 2018-02-01T16:05:34Z

tests/config/basic/butler.yaml

+        photoCalib: "TablePersistable"
+        visitInfo: "TablePersistable"
+        apCorr: "TablePersistable"
+        coaddInputs: "TablePersistable"


Are the quotes needed for the component storage classes? I'd like to specify them without quotes (or as YAML references), since they are internal references. Oh, and we better hope they don't become circular ;)

I can remove the quotes. Making them YAML references is something I'd have to look at (currently I need them to be strings to look up in the registry rather than items to look up from the YAML tree).

pschella · 2018-02-01T16:34:51Z

tests/test_formatter.py

+        storageClassName = "Image"
+        self.factory.registerFormatter(storageClassName, formatterName)
+        f = self.factory.getFormatter(storageClassName)
+        self.assertIsInstance(f, formatter.Formatter)


Also test the explicit subclass type?

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

tests/test_formatter.py

+
+        with self.assertRaises(ValueError):
+            # Try to store something that is not a Formatter but is a class
+            self.factory.registerFormatter("NotFormatter", "lsst.daf.butler.core.formatter.FormatterFactory")


Can be misleading if a value error is raised on the first argument instead of the second. Better reuse the same working identifier.

You mean the cases where "NotFormatter" is in the registry already? Shame I can't use KeyError for the "key already in use" error. What do you mean by "same working identifier"?

pschella · 2018-02-01T16:36:42Z

tests/test_formatter.py

+            self.factory.registerFormatter("NotFormatter", "lsst.daf.butler")
+
+        with self.assertRaises(ValueError):
+            # Try to store something that is not a Formatter but does not exist


and does not exist.

pschella · 2018-02-01T16:48:55Z

tests/test_storageClass.py

+        self.assertIsInstance(sc, storageClass.StorageClass)
+        self.assertEqual(sc.name, className)
+        self.assertIsNone(sc.components)
+        self.assertEqual(sc.type, dict)


ptype / pytype here too?

I did change all the type attributes to pytype at one point but then changed them back because I wasn't entirely sure that type was a bad name for the attribute (and keeping it meant changing less code). I'm fine with making it explicitly pytype so there is no confusion as to what the storage class is referring to.

pschella · 2018-02-01T16:54:42Z

tests/test_storageClass.py

+        self.assertEqual(sc.type, dict)
+
+        # Check that this class is listed in the subclasses
+        self.assertIn(className, newclass.subclasses)


Now that we moved the registry to an explicit factory, I don't think we need StorageClass's 'fancy' metaclass registration anymore (so subclasses and components dict can go I think).
It used to be responsible for mapping names to instances, but non-configurable and without overrides.

I might leave it in for a little just in case I have a problem where I need a storage class that has been made concrete somewhere else but is not in this registry.

pschella · 2018-02-01T16:58:09Z

tests/test_storageClass.py

+
+
+class PythonType:
+    """A dummy class to test the registory of Python types."""


registry?

pschella · 2018-02-01T17:07:59Z

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

-                pass
-        typeName = self._registry[self._getName(storageClass)]
-        return self._getInstanceOf(typeName)
+        return self.getFromRegistry(storageClass, override=datasetType)


Yes, that way we can also get rid of my other gripe (having refType specified) and move that to an argument (e.g. self.mappingFactory = MappingFactory(Formatter)).

pschella · 2018-02-01T17:16:46Z

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

+            Get item from registry associated with this target class, unless
+        override : `str` or object supporting `.name` attribute, optional
+            If given, look if an override has been specified for this and,
+            if so return that instead.


Perhaps make it slightly more generic? By taking *targetClasses and thus allowing for multiple overrides?

timj added 2 commits January 31, 2018 14:23

Ignore some more files

b1d7445

timj commented Jan 31, 2018

View reviewed changes

n8pease and others added 6 commits January 31, 2018 15:04

Move doImport code from daf_persistence

6806a84

Originally in python/lsst/daf/persistence/utils.py

Add some docs to doImport and fix string test

b54ce0c

Add some tests of doImport

8e6eef1

Use local doImport not one from daf_persistence

b26eee8

Add extra test for importing member function

8e4335b

Remove python2 comment and workaround from doImport

950c067

timj requested a review from pschella January 31, 2018 23:21

timj added 3 commits February 1, 2018 09:20

Copy safeFileIo.py from daf_persistence

dbf2764

We are using one of the functions and core daf_butler should not directly depend on daf_persistence. Transferring the whole file for now since we might use a function.

Use daf_butler safeMakeDir rather than daf_persistence version

1596bef

Add explicit dependencies for log, utils and daf_persistence

6560499

timj force-pushed the tickets/DM-13349 branch from 053674c to 6560499 Compare February 1, 2018 16:52

pschella reviewed Feb 1, 2018

View reviewed changes

timj added 6 commits February 1, 2018 10:41

Fixup: Switch to composition for factories

19a7d94

fixup: Remove extraneous quotes from yaml file

7c87172

fixup: implement review comments for tests

33ecb3b

Fixup: allow mapping factory to try as many classes as you like

faa83ba

fixup: typo

1270029

Use pytype as attribute for python type in StorageClass

68f27cc

timj merged commit 02c2778 into master Feb 1, 2018

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

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

DM-13349: Define storage classes in config yaml #11

DM-13349: Define storage classes in config yaml #11

timj commented Jan 31, 2018

timj Jan 31, 2018

pschella Feb 1, 2018

timj commented Feb 1, 2018

pschella Feb 1, 2018

timj Feb 1, 2018

timj Feb 1, 2018

pschella Feb 1, 2018

timj Feb 1, 2018

pschella Feb 1, 2018

pschella Feb 1, 2018

timj Feb 1, 2018

pschella Feb 1, 2018

pschella Feb 1, 2018

timj Feb 1, 2018

pschella Feb 1, 2018

timj Feb 1, 2018

pschella Feb 1, 2018

pschella Feb 1, 2018

pschella Feb 1, 2018



		class PythonType:
		"""A dummy class to test the registory of Python types."""

DM-13349: Define storage classes in config yaml #11

DM-13349: Define storage classes in config yaml #11

Conversation

timj commented Jan 31, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

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