DM-39842: Configurable exception handling in CalibrateImageTask #835
Conversation
parejkoj
force-pushed
the
tickets/DM-39842
branch
from
63dacc7
to
93c41a8
Compare
parejkoj
force-pushed
the
tickets/DM-39842
branch
from
93c41a8
to
893d52a
Compare
parejkoj
force-pushed
the
tickets/DM-39842
branch
2 times, most recently
from
89cb8e8
to
49817ee
Compare
| applied_photo_calib=None, | ||
| astrometry_matches=None, | ||
| photometry_matches=None, | ||
| ) |
There was a problem hiding this comment.
I'm torn as to whether it's better to spell out the possible fields here with None values, vs. letting run be the sole source of truth on those. I think the QuantumContext changes in pipe_base should allow either to work.
There was a problem hiding this comment.
To simplify the call to annotate below (so it's not a mess of getattr), I need to at least specify the items that are passed to annotate. I don't like specifying a result list in both places (runQuantum and the top of run); I've changed it so run just makes an empty Struct (this caught a bug!) which is a bit better.
There was a problem hiding this comment.
This feels like it could be fragile if we ever change this task to output more things in the result Struct. Is it not possible to construct a list of arguments to feed the error annotation by iterating over all the existing things in the Struct?
There was a problem hiding this comment.
Not without using a whole bunch of getattr and tests of whether those things have a metadata property. In this design, the PipelineTask author has to specifically choose which output datasets could get annotated on failure and pass them to annotate individually. It might be nice to just pass the result struct and let annotate figure it out, but that's probably more complicated than it's worth (not all datasets support metadata, e.g. AstropyArrow).
parejkoj
force-pushed
the
tickets/DM-39842
branch
from
49817ee
to
e5eb69d
Compare
parejkoj
force-pushed
the
tickets/DM-39842
branch
7 times, most recently
from
33d9efb
to
5d7d3d4
Compare
parejkoj
force-pushed
the
tickets/DM-39842
branch
from
5d7d3d4
to
cf9e092
Compare
|
|
||
| butlerQC.put(outputs, outputRefs) | ||
| # This should not happen with a properly configured execution context. | ||
| assert not inputs, "runQuantum got more inputs than expected" |
There was a problem hiding this comment.
If/when this falls over, will the error message specify that the runQuantum being talked about is the one from calibrateImage?
There was a problem hiding this comment.
Yes, the assert will give where it happened. This is a "this should absolutely never happen" error, hence the bare assert.
| butlerQC.put(result, outputRefs) | ||
| raise error from e | ||
|
|
||
| butlerQC.put(result, outputRefs) |
There was a problem hiding this comment.
Should this be in a finally block?
There was a problem hiding this comment.
No. We only butler.put if the task succeeded, or if AlgorithmError is caught, all other errors can't be annotated, so writing partial outputs is unhelpful (and potentially harmful, as downstream tasks won't know that they're partial).
| applied_photo_calib=None, | ||
| astrometry_matches=None, | ||
| photometry_matches=None, | ||
| ) |
There was a problem hiding this comment.
This feels like it could be fragile if we ever change this task to output more things in the result Struct. Is it not possible to construct a list of arguments to feed the error annotation by iterating over all the existing things in the Struct?
|
|
||
| self._summarize(exposure, stars, background) | ||
| self._summarize(result.exposure, result.stars_footprints, result.background) |
There was a problem hiding this comment.
Why is this using result.stars_footprints and not result.stars ?
There was a problem hiding this comment.
ComputeExposureSummaryStatsTask takes a SourceCatalog. result.stars is just for the output. Eventually we'll start converting downstream tasks (like ipdiffim, maybe?) to take AstropyArrow input catalogs, but that's a long ways off.
There was a problem hiding this comment.
Ah, so stars_footprints and stars has the same information but in different formats? Cute, OK, carry on.
There was a problem hiding this comment.
See the Connections here: https://github.com/lsst/pipe_tasks/blob/main/python/lsst/pipe/tasks/calibrateImage.py#L87
Eventually, the goal will be fore the stars_footprints to only contain the footprints and none of the catalog information, but that's a long ways off.
| @@ -448,7 +471,7 @@ def run(self, *, exposures, id_generator=None): | |||
| result : `lsst.pipe.base.Struct` | |||
| Results as a struct with attributes: | |||
There was a problem hiding this comment.
Is the results Struct guaranteed to have all of the documented attributes, now?
There was a problem hiding this comment.
I'm not sure what you mean?
If the task completes, it returns result. If an exception is raised, the input parameter has (hopefully!) been modified, but nothing is returned.
There was a problem hiding this comment.
Oh, but apparently I never added docs for the result parameter: I'll do that now.
There was a problem hiding this comment.
Here's what I added:
result : `lsst.pipe.base.Struct`, optional
Result struct that is modified to allow saving of partial outputs
for some failure conditions. If the task completes successfully,
this is also returned.
ed3e79f
to
3314006
Compare
Use the new Task exceptions and a pre-defined results struct to manage partial outputs for failures of CalibrateImage subtasks. Add tests of runQuantum exception handling. The input is now a multiple `exposures`, so we can use that to simplify the code by renaming output_exposure->exposure.
This didn't actually affect the tests, but it's better to have two distinct exposures in the test butler.
parejkoj
force-pushed
the
tickets/DM-39842
branch
from
3314006
to
f2d3e67
Compare
No description provided.