DM-16537: Rework exception handling and add tests. #237
Conversation
| @@ -319,7 +319,7 @@ def runDataRef(self, patchRef, selectDataList=[]): | |||
| if any(exps.values()): | |||
| dataRefList.append(tempExpRef) | |||
| else: | |||
| self.log.warn("Warp %s could not be created", tempExpRef.dataId) | |||
| self.log.error("Warp %s could not be created", tempExpRef.dataId) | |||
There was a problem hiding this comment.
FWIW, this warning has been seen pretty frequently in the HSC-RC2 reprocessing. One case is despite the ccd area overlaps with the sky patch, there are no good pixels in that patch (e.g. pixels masked out as bad). In a way it's not really an "error" because we could expect it and we chose not to write out a warp file in that case. In fact I'd argue for writing out a (~empty) file; that would make the execution workflow easier in the future middleware and production system.
There was a problem hiding this comment.
An old example with illustration plots can be found on this page (sorry -- rotate the ccd images by 180 deg in your brain and that's the lower-left vertical ccd in the sky)
There was a problem hiding this comment.
Thanks for the details. I'll make this back into a warn, but this suggests that we should do a better job managing this output, so that we either don't warn (if there's a good reason the warp wasn't made) or error/fail (if the warp should have been made but wasn't).
I've made a ticket for this: https://jira.lsstcorp.org/browse/DM-16591
There was a problem hiding this comment.
Thanks for filing the ticket!
We can continue processing if individual exposures are missing, but we should raise other exceptions up the chain.
parejkoj
force-pushed
the
tickets/DM-16537
branch
from
53f891b to
7492aa6
Compare
| except Exception as e: | ||
| self.log.warn("Calexp %s not found; skipping it: %s", calExpRef.dataId, e) | ||
| except MissingExposureError as e: | ||
| self.log.warn("Skipping missing data: %s", e) |
There was a problem hiding this comment.
Why no longer include dataId? I would imagine the dataId of the missing calexp useful for debugging, though such information may be obtained through other means.
There was a problem hiding this comment.
The assumption (and implementation) here is that the raised exception contains all the necessary information. Previously, the dataId was being duplicated.
There was a problem hiding this comment.
The only error from getCalibratedExposure that will include the dataId are failed butler calls. Anything else goes wrong and we won't have the dataId logged.
Note: I'm moving all the Gen2 dataRef's out of run in the next month anyway.
| ccdId = calExpRef.get("ccdExposureId", immediate=True) | ||
| except Exception: | ||
| ccdId = calExpInd | ||
| ccdId = calExpRef.get("ccdExposureId", immediate=True) |
There was a problem hiding this comment.
obs_comCam doesn't have a bypass_ccdExposureId, but that's not likely to survive. Your change is now requiring any new obs_package to write a bypass_ccdExposureId in order to be able to make a warp. I thought there MIGHT be some notebooks out there that make mock dataRefs.
The conversion to Gen 3 is going to move this to runDataRef anyway.
|
The ongoing conversation here is relevant to this ticket: https://community.lsst.org/t/warnings-or-other-not-errors-in-the-stack/3452/4 |
We can continue processing if individual exposures are missing, but we should raise other exceptions up the chain.