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-47299: Add the afterburner subset to all of the pipelines #240

Merged
merged 1 commit into from
Nov 6, 2024

Conversation

isullivan
Copy link
Contributor

{Summary of changes. Prefix PR title with JIRA issue.}


  • Do unit tests pass (scons and/or stack-os-matrix)?
  • 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.
  • 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.

Strictly speaking, the instrument-specific ApVerify should splice in the afterburner subset from the instrument-specific ApPipe -- as it is, the ApVerify will drop any overrides to afterburner tasks from ap_pipe. I'd recommend doing that for consistency's sake, even though the odds of such overrides existing are pretty low.

The problem would be if we end up overriding any of those tasks for ap_verify, like we do with diaPipe...

@isullivan
Copy link
Contributor Author

Good point. I have moved the import to the instrument-specific ApVerify definitions.

@kfindeisen
Copy link
Member

I hope you meant copy (_instrument should still be a representative pipeline), and the same goes for ApVerifyWithFakes.

@@ -11,6 +11,7 @@ imports:
- location: $AP_PIPE_DIR/pipelines/DECam/ApPipe.yaml
include:
- prompt
- afterburner
Copy link
Member

Choose a reason for hiding this comment

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

You'd need to also exclude from _ingredients/ApVerify.yaml, else you have double definitions.

@isullivan
Copy link
Contributor Author

I had moved the imports rather than copied and excluded them, but I have fixed that up now.

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.

Looks good, but please confirm that the final pipelines are still valid.

@isullivan isullivan merged commit 04f338d into main Nov 6, 2024
2 checks passed
@isullivan isullivan deleted the tickets/DM-47299 branch November 6, 2024 17:16
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants