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

Fix masked exceptions in makeCoaddTempExp

    XMLWordPrintable

Details

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

            Hi Parejkoj, 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).

            krzys Krzysztof Findeisen added a comment - Hi Parejkoj , 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).

            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.

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

            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.
            Parejkoj John Parejko added a comment -

            hchiang2: 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 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...

            Parejkoj John Parejko added a comment - hchiang2 : 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 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...

            LGTM, so removing my name from the reviewers.

            hchiang2 Hsin-Fang Chiang added a comment - LGTM, so removing my name from the reviewers.
            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.

            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 .
            tjenness Tim Jenness added a comment -

            It looks like the PR associated with this ticket needs to be reworked since the code has moved and the gen2 parts have been removed.

            tjenness Tim Jenness added a comment - It looks like the PR associated with this ticket needs to be reworked since the code has moved and the gen2 parts have been removed.

            It needed to be reworked well before the gen2 replacement, too. MakeWarp still has two `except Exception as e` blocks, though they at least log now instead of just `continue`ing. I'm taking myself off of this ticket as the assignee; it didn't seem like the DRP group cared much about making these exceptions more specific.

            Parejkoj John Parejko added a comment - It needed to be reworked well before the gen2 replacement, too. MakeWarp still has two `except Exception as e` blocks, though they at least log now instead of just `continue`ing. I'm taking myself off of this ticket as the assignee; it didn't seem like the DRP group cared much about making these exceptions more specific.

            People

              Unassigned Unassigned
              Parejkoj John Parejko
              Yusra AlSayyad
              Hsin-Fang Chiang, John Parejko, John Swinbank, Krzysztof Findeisen, Tim Jenness, Yusra AlSayyad
              Votes:
              0 Vote for this issue
              Watchers:
              6 Start watching this issue

              Dates

                Created:
                Updated:

                Jenkins

                  No builds found.