DM-16017: Prototype a metrics-handling Task #30
Conversation
There was a problem hiding this comment.
Looks good. Mostly just thoughts on the docs. The only code recommendation I have from an lsst.verify point of view is that I think you can get away with just instantiating a job = Job() in the controller, rather than also loading up metric and specification data (job = Job.load_metrics_package())
|
|
||
| return Struct(jobs=jobs) | ||
|
|
||
| def _nameJobFile(self, index, dataId): |
There was a problem hiding this comment.
Normally when we say "file" in Python we're talking about a file handle. I think in this case its a "Path" instead?
There was a problem hiding this comment.
"path" seems kind of redundant with "name"; would _jobFilePath be acceptable?
| jobs = [] | ||
| index = 0 | ||
| for dataref in datarefs: | ||
| job = Job.load_metrics_package() |
There was a problem hiding this comment.
I don't think you need to load the metrics package into the Job since you aren't working with job.metrics or job.specs. I think
job = Job()
is sufficient (and faster).
There was a problem hiding this comment.
Can you elaborate? While it would certainly make testing easier for me, SQR-019 recommends the current method.
Also, I noticed while working on a previous ticket that I get errors if I try to use the wrong units for a measurement value. Will those checks still happen if I use a bare Job, and if so, when/where? During SQuaSH upload?
There was a problem hiding this comment.
The unit compatibility check will happen during dispatch_verify, which does run Job.load_metrics_package and integrates in the metric and specification with any new measurement data.
Unless you think tasks will need to do something based on metric and specification data, you can save time by not loading it at this point.
This commit includes MetricTask and unit test code, but not documentation. A guide to the MetricTask framework is not being added at this stage; it will be written once prototyping is complete.
kfindeisen
force-pushed
the
tickets/DM-16017
branch
2 times, most recently
from
7daed43
to
05ff133
Compare
The current implementation is extremely crude, and has some hard-coded dependencies on the LSST pipelines. These will be removed in future work.
kfindeisen
force-pushed
the
tickets/DM-16017
branch
from
05ff133
to
11001f1
Compare
This PR adds prototype implementations of the
MetricTaskandMetricsControllerTaskclasses described by DMTN-098. The implementation ofMetricsControllerTaskcurrently relies on a hard-coded copy ofap.verify.measurements.profiling.TimingMetricTask(see lsst/ap_verify#55 for the "real" version). Its removal is ticketed as DM-16535; setting up a proper configuration system forMetricsControllerTaskwas deemed too challenging for an initial prototype.This PR adds API documentation but no package-level documentation. The current code is not suitable for general use, so I would prefer to defer writing package-level docs until
MetricsControllerTaskis closer to its intended form.