DM-21911: Retrofit Gen 3 functionality onto MetricTask #63
Conversation
| functionality. If subclasses override `setUp`, they must call | ||
| `MetricTaskTestCase.setUp`. | ||
| """ | ||
| @classmethod |
There was a problem hiding this comment.
Should this be a static method? Or if it was a regular method and the author of the task just defined what taskClass was, then this method could set the task attribute with whatever it took to instantiate it.
There was a problem hiding this comment.
Just naming the class is too inflexible, because the task may need specific constructor arguments (for example, ApdbMetricTask needs a custom config if we want to run it in an isolated environment). A factory is much more appropriate here, even if it is unpythonic.
There was a problem hiding this comment.
This feels like too many levels of indirection for what you are actually trying to do. setUp is a two line function, why not have children classes just implement what they need in this method? That is basically how TestCase already works.
There was a problem hiding this comment.
Because of weirdness in how unittest/pytest actually runs these tests, that doesn't work -- self.task needs to be assigned by the generic class, or generic tests won't see it. 🤷♂️ Believe me, I did try alternatives before picking this architecture.
The abstract config for MetricTask provides a centralized source of truth for the choice of metric.
If the metric name contains any "." characters, the user may get confusing Butler errors. Better to catch the problem during config validation.
The task was not yet used, but was too specific: the connections class forced each output granularity to have its own code, adding a lot of boilerplate.
This specialization copies the error-handling logic from gen2tasks.MetricsControllerTask to keep irregularities in individual metrics from stopping the entire pipeline. See DM-21937 for more details.
This change brings `MetadataMetricTask`'s terminology in line with the Gen 3 metadata convention discussed on DM-22162.
Now that MetricConfig provides a standardized way to define the metric, a property on the config makes more sense. Because this API works in Gen 2, and because all references to getOutputMetricName are in known classes, there is no deprecation period.
kfindeisen
force-pushed
the
tickets/DM-21911
branch
from
e904bf6
to
917456b
Compare
This PR makes
MetricTaskaPipelineTask, adding a connections class that defines the metric value. It also updates the existing connections of subclasses to take advantage ofMetricConnections, and modernizes the documentation.