DM-15189: core butler support for raw data ingest #59

TallJimbo · 2018-07-24T20:53:06Z

@timj, this is by no means done yet, but it occurred to me that we might be doing conflicting things with Datastores, so I wanted to get this out there in case you'd like me to spin it off to a new issue and merge sooner. I also need to write some tests for it, but since you've just refactored Datastore tests I definitely didn't want to do that before rebasing on that work.

TallJimbo · 2018-08-02T15:08:37Z

Most of the code originally here was moved to DM-15212, so some earlier comments may no longer make sense.

TallJimbo · 2018-08-22T14:57:59Z

The full ticket is finally ready for review. In this package (as well as obs_subaru), I recommend looking at each commit separately; this package in particular just contains a lot of miscellaneous improvements necessary for making higher-level code in obs_base and obs_subaru work.

timj

In general looks good. I have quite a few comments.

timj · 2018-08-22T15:03:11Z

config/storageClasses.yaml

  ImageF:
+    <<: *Image


Since Image only has one field and that field is being over-ridden, I'm not sure this line does anything (and the other similar changes). Are you worried that Image might get extra fields?

Yeah, it doesn't actually do anything; I just felt that using <<: to indicate inheritance even when it was a no-op was a useful bit of code-as-documentation.

It's not too late to add a parent: keyword and get proper pythonic inheritance for StorageClasses (I think I can sort out print through of those class properties).

Let's see what happens with DM-15238 first.

timj · 2018-08-22T15:04:56Z

config/datastores/posixDatastore.yaml

@@ -5,7 +5,7 @@ datastore:
    table: PosixDatastoreRecords
  create: true
  templates:
-    default: "{datasetType}/{tract:?}/{patch:?}/{filter:?}/{camera:?}_{visit:?}"
+    default: "{collection}/{datasetType}/{tract:?}/{patch:?}/{filter:?}/{camera:?}_{visit:?}"


General comment on defaulting filenames: I'm a bit worried that with this in the base definition it's not possible for people to remove defaulting in their own datastores. Maybe I should allow default: None to disable defaulting.

timj · 2018-08-22T15:07:13Z

config/storageClasses.yaml

+      apCorrMap: TablePersistableApCorr
+      coaddInputs: TablePersistableCoaddInputs
+      transmissionCurve: TablePersistableTransmissionCurve
+      metadata: PropertyList
  ExposureComposite: &ExposureComposite
    <<: *Exposure
    assembler: lsst.daf.butler.assemblers.exposureAssembler.ExposureAssembler
  ExposureCompositeF:


This change won't be needed once #76 is merged because ExposureCompositeF becomes a simple label that allows you to control concrete/virtual at the butler level.

timj · 2018-08-22T15:08:07Z

config/storageClasses.yaml

+      apCorrMap: TablePersistableApCorr
+      coaddInputs: TablePersistableCoaddInputs
+      transmissionCurve: TablePersistableTransmissionCurve
+      metadata: PropertyList


getMetadata is documented to return a PropertySet though?

Yeah, we discussed this on the other ticket. I'll convert to PropertySet here.

I actually left it alone in #76 since it worked regardless.

Ok, I think I'll stick with PropertyList here, too, then.

timj · 2018-08-22T15:09:41Z

python/lsst/daf/butler/butler.py

@@ -266,6 +266,9 @@ def getDirect(self, ref):
        ----------
        ref : `DatasetRef`
            Reference to an already stored dataset.
+        parameters : `dict`
+            Additional StorageClass-defined options to control reading,
+            typically used to efficiently only a subset of the dataset.


Missing word between "efficiently" and "only". Maybe I should work on parameters next (inMemoryDatastore will mess up with any tests where you use parameters).

timj · 2018-08-22T15:22:14Z

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

+            if field_name in ("run", "collection"):
+                usedRunOrCollection = True
+                if optional:
+                    raise KeyError("'run' and 'collection' may not be optional.")


