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

Fix masked exceptions in makeCoaddTempExp

    Details

    • Story Points:
      1
    • Team:
      Alert Production

      Description

      makeCoaddTempExp.run() has 4 cases of except Exception: continue. These need to at minimum respect doRaise, and should be converted to log.error instead of warn, as the warnings get lost.

      I'm not going to clean up the "expected" exceptions on this ticket, since I don't know what they are, but that's a necessary next step.

        Attachments

          Issue Links

            Activity

            Hide
            krzys Krzysztof Findeisen added a comment -

            Hi John Parejko, what do you mean by "respect doRaise"? doRaise controls the error-handling policy of the TaskRunner when run or runDataRef emits an exception; I don't think it's something lower-level code like run should refer to internally (though I'll admit this isn't documented anywhere).

            Show
            krzys Krzysztof Findeisen added a comment - Hi John Parejko , what do you mean by "respect doRaise "? doRaise controls the error-handling policy of the TaskRunner when run or runDataRef emits an exception; I don't think it's something lower-level code like run should refer to internally (though I'll admit this isn't documented anywhere).
            Hide
            Parejkoj John Parejko added a comment -

            Unfortunately, the docstring for doRaise does not distinguish between those two cases: "raise an exception on error (else log a message and continue)?" In this case (as in most of the offending pipe_tasks code), the masked exceptions are in runDataRef anyway.

            Show
            Parejkoj John Parejko added a comment - Unfortunately, the docstring for doRaise does not distinguish between those two cases: "raise an exception on error (else log a message and continue)?" In this case (as in most of the offending pipe_tasks code), the masked exceptions are in runDataRef anyway.
            Hide
            krzys Krzysztof Findeisen added a comment - - edited

            Yes, but if you look at the code, that's how it's organized (in fact, I'm not sure how a Task could refer to doRaise if it wanted to). Besides, it's better separation of concerns if the global error-handling strategy is controlled from one place.

            Show
            krzys Krzysztof Findeisen added a comment - - edited Yes, but if you look at the code, that's how it's organized (in fact, I'm not sure how a Task could refer to doRaise if it wanted to). Besides, it's better separation of concerns if the global error-handling strategy is controlled from one place.
            Hide
            Parejkoj John Parejko added a comment -

            Hsin-Fang Chiang: do you mind reviewing this? I've only tried to deal with two of the four cases, as the other two will likely require more careful refactoring and a better understanding of what exceptions can safely be ignored for those code blocks, and what should cause processing to stop.

            I've added Yusra AlSayyad as a reviewer as well, as we had discussed this some on Slack last week while talking about using jointcal for coadds. getCalibratedExposure() will only raise MissingExposureError if the calexp doesn't exist, or if the mosic output doesn't exist. Jointcal should produce output for every calexp in a visit that overlaps a tract, so a missing jointcal dataset when doApplyUberCal is True is a real error.

            While digging into that, I found that warpAndPsfMatch also masks all psfMatch exceptions as INFO messages...

            Show
            Parejkoj John Parejko added a comment - Hsin-Fang Chiang : do you mind reviewing this? I've only tried to deal with two of the four cases, as the other two will likely require more careful refactoring and a better understanding of what exceptions can safely be ignored for those code blocks, and what should cause processing to stop. I've added Yusra AlSayyad as a reviewer as well, as we had discussed this some on Slack last week while talking about using jointcal for coadds. getCalibratedExposure() will only raise MissingExposureError if the calexp doesn't exist, or if the mosic output doesn't exist. Jointcal should produce output for every calexp in a visit that overlaps a tract, so a missing jointcal dataset when doApplyUberCal is True is a real error. While digging into that, I found that warpAndPsfMatch also masks all psfMatch exceptions as INFO messages...
            Hide
            hchiang2 Hsin-Fang Chiang added a comment -

            LGTM, so removing my name from the reviewers.

            Show
            hchiang2 Hsin-Fang Chiang added a comment - LGTM, so removing my name from the reviewers.
            Hide
            Parejkoj John Parejko added a comment -

            Another example of why we want to not mask these exceptions: after the merge of DM-16598, I couldn't get my rebased DM-16650 branch of pipe_tasks to build without errors. This was because I hadn't rebased and rebuilt my afw to get the includeScaleUncertainty argument, but the errors reported in the unittests didn't give me any idea that was the problem, they were just a slew of Butler NoResults.

            Show
            Parejkoj John Parejko added a comment - Another example of why we want to not mask these exceptions: after the merge of DM-16598 , I couldn't get my rebased DM-16650 branch of pipe_tasks to build without errors. This was because I hadn't rebased and rebuilt my afw to get the includeScaleUncertainty argument, but the errors reported in the unittests didn't give me any idea that was the problem, they were just a slew of Butler NoResults .

              People

              • Assignee:
                Parejkoj John Parejko
                Reporter:
                Parejkoj John Parejko
                Reviewers:
                Yusra AlSayyad
                Watchers:
                Hsin-Fang Chiang, John Parejko, John Swinbank, Krzysztof Findeisen, Yusra AlSayyad
              • Votes:
                0 Vote for this issue
                Watchers:
                5 Start watching this issue

                Dates

                • Created:
                  Updated:

                  Summary Panel