Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

DM-41108: add associated diaSources to diaPipe output #217

Merged
merged 2 commits into from Mar 15, 2024

Conversation

isullivan
Copy link
Contributor

  • updates the unit tests to account for changing doWriteAssociatedSources to True in diaPipe.
  • ap_pipe now includes analysis_tools metrics, so make sure they only show up in the ap_verify pipelines once.

  • [ x] Do unit tests pass (scons and/or stack-os-matrix)?
  • [ x] Did you run ap_verify.py on at least one of the standard datasets?
    For changes to metrics, the print_metricvalues script from lsst.verify will be useful.
  • [x ] Is the Sphinx documentation up-to-date?

Copy link
Member

@kfindeisen kfindeisen left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The second commit seems to be trying to hack around an underlying problem, possibly in the ApPipe definition; happy to help with diagnosing and fixing it.

pipelines/_ingredients/ApVerify.yaml Outdated Show resolved Hide resolved
@kfindeisen
Copy link
Member

Oh, I forgot: when rebasing, please update https://github.com/lsst/ap_verify/blob/main/pipelines/_ingredients/MetricsRuntime.yaml#L276-L278 to account for analyzeAssocDiaSrcCore (I assume it's executed after analyzeTrailedDiaSrcCore?).

@isullivan
Copy link
Contributor Author

Oh, I forgot: when rebasing, please update https://github.com/lsst/ap_verify/blob/main/pipelines/_ingredients/MetricsRuntime.yaml#L276-L278 to account for analyzeAssocDiaSrcCore (I assume it's executed after analyzeTrailedDiaSrcCore?).

Thinking about this further, I believe we want the current timing metric to only include the tasks up through diaPipe and writing to the APDB. We might want an additional timing metric to include the afterburner, or at least capture the timing for the afterburner by itself. I don't believe we have any guarantee in what order the analysis_tools tasks are executed, so I suspect we would have to capture the timing of each task separately.

@kfindeisen
Copy link
Member

Thinking about this further, I believe we want the current timing metric to only include the tasks up through diaPipe and writing to the APDB.

Then the metric is no longer even pretending to measure the running time of ApPipe, making it even more arbitrary. I suggest removing it or, barring that, editing its description in verify_metrics.

@isullivan
Copy link
Contributor Author

Thinking about this further, I believe we want the current timing metric to only include the tasks up through diaPipe and writing to the APDB.

Then the metric is no longer even pretending to measure the running time of ApPipe, making it even more arbitrary. I suggest removing it or, barring that, editing its description in verify_metrics.

I don't feel like the timing metric is arbitrary; it's tracking the wall clock time of the core AP tasks that contribute towards our timing requirements. It is clearly not the entire time since it can't include data transfer (etc..), but it's the part we have the most direct control over. The analysis_tools "afterburner" can happen after we send alerts, so I don't believe we need to include it in the timing metric that tracks the core pipeline time. Would it help if I changed the text in verify metrics from "the entire AP pipeline" to "the core AP pipeline", or something similar?

@kfindeisen
Copy link
Member

It's arbitrary for two reasons:

  • (pre-existing) in ap_verify, BPS, or other batch processing, where this is actually run, the quanta are processed in parallel as available/needed. The time between ISR for one data ID and prompt processing for the same data ID is not a physical quantity because there is no linear sequence of tasks (and in fact the measured time scales with the number of data IDs being processed together). So no, we do not have "direct control" over the result, not even if we run with --processes 1.
  • (new) the AP pipeline is whatever is in ApPipe.yaml; any attempt to divide it into "core" or "fringe" tasks is a philosophical exercise that does not affect processing. If you want a metric that measures the time at which alerts are sent (or mocked, in batch?), you should say what you mean: measure that and call it something other than "the total run time of the AP pipeline", instead of trying to proxy things so that the overheads cancel out.

The most natural place to constrain time-to-alerts would be in Prompt Processing itself (-dev or -prod), where we have the appropriate environment and execution strategy. But something like HFC's notebooks would be a more appropriate tool than lsst.verify for that.

@isullivan
Copy link
Contributor Author

OK, I completely agree on two points: any official "time-to-alerts" metric should only come from Prompt Processing (and can't be captured by lsst.verify metrics), and if the afterburner metrics are part of ApPipe.yaml then a timing metric that claims to be the total time for ApPipe should include everything in the pipeline. I will update MetricsRuntime as you originally suggested.

Separately, I do also want a timing metric for "core" AP, so that we can monitor the timing of just that portion of the pipeline with our CI datasets. This would be just to monitor trends over time, and not anything we would officially report. That can be done on a different ticket, though.

Copy link
Member

@kfindeisen kfindeisen left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

No objections as things stand. Thanks for hearing me out!

@isullivan isullivan merged commit 193d4ad into main Mar 15, 2024
2 checks passed
@isullivan isullivan deleted the tickets/DM-41108 branch March 15, 2024 19:37
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
2 participants