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

Single-frame processing tasks are no longer usable without a Butler

    XMLWordPrintable

    Details

    • Type: Bug
    • Status: Done
    • Resolution: Done
    • Fix Version/s: None
    • Component/s: pipe_tasks
    • Labels:
      None
    • Story Points:
      3
    • Epic Link:
    • Sprint:
      DRP F16-1, DRP F16-2
    • Team:
      Data Release Production

      Description

      Adding a butler argument to the constructor signatures for CharacterizeImageTask, CalibrateTask, and ProcessCcdTask makes these tasks difficult to use without a butler.

      The fix is to make the butler argument optional (with a default of None), while adding another argument that allows a fully-constructed reference object loader to be provided directly instead.

      This is closely related to DM-6597, which has the opposite problem: pipe_drivers' SingleFrameDriverTask doesn't take a butler argument, but it needs to in order to provide one to ProcessCcdTask.

      I have a fix for this just about ready, but I'd like to add some unit tests that verify we can run all of these tasks both from the command-line and directly before calling it complete.

        Attachments

          Issue Links

            Activity

            Hide
            jbosch Jim Bosch added a comment -

            I have just moved some reviewers to watcher status (I'd just put them in the wrong place to start with). Sorry about the confusion.

            Show
            jbosch Jim Bosch added a comment - I have just moved some reviewers to watcher status (I'd just put them in the wrong place to start with). Sorry about the confusion.
            Hide
            jbosch Jim Bosch added a comment -

            This ticket makes it possible to run the immediate subtasks of ProcessCcdTask IsrTask, CharacterizeImageTask, and CalibrateTask) both directly from the command-line and from Python without access to the butler. The changes to the tasks themselves are small - mostly adding construcor arguments and making other arguments optional. Most of the new code is test code in pipe_tasks, but there are also changes to the camera mappers to provide config/metadata entries for these tasks (which are necessary to run them directly from the command line).

            Feel free to create any GitHub PRs that would be helpful in the review; I'm not going to create them all in advance because the changes in some packages are sufficiently trivial that a PR may not be necessary.

            Show
            jbosch Jim Bosch added a comment - This ticket makes it possible to run the immediate subtasks of ProcessCcdTask IsrTask, CharacterizeImageTask , and CalibrateTask ) both directly from the command-line and from Python without access to the butler. The changes to the tasks themselves are small - mostly adding construcor arguments and making other arguments optional. Most of the new code is test code in pipe_tasks, but there are also changes to the camera mappers to provide config/metadata entries for these tasks (which are necessary to run them directly from the command line). Feel free to create any GitHub PRs that would be helpful in the review; I'm not going to create them all in advance because the changes in some packages are sufficiently trivial that a PR may not be necessary.
            Hide
            hchiang2 Hsin-Fang Chiang added a comment -
            • A question about passing refObjLoader from SingleFrameDriver to ProcessCcdTask down to CharacterizeImageTask and CalibrateTask, noted on PR.
            • About the mapper policy paf files in obs packages:
              obs_cfht, obs_lsstSim, obs_sdss, obs_test: Isn't raw_skyTile no longer used?
              obs_decam: processCcd_config and processCcd_metadata are repeated
            • Currently the config/processCcd.py files in most obs packages include config for the three subtasks. Does it make sense to split the processCcd configuration into separate files for all obs packages? Or we leave that for each obs pacakge to do later?
            • Now IsrTask is a usable command line task (thank you!), does it become the only CmdLineTask outside pipe_tasks? If so, should we move it to pipe_tasks now, or leave it for the future package reorginization plan? Also, do we care to add executables for the three tasks?
            Show
            hchiang2 Hsin-Fang Chiang added a comment - A question about passing refObjLoader from SingleFrameDriver to ProcessCcdTask down to CharacterizeImageTask and CalibrateTask, noted on PR. About the mapper policy paf files in obs packages: obs_cfht, obs_lsstSim, obs_sdss, obs_test: Isn't raw_skyTile no longer used? obs_decam: processCcd_config and processCcd_metadata are repeated Currently the config/processCcd.py files in most obs packages include config for the three subtasks. Does it make sense to split the processCcd configuration into separate files for all obs packages? Or we leave that for each obs pacakge to do later? Now IsrTask is a usable command line task (thank you!), does it become the only CmdLineTask outside pipe_tasks ? If so, should we move it to pipe_tasks now, or leave it for the future package reorginization plan? Also, do we care to add executables for the three tasks?
            Hide
            rowen Russell Owen added a comment -

            Command-line tasks are encouraged to be in the package that makes most sense for them, so IsrTask belongs in ip_isr. pipe_tasks is intended for tasks (command-line and otherwise) that need so many packages that they don't make sense being in a lower-level package

            Show
            rowen Russell Owen added a comment - Command-line tasks are encouraged to be in the package that makes most sense for them, so IsrTask belongs in ip_isr. pipe_tasks is intended for tasks (command-line and otherwise) that need so many packages that they don't make sense being in a lower-level package
            Hide
            jbosch Jim Bosch added a comment -

            Hsin-Fang Chiang, all good questions.

            I'm afraid I don't know anything about raw_skyTile's status - I was just copying existing paf entries and modifying them, so I'm just inheriting an existing problem. If you can point me at an example of how I should remove any references to raw_skyTile, I'd be more than happy to fix that in the lines I've changed.

            I'll fix the duplications in obs_decam.

            I think we do want to split the processCcd configurations into separate files in the other obs packages, but I'd prefer not do do that on this ticket. I imagine that means I should create some new issues for that.

            I second Russell Owen's reply on CmdLineTask lcoations; FWIW, there are also a few in in meas_base and (I think) ip_diffim.

            Finally, I'm embarrassed I didn't notice the lake of executables for these Tasks. I'll certainly go ahead and add them. We may also want to encourage obs packages with their own ISR subclass to provide their own executables (I'll mention that in the issues I already promised to make). Eventually we'll need to solve the problem how how to run camera-retargeted tasks from the command-line in a generic way, but we'll need new task/config framework features to do that.

            Show
            jbosch Jim Bosch added a comment - Hsin-Fang Chiang , all good questions. I'm afraid I don't know anything about raw_skyTile 's status - I was just copying existing paf entries and modifying them, so I'm just inheriting an existing problem. If you can point me at an example of how I should remove any references to raw_skyTile , I'd be more than happy to fix that in the lines I've changed. I'll fix the duplications in obs_decam. I think we do want to split the processCcd configurations into separate files in the other obs packages, but I'd prefer not do do that on this ticket. I imagine that means I should create some new issues for that. I second Russell Owen 's reply on CmdLineTask lcoations; FWIW, there are also a few in in meas_base and (I think) ip_diffim. Finally, I'm embarrassed I didn't notice the lake of executables for these Tasks. I'll certainly go ahead and add them. We may also want to encourage obs packages with their own ISR subclass to provide their own executables (I'll mention that in the issues I already promised to make). Eventually we'll need to solve the problem how how to run camera-retargeted tasks from the command-line in a generic way, but we'll need new task/config framework features to do that.
            Hide
            rowen Russell Owen added a comment -

            I think that a bin script to handle ISR is going to take some thought, because every camera overrides that task. It may need a parent task whose sole purpose is to call the retargetable IsrTask. Another option is a set of bin scripts, one per camera. I suggest it be a different ticket.

            Show
            rowen Russell Owen added a comment - I think that a bin script to handle ISR is going to take some thought, because every camera overrides that task. It may need a parent task whose sole purpose is to call the retargetable IsrTask. Another option is a set of bin scripts, one per camera. I suggest it be a different ticket.
            Hide
            jbosch Jim Bosch added a comment - - edited

            Russell Owen good point on ISR bin scripts. I'll defer that, and I think that means I should remove the isr_config and isr_metadata paf entries I've added for most cameras as well, given that they'll only enable something that shouldn't be run anyway.

            UPDATED: Actually, I think I should just modify the isr_config entries to point to the right per-obs IsrTasks. I think that's an unambiguous step in the right direction, even if it doesn't enable anything on its own.

            Show
            jbosch Jim Bosch added a comment - - edited Russell Owen good point on ISR bin scripts. I'll defer that, and I think that means I should remove the isr_config and isr_metadata paf entries I've added for most cameras as well, given that they'll only enable something that shouldn't be run anyway. UPDATED: Actually, I think I should just modify the isr_config entries to point to the right per-obs IsrTasks. I think that's an unambiguous step in the right direction, even if it doesn't enable anything on its own.
            Hide
            hchiang2 Hsin-Fang Chiang added a comment -

            Agree, and thanks for pointing out meas_base. I think SuperTask work might want to know where all CmdLineTasks are.

            Show
            hchiang2 Hsin-Fang Chiang added a comment - Agree, and thanks for pointing out meas_base. I think SuperTask work might want to know where all CmdLineTasks are.
            Hide
            price Paul Price added a comment -

            Please add new common mapper entries in daf_butlerUtils rather than in every obs package.

            Show
            price Paul Price added a comment - Please add new common mapper entries in daf_butlerUtils rather than in every obs package.
            Hide
            jbosch Jim Bosch added a comment -

            Paul Price That sounds great, but do you have some docs or an example of how to do that?

            Show
            jbosch Jim Bosch added a comment - Paul Price That sounds great, but do you have some docs or an example of how to do that?
            Hide
            jbosch Jim Bosch added a comment -

            Found the docs here: https://community.lsst.org/t/centrally-defined-butler-datasets/841 (I was confused by the fact that only datasets.yaml currently exists).

            I've moved the _config entries (except isr_config) to daf_butlerUtils. The metadata entries and isr_config have to stay because they differ between obs packages.

            Just waiting on final confirmation from Jenkins now.

            Show
            jbosch Jim Bosch added a comment - Found the docs here: https://community.lsst.org/t/centrally-defined-butler-datasets/841 (I was confused by the fact that only datasets.yaml currently exists). I've moved the _config entries (except isr_config) to daf_butlerUtils. The metadata entries and isr_config have to stay because they differ between obs packages. Just waiting on final confirmation from Jenkins now.
            Hide
            jbosch Jim Bosch added a comment -

            Merged to master.

            Show
            jbosch Jim Bosch added a comment - Merged to master.

              People

              Assignee:
              jbosch Jim Bosch
              Reporter:
              jbosch Jim Bosch
              Reviewers:
              Hsin-Fang Chiang
              Watchers:
              Hsin-Fang Chiang, Jim Bosch, Paul Price, Robert Lupton, Russell Owen, Simon Krughoff
              Votes:
              0 Vote for this issue
              Watchers:
              6 Start watching this issue

                Dates

                Created:
                Updated:
                Resolved:

                  Jenkins

                  No builds found.