Details
-
Type:
Story
-
Status: Done
-
Resolution: Done
-
Fix Version/s: None
-
Component/s: ip_isr, obs_cfht, obs_lsstSim, obs_sdss, obs_subaru, pipe_tasks
-
Labels:
-
Story Points:12
-
Epic Link:
-
Sprint:DRP F18-2
-
Team:Data Release Production
Description
DM-1299 changed the API of IsrTask so that run takes exposures and runDataRef takes a DataRef. This was implemented per the interest in RFC-26 to make run the method of reuse across our tasks, which seemed reasonable. The problem here is that CmdLineTask.parseAndRun calls "run," and we cannot trivially run IsrTask as a CmdLineTask.
If we want run to be the method of reuse and runDataRef to be the CmdLineTask:
- parseAndRun should be updated to call runDataRef
- Remaining run methods that take dataRefs should be renamed to runDataRef in all PipeTasks.
Attachments
Issue Links
- is duplicated by
-
- To Do
- is triggered by
-
- Done
- relates to
-
- Done
-
- Retired
-
DM-3756 Implement feature sets requested by SUI and DRP processing in Process Execution Framework
- Done
-
- Won't Fix
-
- Invalid
- mentioned in
-
Page Loading...
Activity
With the understanding that the goal here is not full RFC-352 compliance (which I think we've agreed needs some more thought and/or guidelines and/or work) but easing the SuperTask transition, this looks fine.
The commit messages are very short. In general, I shouldn't need to know what RFC-352 is (or even have access to Jira to look it up) in order to understand the motivation behind the changes. Please have a look at the commit message best practices in the Dev Guide. And I think you shouldn't pass this off as RFC-352 compliance in the commit summary line (which confused me) but easing the SuperTask transition (as I understand it, the fact that this helps RFC-352 is nice, but not the primary motivation).
I trust this has all been run through Jenkins and ci_hsc, and that there will be an announcement on CLO when this merges.
Paul Price
added a comment - With the understanding that the goal here is not full RFC-352 compliance (which I think we've agreed needs some more thought and/or guidelines and/or work) but easing the SuperTask transition, this looks fine.
The commit messages are very short. In general, I shouldn't need to know what RFC-352 is (or even have access to Jira to look it up) in order to understand the motivation behind the changes. Please have a look at the commit message best practices in the Dev Guide . And I think you shouldn't pass this off as RFC-352 compliance in the commit summary line (which confused me) but easing the SuperTask transition (as I understand it, the fact that this helps RFC-352 is nice, but not the primary motivation).
I trust this has all been run through Jenkins and ci_hsc, and that there will be an announcement on CLO when this merges. Made another review pass for completeness. Nice work. Looks like all that's left is documentation.
There are currently two copies of "How to write a commandline Task" and "How to write a Task" the dox version is in pipe_tasks and the rst version is in pipe_base. We're in transition. I would say you must update the rst and should update the dox.
I reviewed the dox first so I'll put those down first only because the comments are more complete.
pipe_tasks:
pipe_tasks/doc/old/writeCmdLineTask.dox:
- run--> runDataRef lines 51, 53, 68, 86, 151, 176, 190, 202, 280
- in: 198 \section pipeTasks_writeCmdLineTask_customTaskRunner Custom Task Runner
It’d be good to add the sentence to the intro of custom TaskRunners: “If your Task uses the pre-2018 naming convention and has a `run` method that operates on a `DataRef` instead of a `runDataRef`, you can still run this as a CmdLineTask by using the \ref lsst.pipe.base.cmdLineTask.LegacyTaskRunner "lsst.pipe.base.LegacyTaskRunner”, which will call your Task’s `run` method.”
pipe_tasks/doc/old/writeTask.dox:
- line 150 should now be be: “In addition, Command-line tasks require a `runDataRef` method that fetches the appropriate data products specified by Gen-2 Butler DataRefs”
- 164: The `run` and `runDataRef` methods should always…
- 174: `runDataRef` does everything
- 215: Including `run` and `runDataRef` (if present)
pipe_base:
pipe_base/doc/lsst.pipe.base/command-line-task-retargeting-howto.rst
lines: 32, 39, 55, 75, 131, 146, 174, 193
pipe_base/doc/lsst.pipe.base/command-line-task-retargeting-howto.rst
- lines: 32, 39, 55, 75, 131, 146, 174, 193
- add the info from the old docs about the LegacyTaskRunner here in the Custom Task Runner section that begins with 208. 244, 247
Nice doc string in TaskRunner.runTask, but check the veracity of the copied and pasted text from _call_.
pipe_base/python/lsst/pipe/base/task.py:
- docstring for Task. ``run`` —> ``runDataRef``
pipe_drivers:
Note to Self: Reminding myself that `CalibCombineTask` is OK as is since its not a cmdlineTask and will be refactored later. Tasks in obs_subaru liek HscFlatCombineTask inherit from this.
pipe_drivers/python/lsst/pipe/drivers/utils.py
- line 13 should prob say `runDataRef`
And finally squash the commits in all packages.
I've taken the duplicate reference to pipe_base/doc/lsst.pipe.base/command-line-task-retargeting-howto.rst as a typo, and checked all the files in that directory, modifying the two creating-*-task.rst files which had run references that needed updating to runDataRef.
Closing. SPs include time becoming familiar with our workflow.
That's great, Jim, thanks!