Tickets/dm 7469 #27

n8pease · 2016-09-21T23:55:48Z

No description provided.

ktlim

Mostly looks good.

ktlim · 2016-09-22T00:24:36Z

python/lsst/daf/persistence/butler.py

-                repoData.repo.write(location, obj)
+                if isinstance(location, ButlerComposite):
+                    components = {}
+                    location.disassembler(obj=obj, dataId=location.dataId, componentDict=components)


Might be nice to prefill components with the names of the datasets expected and even to pass the disassembler the expected dataset types. It's also a little confusing to have a local components dict as well as location.components.

ktlim · 2016-09-22T00:26:03Z

python/lsst/daf/persistence/butler.py

+            if isinstance(location, ButlerComposite):
+                for name, info in location.components.items():
+                    location.components[name] = self.get(info.datasetType, location.dataId, immediate=True, 
+                                                         **rest)


**rest has already been added to the dataId, so it shouldn't need to be passed through here.

ktlim · 2016-09-22T00:27:47Z

python/lsst/daf/persistence/butlerLocation.py

+from . import iterify, doImport
+
+class ButlerComposite(object):
+    componentInfo = namedtuple('componentInfo', 'datasetType subset inputOnly')


This is a class, so it should be capitalized, I would think.

ktlim · 2016-09-22T00:29:31Z

python/lsst/daf/persistence/butlerLocation.py

+        :param dataId: the dataId that is used to look up components
+        :param mapper: a reference to the mapper that created this ButlerComposite object
+        """
+        self.assembler = doImport(assembler) if isinstance(assembler, basestring) else assembler


Do you need a from past.builtins import basestring to run under Python3?

ktlim · 2016-09-22T00:33:11Z

python/lsst/daf/persistence/repositoryCfg.py

@@ -27,7 +27,7 @@
 import inspect


Not using inspect anymore, I think.

ktlim · 2016-09-22T00:33:48Z

python/lsst/daf/persistence/repositoryCfg.py

-                if type(self._mapper) != type(repositoryArgs.mapper):
-                    return False
-        # check mapperArgs for any keys in common and if their value does not match then return false.
-        if self._mapperArgs is not None and repositoryArgs.mapperArgs is not None:


You don't still need to check the mapperArgs?

ktlim · 2016-09-22T00:36:21Z

python/lsst/daf/persistence/repositoryCfg.py

@@ -123,20 +123,16 @@ def makeFromArgs(repositoryArgs, parents):
    def matchesArgs(self, repositoryArgs):


ktlim · 2016-09-22T00:38:28Z

python/lsst/daf/persistence/butler.py

@@ -606,6 +606,14 @@ def get(self, datasetType, dataId=None, immediate=False, **rest):
            bypassFunc = getattr(location.mapper, "bypass_" + datasetType)
            callback = lambda: bypassFunc(datasetType, pythonType, location, dataId)
        else:
+            if isinstance(location, ButlerComposite):
+                for name, info in location.components.items():
+                    location.components[name] = self.get(info.datasetType, location.dataId, immediate=True, 


If this is a subset, this will have to be more complicated, no?

ktlim · 2016-09-22T00:40:08Z

python/lsst/daf/persistence/butlerLocation.py

+    componentInfo = namedtuple('componentInfo', 'datasetType subset inputOnly')
+
+    def __init__(self, assembler, disassembler, python, dataId, mapper):
+        """Initializer


