DM-10779: Implement running time metric(s) #8
Conversation
There was a problem hiding this comment.
Two recommended changes:
-
Change the comment line at the top of packages to not say "Alert Production metric definitions". This is because there's one metrics file per package, unlike specifications, so we can't separate by metric users, per se.
-
I think the
verify_ap.yamlfile needs to be renamed.
Other than that, I like your use of YAML anchors & references to DRY the definitions.
Have you verified that lsst.verify parses everything okay?
metrics/verify_ap.yaml
Outdated
| @@ -0,0 +1,13 @@ | |||
| # Alert Production metric definitions in verify_ap | |||
There was a problem hiding this comment.
Should this be ap_verify now?
metrics/pipe_tasks.yaml
Outdated
| @@ -0,0 +1,29 @@ | |||
| # Alert Production metric definitions in pipe_tasks | |||
There was a problem hiding this comment.
Since these are metrics generically for pipe_tasks, I'd say "pipe_tasks metric definitions" and not mention alert production specifically. (This is because there's one metrics file per package, unlike specifications, so we can't separate by metric users, per se).
There was a problem hiding this comment.
My intent was that these would evolve into section headers:
# Alert Production metric definitions in pipe_tasks
[code]
# Data Release Production metric definitions for pipe_tasks
[code]
Is that not a good way of organizing the metrics within each package?
There was a problem hiding this comment.
Oh I see what you mean. I guess the trick is that some metrics (especially generic ones like run times, may be shared by multiple user groups. Using tags like you are is a good way to declare usage/ownership. But it's fine if you want to keep those headers as-is for now.
There was a problem hiding this comment.
Anonymized the headers. You're right that the concept of metric ownership might not be a useful or desirable one.
metrics/meas_astrom.yaml
Outdated
| @@ -0,0 +1,13 @@ | |||
| # Alert Production metric definitions in meas_astrom | |||
There was a problem hiding this comment.
Same comment as in pipe_tasks.yaml
metrics/meas_algorithms.yaml
Outdated
| @@ -0,0 +1,13 @@ | |||
| # Alert Production metric definitions in meas_algorithms | |||
There was a problem hiding this comment.
Same comment as in pipe_tasks.yaml
metrics/ip_diffim.yaml
Outdated
| @@ -0,0 +1,17 @@ | |||
| # Alert Production metric definitions in ip_diffim | |||
There was a problem hiding this comment.
Same comment as in pipe_tasks.yaml
metrics/ap_association.yaml
Outdated
| @@ -0,0 +1,13 @@ | |||
| # Alert Production metric definitions in ap_association | |||
There was a problem hiding this comment.
Same comment as in pipe_tasks.yaml
kfindeisen
force-pushed
the
tickets/DM-10779
branch
from
4bf7868
to
47aff28
Compare
This ticket adds running time metrics for major Tasks run by the AP pipeline. These metrics are not associated with any specifications.