DM-14503: Implement YAML I/O for PropertySet/List. #94
Conversation
ktlim
changed the title
| """Represent an lsst.daf.base.DateTime (as ISO8601-formatted string in UTC) | ||
| """ | ||
| return dumper.represent_scalar(u'lsst.daf.base.DateTime', | ||
| data.toString(lsst.daf.base.DateTime.UTC)) |
There was a problem hiding this comment.
UTC time handling is broken, because the representation doesn't handle the string value when a leap second is added. I'm hoping we can ditch UTC support from this class. Please use TAI instead and/or rely on astropy to provide the string representation?
There was a problem hiding this comment.
Are you sure? I thought it handled it with T23:59:60 (and that there was a test for this). I'll check later. The problem with TAI is that it is not absolutely clear that it is in use (the only indicator is the lack of "Z"), and I was expecting the YAML to be human-readable.
There was a problem hiding this comment.
You were right that lsst.daf.base.DateTime doesn't handle leap seconds in UTC (because it defers to gmtime). This is fixable with a few lines of code for past and near-future times (since we know the leap second table). I'm not sure if it's worth doing.
There was a problem hiding this comment.
I think we should ditch UTC support from DateTime, but I suppose that's an RFC or RFD. In any case...please use astropy here if you want to save using UTC. But I'm keen on TAI; I think it's a much saner time standard and as long as we document what we're doing it'll be plenty human readable.
| for name in data.getOrderedNames(): | ||
| pairList.append([name, data.get(name), data.getComment(name)]) | ||
| return dumper.represent_sequence(u'lsst.daf.base.PropertyList', pairList, | ||
| flow_style=None) |
There was a problem hiding this comment.
You later modify this by adding type info. Could those commits be combined?
(Also...pairList for a list of 3-element tuples? That is a very misleading name. But with any luck you won't be keeping this commit)
|
|
||
| from . import (LogicalLocation, Persistence, Policy, StorageList, | ||
| StorageInterface, Storage, ButlerLocation, | ||
| NoRepositroyAtRoot, RepositoryCfg, doImport) | ||
| from lsst.log import Log | ||
| import lsst.pex.policy as pexPolicy | ||
| from .safeFileIo import SafeFilename, safeMakeDir | ||
| from . import baseYaml # noqa F401 |
There was a problem hiding this comment.
please explain in the code comment why you are importing this
| Parameters | ||
| ---------- | ||
| butlerLocation : ButlerLocation | ||
| The location & formatting for the object to be written. |
There was a problem hiding this comment.
Perhaps the same question again (depending if "persistence format" and "format" mean the same thing): is the format specified by butlerLocation? If so, please say more in the doc string. If not, please remove "& formatting"
| @@ -671,6 +673,25 @@ def writeParquetStorage(butlerLocation, obj): | |||
| obj.write(filename) | |||
|
|
|||
|
|
|||
| def writeYamlStorage(butlerLocation, obj): | |||
| """Writes an object to a location and persistence format specified by | |||
| ButlerLocation | |||
There was a problem hiding this comment.
In what way is the persistence format specified by butlerLocation? Isn't it always yaml?
There was a problem hiding this comment.
I was just copying the comments from other similar methods. I can try to go through and fix them all up instead.
| @@ -863,7 +884,13 @@ def readYamlStorage(butlerLocation): | |||
| for locationString in butlerLocation.getLocations(): | |||
| logLoc = LogicalLocation(butlerLocation.getStorage().locationWithRoot(locationString), | |||
| butlerLocation.getAdditionalData()) | |||
| finalItem = Policy(filePath=logLoc.locString()) | |||
| if not os.path.exists(logLoc.locString()): | |||
| raise RuntimeError("No such YAML file: " + logLoc.locString()) | |||
There was a problem hiding this comment.
I'm confused...why are you reading yaml data from a pickle (at least I assume it's a pickle based on the method name)? Have you changed the pickle format for PropertySet and PropertyList?
There was a problem hiding this comment.
You missed the blue bar saying that the function had changed, I think. I did add pickle-ability for PropertySet over in daf_base so that I could use the getstate() function in the YAML representer.
There was a problem hiding this comment.
I think I was not clear. Why is there an error message "No such YAML file" in a function named readPickleStorage? What does YAML have to do with pickle storage?
I don't understand your statement about a blue bar. I see is that readPickleStorage has changed dramatically and the changes seem to have nothing to do with pickle format. For instance below you now check for butlerLocation.pythonType == 'lsst.daf.persistence.RepositoryCfg' which doesn't seem like a pickle, and if that test fails, then read as a yaml file. I'm really confused.
There was a problem hiding this comment.
readPickleStorage (starting at line 695) has not changed at all. All these changes are in readYamlStorage (starting at line 870).
No description provided.