# Standardize primary method names, run/runDataRef, across PipeTasks

XMLWordPrintable

#### Details

• Type: Story
• Status: Done
• Resolution: Done
• Fix Version/s: None
• Component/s:
• Labels:
• Story Points:
12
• 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.

#### Activity

Hide
Paul Price added a comment -

That's great, Jim, thanks!

Show
Paul Price added a comment - That's great, Jim, thanks!
Hide
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.

Show
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.
Hide

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.

• run--> runDataRef lines 51, 53, 68, 86, 151, 176, 190, 202, 280

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.”

• 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:
lines: 32, 39, 55, 75, 131, 146, 174, 193

• 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_.

• 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.

Show
Yusra AlSayyad added a comment - 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.
Hide
Christopher Waters added a comment -

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.

Show
Christopher Waters added a comment - 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.
Hide

Closing. SPs include time becoming familiar with our workflow.

Show
Yusra AlSayyad added a comment - Closing. SPs include time becoming familiar with our workflow.

#### People

Assignee:
Christopher Waters
Reporter:
Reviewers:
Paul Price
Watchers:
Christopher Waters, Hsin-Fang Chiang, Jim Bosch, John Swinbank, Paul Price, Robert Lupton, Russell Owen, Simon Krughoff, Yusra AlSayyad