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

    • Bug
    • Status: Done
    • Resolution: Done
    • None
    • pipe_tasks
    • None
    • 3
    • DRP F16-1, DRP F16-2
    • 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

            No builds found.
            jbosch Jim Bosch created issue -
            jbosch Jim Bosch made changes -
            Field Original Value New Value
            Status To Do [ 10001 ] In Progress [ 3 ]
            jbosch Jim Bosch made changes -
            Story Points 1 3
            jbosch Jim Bosch made changes -
            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, so this will be out for review shortly.
            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.
            swinbank John Swinbank made changes -
            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.
            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.
            swinbank John Swinbank made changes -
            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.
            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.
            swinbank John Swinbank made changes -
            Epic Link DM-6172 [ 24685 ]
            swinbank John Swinbank made changes -
            Link This issue relates to DM-6597 [ DM-6597 ]
            swinbank John Swinbank made changes -
            Sprint DRP F16-1 [ 225 ]
            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.

            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.
            jbosch Jim Bosch made changes -
            Reviewers Robert Lupton, Russell Owen, Simon Krughoff [ rhl, rowen, krughoff ]
            Watchers Hsin-Fang Chiang, Jim Bosch [ Hsin-Fang Chiang, Jim Bosch ] Hsin-Fang Chiang, Jim Bosch, Robert Lupton, Russell Owen, Simon Krughoff [ Hsin-Fang Chiang, Jim Bosch, Robert Lupton, Russell Owen, Simon Krughoff ]
            jbosch Jim Bosch made changes -
            Link This issue is blocked by DM-6641 [ DM-6641 ]
            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.

            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.
            jbosch Jim Bosch made changes -
            Reviewers Hsin-Fang Chiang [ hchiang2 ]
            Status In Progress [ 3 ] In Review [ 10004 ]
            • 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?
            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?
            hchiang2 Hsin-Fang Chiang made changes -
            Status In Review [ 10004 ] Reviewed [ 10101 ]
            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

            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
            jbosch Jim Bosch added a comment -

            hchiang2, 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 rowen'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.

            jbosch Jim Bosch added a comment - hchiang2 , 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 rowen '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.
            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.

            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.
            jbosch Jim Bosch added a comment - - edited

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

            jbosch Jim Bosch added a comment - - edited rowen 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.

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

            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.
            price Paul Price added a comment -

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

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

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

            jbosch Jim Bosch added a comment - price That sounds great, but do you have some docs or an example of how to do that?
            swinbank John Swinbank made changes -
            Sprint DRP F16-1 [ 225 ] TCAMS 2016_03, DRP F16-1 [ 209, 225 ]
            swinbank John Swinbank made changes -
            Rank Ranked higher
            swinbank John Swinbank made changes -
            Sprint TCAMS 2016_03, DRP F16-1 [ 209, 225 ] DRP F16-1, DRP F16-2 [ 225, 231 ]
            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.

            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.
            jbosch Jim Bosch added a comment -

            Merged to master.

            jbosch Jim Bosch added a comment - Merged to master.
            jbosch Jim Bosch made changes -
            Resolution Done [ 10000 ]
            Status Reviewed [ 10101 ] Done [ 10002 ]
            swinbank John Swinbank made changes -
            Link This issue relates to DM-6877 [ DM-6877 ]
            hchiang2 Hsin-Fang Chiang made changes -
            Link This issue relates to DM-8000 [ DM-8000 ]

            People

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

              Dates

                Created:
                Updated:
                Resolved:

                Jenkins

                  No builds found.