Details
-
Type:
Bug
-
Status: In Review
-
Resolution: Unresolved
-
Fix Version/s: None
-
Component/s: pipe_tasks
-
Labels:
-
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
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.
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.
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. 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...
LGTM, so removing my name from the reviewers.
Hsin-Fang Chiang
added a comment - LGTM, so removing my name from the reviewers. 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.
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.
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).