DM-13163: Refactor ap_pipe to use CmdLineTask primitives #22
Conversation
| @@ -47,7 +47,6 @@ | |||
| from lsst.pipe.tasks.ingestCalibs import IngestCalibsConfig, IngestCalibsTask | |||
| from lsst.pipe.tasks.ingestCalibs import IngestCalibsArgumentParser | |||
| import lsst.daf.base as dafBase | |||
| from lsst.ap.pipe import doIngestTemplates | |||
|
|
|||
| # Name of defects tarball residing in dataset's defects directory | |||
| _DEFECT_TARBALL = 'defects_2014-12-05.tar.gz' | |||
There was a problem hiding this comment.
Please label with an appropriate TODO for removing DECam-isms
| oldDir = os.path.abspath(os.path.join(inputTemplates, baseName)) | ||
| if os.path.isdir(oldDir): | ||
| os.symlink(oldDir, os.path.join(templateRepo, baseName)) | ||
| return None | ||
|
|
||
|
|
||
| def _doIngest(repo, refcats, dataFiles): |
There was a problem hiding this comment.
Please look for DECam and remove or add a TODO message, as appropriate. I see several instances such as the one in the doc string immediately following (github would not let me insert a comment after it).
I also found a note "At present, the code is specific to DECam; it will be generalized to other
instruments in the future." Please update that with the appropriate ticket.
kfindeisen
force-pushed
the
tickets/DM-13163
branch
from
953f110
to
09dde4f
Compare
This hack has been replaced with a multi-input Butler in ap_pipe. ap_verify still needs it in order to simulate operations repositories.
These will likely be removed in the future, depending on how much interaction is desired between ap_verify and the details of ap_pipe. Unfortunately, there is no clean way to parse a dataId string unless in a command-line task or calling one.
kfindeisen
force-pushed
the
tickets/DM-13163
branch
2 times, most recently
from
6dff9f2
to
a73112a
Compare
There was a problem hiding this comment.
Please consider making this a task (not a command-line task) so you get retargetable subtasks and the benefit of automatic metadata accretion and built-in logs. I think that would be a pretty minor change and it ought to simplify the code.
Other than that, a few minor requests including a few places where DECam-specific code needs a TODO note.
| workspace : `lsst.ap.verify.workspace.Workspace` | ||
| The abstract location containing input and output repositories. | ||
| dataId : `str` | ||
| dataId : `dict` of any |
There was a problem hiding this comment.
Does "of any" add useful information? To me it just adds frustration since it tells me nothing about what should be in the dict.
There was a problem hiding this comment.
Oops, that should be "dict from str to any". The key type is constrained.
For me, just saying dict adds frustration because that usually means there are hidden assumptions about the key/value types.
These are the minimal changes needed to make ap_verify work with ap_pipe again. Optimization of pipeline_driver will happen on a different ticket.
The new code imports metrics on exit or failure instead of after each pipeline step, allowing us to, for the moment, query the task object for metadata. This may need to be reworked depending on how parallel processing is handled. This commit makes one behavior change, in that it now tries to extract metrics from the failed pipeline step. If this process raises an exception, it will suppress any exception raised by the pipeline. This is an acceptable cost for the time being.
kfindeisen
force-pushed
the
tickets/DM-13163
branch
from
a73112a
to
f723aaa
Compare
This is a minimal set of changes needed for
ap_verifyto be able to callap_pipe's API introduced in lsst/ap_pipe#17. In particular, I did not attempt to change the basic architecture ofpipeline_driver, as the best design depends on the as-yet nebulous requirements forap_verify(in particular, the desired behavior in the event of pipeline failures).