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

JointcalRunner.__call__ not receiving "butler" in kwargs

    XMLWordPrintable

    Details

    • Story Points:
      3
    • Sprint:
      AP S19-2, AP S19-3, AP S19-4
    • Team:
      Alert Production

      Description

      Dominique Boutigny reported on slack that he was getting failures in jointcal when running multiple tracts, because "butler" was not in the kwargs list. There haven't been any relevant changes to pipeBase.ButlerInitializedTaskRunner, so I'm not sure what could have happened.

      I'll try to come up with a test case for this, but to start with I should try to reproduce it on lsst-dev.

        Attachments

          Activity

          Hide
          Parejkoj John Parejko added a comment -

          Thinking about this some, I may have never called jointcal with more than one tract at a time, and I suspect Hsin-Fang Chiang does the same (specifying one tract plus its visit list per run). Which is why I hadn't encountered this before.

          That said, I suspect the problem is that JointcalRunner calls kwargs.pop('butler'), which removes "butler" from the kwargs dict. I'm not sure why I made that pop instead of just butler=kwargs['butler']: I probably cargo cult-ed some other file. I'm betting that the kwargs list gets reused in whatever calls jointcalRunner(), so the butler is lost for the second call.

          I've pushed what might be a trivial fix to this. Dominique Boutigny: does this ticket fix the issue for you? Given that JointcalRunner is going to be replaced in butler gen3, I'm not sure its worth spending time developing a unittest for this.

          Show
          Parejkoj John Parejko added a comment - Thinking about this some, I may have never called jointcal with more than one tract at a time, and I suspect Hsin-Fang Chiang does the same (specifying one tract plus its visit list per run). Which is why I hadn't encountered this before. That said, I suspect the problem is that JointcalRunner calls kwargs.pop('butler') , which removes "butler" from the kwargs dict. I'm not sure why I made that pop instead of just butler=kwargs ['butler'] : I probably cargo cult-ed some other file. I'm betting that the kwargs list gets reused in whatever calls jointcalRunner() , so the butler is lost for the second call. I've pushed what might be a trivial fix to this. Dominique Boutigny : does this ticket fix the issue for you? Given that JointcalRunner is going to be replaced in butler gen3, I'm not sure its worth spending time developing a unittest for this.
          Hide
          Parejkoj John Parejko added a comment -

          Dominique Boutigny: would you please be able to check to see if this branch fixes the issue? I just rebased it to master.

          Show
          Parejkoj John Parejko added a comment - Dominique Boutigny : would you please be able to check to see if this branch fixes the issue? I just rebased it to master.
          Hide
          Parejkoj John Parejko added a comment -

          Dominique reports that my attempted fix results in an immediately failure in JointcalTask:

          jointcal FATAL: Failed processing tract 4848, TypeError: runDataRef() got an unexpected keyword argument 'butler'

          I've started trying to implement some tests of JointcalRunner, but mocking enough of the butler is getting to be tricky. Given that there is a fix (explicitly specify one tract at a time when running jointcal), and the runner will be replaced in Gen3, I'm tempted to close this as won't fix and see if we can have a more comprehensive test suite in gen3.

          Show
          Parejkoj John Parejko added a comment - Dominique reports that my attempted fix results in an immediately failure in JointcalTask: jointcal FATAL: Failed processing tract 4848, TypeError: runDataRef() got an unexpected keyword argument 'butler' I've started trying to implement some tests of JointcalRunner , but mocking enough of the butler is getting to be tricky. Given that there is a fix (explicitly specify one tract at a time when running jointcal), and the runner will be replaced in Gen3, I'm tempted to close this as won't fix and see if we can have a more comprehensive test suite in gen3.
          Hide
          Parejkoj John Parejko added a comment -

          James Chiang: can you please test that this fixes the problem of running jointcal with multiple tracts specified at once?

          Tim Jenness: Can you please review this ~150 line change? This is the butler mock that I was asking about on slack. I will probably want a similar functionality test once we write the gen3 jointcal. If you have a better approach to such a test, please let me know.

          Nate Lust: see the PR for how I used unitest.mock to patch butler calls, etc. to force particular behavior. It wasn't pretty, but it does the job, and helped me figure out how to fix the listed issue.

          Show
          Parejkoj John Parejko added a comment - James Chiang : can you please test that this fixes the problem of running jointcal with multiple tracts specified at once? Tim Jenness : Can you please review this ~150 line change? This is the butler mock that I was asking about on slack. I will probably want a similar functionality test once we write the gen3 jointcal. If you have a better approach to such a test, please let me know. Nate Lust : see the PR for how I used unitest.mock to patch butler calls, etc. to force particular behavior. It wasn't pretty, but it does the job, and helped me figure out how to fix the listed issue.
          Hide
          jchiang James Chiang added a comment -

          John Parejko With these changes, I confirm that the code will run to completion if I provide just a list of visits and omit any tract specification in the `–id`.  All of the tracts covering the visits appear to have been processed.

          Show
          jchiang James Chiang added a comment - John Parejko With these changes, I confirm that the code will run to completion if I provide just a list of visits and omit any tract specification in the `–id`.  All of the tracts covering the visits appear to have been processed.
          Hide
          tjenness Tim Jenness added a comment -

          Looks okay to me but it needs much more clarity as to what is being tested. Mocking is uncommon in LSST test code and it's not clear at first glance what is going on and which code is being tested.

          Show
          tjenness Tim Jenness added a comment - Looks okay to me but it needs much more clarity as to what is being tested. Mocking is uncommon in LSST test code and it's not clear at first glance what is going on and which code is being tested.
          Hide
          Parejkoj John Parejko added a comment -

          Thank you for the review. I expanded the docstrings and comments to clarify what was being mocked, and what was being tested.

          Merged and done.

          Show
          Parejkoj John Parejko added a comment - Thank you for the review. I expanded the docstrings and comments to clarify what was being mocked, and what was being tested. Merged and done.

            People

            Assignee:
            Parejkoj John Parejko
            Reporter:
            Parejkoj John Parejko
            Reviewers:
            James Chiang, Tim Jenness
            Watchers:
            Dominique Boutigny, Hsin-Fang Chiang, James Chiang, Jim Bosch, John Parejko, John Swinbank, Tim Jenness
            Votes:
            0 Vote for this issue
            Watchers:
            7 Start watching this issue

              Dates

              Created:
              Updated:
              Resolved: