Details
-
Type:
RFC
-
Status: Implemented
-
Resolution: Done
-
Component/s: DM
-
Labels:None
Description
In the wake of DM-30649 (and the changes described in this Community post), I'd like to update lsst.verify.MetricTask to make use of the new workflow infrastructure. This would be a breaking change to the MetricTask API, affecting metric implementations throughout Science Pipelines.
Currently, lsst.verify.MetricTask has the following specification:
- All metric inputs are considered optional, and the run method must handle None values. (Not clear if this was actually possible before
DM-30649, but it does get unit tested.) - Errors in analysis must raise MetricComputationError.
- Metrics that are not applicable to the data (including because of missing data) must return None instead of raising.
The handling of this nonstandard interface is currently done by MetricTask.runQuantum, which forces a lot of boilerplate on metric developers who need to override runQuantum themselves.
I would like to redesign MetricTask to have the following specification:
- Metric tasks may assume that all their inputs be not None, like ordinary PipelineTasks. Supporting optional inputs is discouraged because it interferes with metrics from being attached to other pipelines (see discussion below).
- Errors in analysis must raise MetricComputationError.
- Metrics that are not applicable to the data must raise NoWorkFound.
This specification change will require changes to all MetricTask implementations. I think the only change that requires a deprecation period is the replacement of None output with NoWorkFound. This can be handled in most cases with a deprecation warning emitted by MetricTask.runQuantum, though tasks that override runQuantum will miss the warning. Per discussion below, there is no need for a deprecation period, as the Middleware framework already provides the desired fallback behavior.
Attachments
Issue Links
Activity
Hmm... no feedback, which I assume means no objections.
Jim Bosch, can you confirm that I'm using the new framework correctly? I'd like to be sure that I didn't misunderstand anything from the Community post.
Krzysztof Findeisen
added a comment - Hmm... no feedback, which I assume means no objections.
Jim Bosch , can you confirm that I'm using the new framework correctly? I'd like to be sure that I didn't misunderstand anything from the Community post. Metric tasks may require, and assume, that all their inputs be not None. If a task does have optional inputs, they should be PrerequisiteInputs with minimum=0. I think both of these already apply to PipelineTask in general, so they'd be implicit.
This is correct for multiple=False connections; it seems like those are the only ones you are concerned with, but I didn't want to assume. For multiple=True connections, "inputs be not None" translates to "at least one input will be present", and if a task wants to put further requirements on those, they can override adjustQuantum.
Also note that using PrerequisiteInput will also block the MetricTask from being run in the same pipeline as the task that produces that dataset, which is probably even less desirable for MetricTasks than other PipelineTasks, but it's a limitation that's hard for us to remove right now. So it might be that the best response to needing an optional input is (when possible) to refactor the MetricTask into multiple MetricTasks such that one can be skipped when its inputs are missing without affecting the other(s).
Metrics that are not applicable to the data must raise NoWorkFound.
Exiting without writing any datasets is handled the same way by the execution harness as NoWorkFound being raised, so if the base MetricTask already handles None returns by not writing anything (or could be made to do so), this may not need to be a disruptive change after all.
Thanks for clarifying the problem with PrerequisiteInput. It sounds like optional inputs actually can't be supported unless they're external, is that correct?
Not writing is exactly what MetricTask does.
Krzysztof Findeisen
added a comment - Thanks for clarifying the problem with PrerequisiteInput . It sounds like optional inputs actually can't be supported unless they're external, is that correct?
Not writing is exactly what MetricTask does . It sounds like optional inputs actually can't be supported unless they're external, is that correct?
Pretty much. Either external or something that is always going to be produced in a different production - so ISR's calibrations and templates in an AP-only ImageSubtractionTask (if such a thing were to exist) would also qualify.
Removing from flagged status, as the revised proposal no longer requires any deprecation.
Krzysztof Findeisen
added a comment - Removing from flagged status, as the revised proposal no longer requires any deprecation. Adopting as amended.
Krzysztof Findeisen
added a comment - Adopting as amended. 
Escalating as a deprecation request.