Are you allowed to specify an optional run if collection is mandatory? (not that it's possible for run to be optional of course). Maybe the error here should be "can not be optional" ("may" sounds like you might be able to be optional). Do you have a test for optional collection below?

👍 on "can be optional". Will add a test.

Oh, actually, yeah, I suppose you could make one optional if the other is mandatory. Probably best to just remove this particular check, as there is another one to verify that at least one of those is present.

timj · 2018-08-22T17:46:05Z

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

+        tiny = self.readFull(fileDescriptor, parameters)
+        return fileDescriptor.storageClass.assembler().getComponent(tiny, component)
+
+    def readFull(self, fileDescriptor, parameters=None):


You are defining the parameters for get to be any parameters understood by the Exposure constructor?

Not at the moment - just bbox and origin. I suppose we need a place to document this at the StorageClass level. I'm open to ideas.

timj · 2018-08-22T17:50:08Z

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

+__all__ = ("FitsRawFormatterBase",)
+
+
+class FitsRawFormatterBase(FitsExposureFormatter, metaclass=ABCMeta):


All of the methods here need parameters and returns sections to the numpydocs.

timj · 2018-08-22T17:50:30Z

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

@@ -33,6 +33,53 @@ class FitsExposureFormatter(Formatter):

    parameters = frozenset(("bbox", "origin"))

+    def readImageComponent(self, fileDescriptor, component):


Please add parameters/returns sections to docstrings.

timj · 2018-08-22T17:53:44Z

python/lsst/daf/butler/instrument.py

+        A dictionary containing all fields in the Exposure table.
+    """
+    avg = visitInfo.getDate()
+    begin = DateTime(int(avg.nsecs(DateTime.TAI) - 0.5E9*visitInfo.getExposureTime()), DateTime.TAI)


You mean we can't get the value of DATE-OBS from the object but need to guess it by subtracting half the exposure time?

AFAIK, yes. I don't know much about VisitInfo. I stole this recipe from obs.base.makeRawVisitInfo.offsetDate, and I'd like to replace both that and VisitInfo itself, but this seemed like an easy way to get the correct (or at least consistent with Gen2) answer.

TallJimbo · 2018-08-24T01:36:43Z

I think I've addressed all comments (and rebased on latest master).

One question from that rebase (which I should have asked on a previous review, probably): do we still need the ExposureComposite StorageClasses?

timj · 2018-08-24T04:34:37Z

Re ExposureComposite: I think we should move it to the test configs so that the tests can try a virtual and concrete composite.

TallJimbo · 2018-08-24T12:41:45Z

I think we should move it to the test configs so that the tests can try a virtual and concrete composite.

I gave this a quick try on this ticket, but it's not easy because aliases apparently can't be used outside the file where they're defined (so you can't easily make ExposureComposite inherit attributes from Exposure when they're in different files).

timj · 2018-08-24T15:39:56Z

I was worrying about that cross-yaml file inheritance this morning. Would trivially be fixed if we had true python inheritance as well. Leave ExposureComposite in there for now -- I really do need to make a test for Exposure that uses virtual and one that uses concrete.

These are useful for raw data ingest, too, so move them out of the convert package. New home still probably isn't their permanent one.

This change didn't turn out to be necessary for this ticket, but it seemed worth preserving.

The duplication of all Exposure components in order to refine the types of some is unfortunate, but should go away after DM-15238.

In retrospect, this needs to be modified to produce an Exposure, not a DecoratedImage, and we can safely remove the write method.

This can be useful when you don't want an exception to start a rollback because you're about to catch it.

TallJimbo force-pushed the tickets/DM-15189 branch 2 times, most recently from 1e6c30e to d6b4931 Compare July 25, 2018 17:31

TallJimbo closed this Aug 2, 2018

TallJimbo force-pushed the tickets/DM-15189 branch from d6b4931 to 7628b5a Compare August 2, 2018 14:47

TallJimbo reopened this Aug 2, 2018

TallJimbo force-pushed the tickets/DM-15189 branch 5 times, most recently from 93dd568 to 82378d4 Compare August 11, 2018 14:51

TallJimbo force-pushed the tickets/DM-15189 branch 5 times, most recently from f2aae5e to 7aa301e Compare August 22, 2018 14:42

TallJimbo requested a review from timj August 22, 2018 14:56

timj approved these changes Aug 22, 2018

View reviewed changes

TallJimbo force-pushed the tickets/DM-15189 branch from 7aa301e to 255f119 Compare August 24, 2018 01:33

TallJimbo added 6 commits August 24, 2018 13:34

Add method to retrieve a DataUnit object from SqlRegistry.

00880f1

Refactor Visit/Exposure entry from VisitInfo routines.

25c3d13

These are useful for raw data ingest, too, so move them out of the convert package. New home still probably isn't their permanent one.

Make TopologicalSet actually behave like a Set.

adc5d8c

This change didn't turn out to be necessary for this ticket, but it seemed worth preserving.

Concrete StorageClass adjustments.

fa15115

The duplication of all Exposure components in order to refine the types of some is unfortunate, but should go away after DM-15238.

Refactor FitsExposureFormatter for readability, extensibility.

9f6bc33

Add formatter base class for raw data.

d849fb6

In retrospect, this needs to be modified to produce an Exposure, not a DecoratedImage, and we can safely remove the write method.

TallJimbo added 8 commits August 24, 2018 13:34

Add TransmissionCurve component to Exposure StorageClass

a9f6867

Make Dataset-not-found error friendlier.

0268882

Make certain ingested paths are relative to root.

d1de87e

Add run attribute to DatasetRef.

3710562

Add parameter support to Butler.get.

ef05c86

Make it possible to disable method-level transactionality.

f885c10

This can be useful when you don't want an exception to start a rollback because you're about to catch it.

Add run, collection keys to file templates.

99bc37c

Make addDataset optionally recursive.

c88c322

TallJimbo force-pushed the tickets/DM-15189 branch from 4404687 to c88c322 Compare August 24, 2018 17:34

TallJimbo merged commit a6f5c36 into master Aug 24, 2018

timj mentioned this pull request Aug 24, 2018

Enhance file template defaulting behavior #77

Merged

TallJimbo deleted the tickets/DM-15189 branch August 24, 2018 18:54

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

DM-15189: core butler support for raw data ingest #59

DM-15189: core butler support for raw data ingest #59

TallJimbo commented Jul 24, 2018

TallJimbo commented Aug 2, 2018

TallJimbo commented Aug 22, 2018

timj left a comment

timj Aug 22, 2018

TallJimbo Aug 23, 2018

timj Aug 23, 2018

TallJimbo Aug 24, 2018

timj Aug 22, 2018

timj Aug 22, 2018

timj Aug 22, 2018

TallJimbo Aug 23, 2018

timj Aug 23, 2018

TallJimbo Aug 24, 2018

timj Aug 22, 2018

timj Aug 22, 2018

TallJimbo Aug 23, 2018

TallJimbo Aug 23, 2018

timj Aug 22, 2018

TallJimbo Aug 24, 2018

timj Aug 22, 2018

timj Aug 22, 2018

timj Aug 22, 2018

TallJimbo Aug 24, 2018

TallJimbo commented Aug 24, 2018

timj commented Aug 24, 2018

TallJimbo commented Aug 24, 2018 •

edited

timj commented Aug 24, 2018

		__all__ = ("FitsRawFormatterBase",)


		class FitsRawFormatterBase(FitsExposureFormatter, metaclass=ABCMeta):

		@@ -33,6 +33,53 @@ class FitsExposureFormatter(Formatter):

		parameters = frozenset(("bbox", "origin"))

		def readImageComponent(self, fileDescriptor, component):

DM-15189: core butler support for raw data ingest #59

DM-15189: core butler support for raw data ingest #59

Conversation

TallJimbo commented Jul 24, 2018

TallJimbo commented Aug 2, 2018

TallJimbo commented Aug 22, 2018

timj 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

TallJimbo commented Aug 24, 2018

timj commented Aug 24, 2018

TallJimbo commented Aug 24, 2018 • edited

timj commented Aug 24, 2018

TallJimbo commented Aug 24, 2018 •

edited