DM-48044: Test subset definitions in ap_pipe pipeline tests #216
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
parejkoj
reviewed
Dec 18, 2024
There was a problem hiding this comment.
Handful of comments, mostly clarifications of test docs and/or intent. In general, this seems like a good approach, and it looks like it caught a few incomplete pipeline definitions.
Thanks for using subTests! That should make debugging test failures much easier.
While you're in here: there's a bunch of extra spaces after mpSkyEphemerisQuery in the generic ApPipe.yaml (my editor highlights whitespace at the end of lines).
| """Test that instrument-specific pipelines have all the subsets of their | ||
| generic counterparts. | ||
|
|
||
| Note that this does not check inheritance *within* `_ingredients`! |
There was a problem hiding this comment.
Just to be explicit?
Suggested change
| Note that this does not check inheritance *within* `_ingredients`! | |
| Note that this does not check inheritance *within* `_ingredients/`! |
I don't remember why I recommended using `processCcd` here, but it's surprising behavior, and will make less sense once we remove ProcessCcd.yaml.
These subsets have never been needed (these pipelines are an implementation detail of DECam/ProcessCcd.yaml), but I can't find a good a priori reason why they should have an exception in the pipeline tests.
These tests verify our de-facto conventions for AP pipelines, but try to avoid hardcoding which tasks must be associated with which subsets.
These subsets were previously documented only in an RFC, but they're a key part of the interface that Prompt Processing uses to run the pipeline.
kfindeisen
force-pushed
the
tickets/DM-48044
branch
from
ddcc95c to
5eae529
Compare
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
This PR fixes outstanding issues with subset definitions, and adds unit tests to check the more mechanical ones.