Not required now, but you might think of starting to convert to Numpydoc (https://developer.lsst.io/docs/py_docs.html#documenting-python-apis)

TallJimbo · 2016-09-28T17:01:38Z

python/lsst/daf/persistence/__init__.py

+import pkgutil
+import lsstimport
+__path__ = pkgutil.extend_path(__path__, __name__)
+


Why is this needed? I'm concerned it may be fragile to have an __init__.py file that has both the above invocation and any real content (such as the imports below). It implies that there is another __init__.py with the same Python name (i.e. lsst.daf.persistence) in another location, and I believe only one of those is ever executed.

TallJimbo · 2016-09-28T17:12:14Z

python/lsst/daf/persistence/butlerLocation.py

+            The dataId that is used to look up components.
+        mapper : Mapper instance
+            A reference to the mapper that created this ButlerComposite object.
+        """


I think docstrings for __init__ functions don't actually work as you might expect in Python (they don't appear when you ask for on-line help on the type). It's standard to instead document the constructor and its parameters in the class docstring. More on that here.

TallJimbo · 2016-09-28T17:15:58Z

python/lsst/daf/persistence/test/__init__.py

+#
+
+"""Python interface to lsst::daf::persistence classes
+"""


Docstring is inaccurate (and perhaps not useful here).

TallJimbo · 2016-09-28T17:17:25Z

python/lsst/daf/persistence/test/testObject.py

+        self.data = data
+
+    def __eq__(self, other):
+        return self.data == other.data


Even if you don't use it, I'd recommend implementing __ne__ as well, just so avoid confusing behavior down the road (if you don't implement it, != will fall back to pointer equality instead of the inverse of your custom equality operator).

also becuase of auto import behavior of unit test packages, TestObject had been renamed to ObjectTest; I put it back to TestObject.

ktlim reviewed Sep 22, 2016

View reviewed changes

n8pease force-pushed the tickets/DM-7469 branch 4 times, most recently from 84db7e9 to 1aa0ec2 Compare September 27, 2016 21:16

TallJimbo reviewed Sep 28, 2016

View reviewed changes

n8pease force-pushed the tickets/DM-7469 branch 2 times, most recently from 73b038c to 148196b Compare September 28, 2016 21:15

n8pease added 8 commits September 28, 2016 15:33

simplify args/cfg match check

d836280

add butler.get support for composites

0f2a1c2

add butler.put support for composites

8fb7130

clarify composite get and set algorithm and names

929e49a

move TestObject to an importable location

3dbf3e5

also becuase of auto import behavior of unit test packages, TestObject had been renamed to ObjectTest; I put it back to TestObject.

add __ne__ to TestObject

dcf7a02

make repository use doImport helper

8572bba

extend doImport to allow member func

0bd26ef

n8pease force-pushed the tickets/DM-7469 branch from 148196b to 0bd26ef Compare September 28, 2016 22:33

n8pease added 2 commits October 10, 2016 14:19

Add docstring to RepositoryCfg.matchesArgs

146fbb4

add basestring import to repositoryCfg.py

9a47476

n8pease merged commit 9a47476 into master Oct 13, 2016

ktlim deleted the tickets/DM-7469 branch August 25, 2018 06:14

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Tickets/dm 7469 #27

Tickets/dm 7469 #27

n8pease commented Sep 21, 2016

ktlim left a comment

ktlim Sep 22, 2016 •

edited

ktlim Sep 22, 2016

ktlim Sep 22, 2016

ktlim Sep 22, 2016

ktlim Sep 22, 2016 •

edited

ktlim Sep 22, 2016

ktlim Sep 22, 2016

ktlim Sep 22, 2016

ktlim Sep 22, 2016

TallJimbo Sep 28, 2016

TallJimbo Sep 28, 2016

TallJimbo Sep 28, 2016

TallJimbo Sep 28, 2016

		@@ -123,20 +123,16 @@ def makeFromArgs(repositoryArgs, parents):
		def matchesArgs(self, repositoryArgs):

Tickets/dm 7469 #27

Tickets/dm 7469 #27

Conversation

n8pease commented Sep 21, 2016

ktlim left a comment

Choose a reason for hiding this comment

ktlim Sep 22, 2016 • 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

ktlim Sep 22, 2016 • 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

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 Sep 22, 2016 •

edited

ktlim Sep 22, 2016 •

edited