XMLWordPrintable

#### Details

• Type: Bug
• Status: In Review
• Resolution: Unresolved
• Fix Version/s: None
• Component/s:
• Labels:
• Story Points:
1
• Team:

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

#### Activity

Hide
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
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
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
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
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
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
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
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
Hsin-Fang Chiang added a comment -

LGTM, so removing my name from the reviewers.

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

Show
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.
Hide
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 continueing. 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.

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

Assignee:
Unassigned
Reporter:
John Parejko
Reviewers:
Watchers:
Hsin-Fang Chiang, John Parejko, John Swinbank, Krzysztof Findeisen, Tim Jenness, Yusra AlSayyad