DM-12627: Implement Butler DatasetType #20
Conversation
pschella
force-pushed
the
tickets/DM-12627
branch
from
d78ea79
to
c8d22ac
Compare
| return self._storageClass | ||
|
|
||
| def __init__(self, name, template, units, storageClass): | ||
| def __init__(self, name, dataUnits, storageClass, template=None): |
There was a problem hiding this comment.
Is the dataUnits argument here a frozenset and you copy it in the constructor?
There was a problem hiding this comment.
It may be any iterable, but it becomes a frozenset in the constructor.
| @@ -46,7 +45,7 @@ class DatasetType(object): | |||
| All arguments correspond directly to instance attributes. | |||
There was a problem hiding this comment.
Could really do with explicit Attributes and Parameters section.
There was a problem hiding this comment.
I thought it was policy to document these in the property docstrings?
There was a problem hiding this comment.
Yes. I thought that in this case there was a difference between init parameters and attributes (on phone so not checking).
| """A `StorageClass` that defines how this `DatasetType` is persisted. | ||
| """ | ||
| return self._storageClass | ||
|
|
||
| @property | ||
| def template(self): | ||
| """A string with `str`.format-style replacement patterns that can be |
There was a problem hiding this comment.
This is not related to the file name template used by datastore. Correct? (so this template is not a FileTemplate).
There was a problem hiding this comment.
It is related to that template. But it is unclear (to me at the moment) if we still want it here. I'll remove it for now.
| datasetTypeUnitsTable = self._schema.metadata.tables['DatasetTypeUnits'] | ||
| with self._engine.begin() as connection: | ||
| connection.execute(datasetTypeTable.insert().values(dataset_type_name=datasetType.name, | ||
| storage_class=datasetType.storageClass, |
There was a problem hiding this comment.
StorageClass doesn't have a __str__ method so what gets inserted into the table? Does it magically use StorageClass.name? Is it the full class name? StorageClasses are defined by the datastore so we have to work out how to verify that a registry is not specifying storage class names that a datastore doesn't know about.
There was a problem hiding this comment.
It is a name of a StorageClass. Not an instance.
| result = connection.execute(select([datasetTypeTable.c.storage_class, | ||
| datasetTypeTable.c.template]).where( | ||
| datasetTypeTable.c.dataset_type_name == name)).fetchone() | ||
| storageClass = result['storage_class'] |
There was a problem hiding this comment.
But DatasetType.storageClass is documented to be a StorageClass not a str.
There was a problem hiding this comment.
I'll change the docstring. Should be a name (for now at least).
tests/test_datasetType.py
Outdated
| class DatasetTypeTestCase(lsst.utils.tests.TestCase): | ||
| """Test for DatasetType. | ||
| """ | ||
| def setUp(self): |
There was a problem hiding this comment.
No need to define it if it doesn't do anything.
tests/test_datasetType.py
Outdated
| datasetTypeName = "test" | ||
| storageClass = "StructuredData" | ||
| dataUnits = frozenset(("camera", "visit")) | ||
| template = "{datasetType}/{camera}/{visit}" |
There was a problem hiding this comment.
I'm worried that we have two distinct template systems now.
There was a problem hiding this comment.
It should really be the same, but I'll remove it since this seems to be just a remnant of the old system.
tests/test_datasetType.py
Outdated
| DatasetType("a", "StorageA", ("UnitA", ), "{UnitB}")) | ||
|
|
||
| def testHashability(self): | ||
| types = [] |
There was a problem hiding this comment.
I think this test method needs a docstring to explain what is actually being tested. There's a lot going on here before the final conversion of the list to a set.
No description provided.