Uploaded image for project: 'Data Management'
  1. Data Management
  2. DM-2639

Standardize primary method names, run/runDataRef, across PipeTasks

    Details

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

        Attachments

          Issue Links

            Activity

            Hide
            price Paul Price added a comment -

            That's great, Jim, thanks!

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

            Show
            yusra 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
            czw 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
            czw 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
            yusra Yusra AlSayyad added a comment -

            Closing. SPs include time becoming familiar with our workflow.

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

              People

              • Assignee:
                czw Christopher Waters
                Reporter:
                yusra Yusra AlSayyad
                Reviewers:
                Paul Price
                Watchers:
                Christopher Waters, Hsin-Fang Chiang, Jim Bosch, John Swinbank, Paul Price, Robert Lupton, Russell Owen, Simon Krughoff, Yusra AlSayyad
              • Votes:
                0 Vote for this issue
                Watchers:
                9 Start watching this issue

                Dates

                • Created:
                  Updated:
                  Resolved:

                  Summary Panel