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-46425: Move DECam precursor step to its own pipeline #159

Merged
merged 2 commits into from
Sep 25, 2024

Conversation

leeskelvin
Copy link
Contributor

No description provided.

Copy link
Contributor

@fred3m fred3m left a comment

Choose a reason for hiding this comment

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

LGTM. I'm surprised that this is the best way to replace a task, but if this is what's required then I'm fine with it. You should probably add a comment in the description that mentions this ticket and the the ticket that will make it obsolete (or at least the conditions for reverting it).

Comment on lines 9 to 12
- location: $DRP_PIPE_DIR/pipelines/_ingredients/DECam/DRP.yaml#isrForCrosstalkSources
exclude:
- isrForCrosstalkSources
- location: $ANALYSIS_DRP_DIR/pipelines/analysis_drp_plots.yaml
Copy link
Contributor

Choose a reason for hiding this comment

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

Is this really the way this is done? You import isrForCrosstalkSources (presumably to get its dependencies and config options) and then exclude it?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Right, I didn't want to touch the main _ingredients file, as long-term the hope is that this short-term workaround will not be required. However, I don't have a feel for how long that long-term fix will be in place, so we needed something that works for now.

My solution here was to leave _ingredients as-is, but exclude the offending task from the user-facing pipeline YAML, and construct a second YAML containing only the offending task.

However, I'm not averse to doing something completely different if you can think of a more optimal approach?

Copy link
Contributor Author

@leeskelvin leeskelvin Sep 24, 2024

Choose a reason for hiding this comment

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

The alternative to the approach on this PR (lets call this PR option a) is to strip isrForCrosstalkSources out of the main _ingredients file and put it either b) into its own _ingredients file, which then gets imported into its own user-facing YAML, or c) just define the task directly in its own user-facing YAML.

I don't have a strong preference for any of these options.

Copy link
Contributor

Choose a reason for hiding this comment

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

I agree this is weird, but I think if you add an in-line comment explaining why you import a task and then immediately exclude it, all will be forgiven 😄

Copy link
Contributor

Choose a reason for hiding this comment

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

I agree that this is probably fine as is, provided there is a bit more explanation in the description as to why this is being done and when it can be reverted.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Ah, wait! Apologies, I did not intend to push the #isrForCrosstalkSources - this is a mistake. The power of review for the win.

It should instead just be the main pipeline YAML.

Copy link
Contributor

Choose a reason for hiding this comment

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

That makes more sense!!

Copy link
Contributor Author

Choose a reason for hiding this comment

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

My bad - this section should be (and now is):

imports:
  - location: $DRP_PIPE_DIR/pipelines/_ingredients/DECam/DRP.yaml
    exclude:
      - isrForCrosstalkSources
  - location: $ANALYSIS_DRP_DIR/pipelines/analysis_drp_plots.yaml

I was testing out if you can include only a single task from a YAML (note: you can't), but I forgot to remove my test!

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thanks for picking up on this @fred3m and @mrawls! 🥲

@leeskelvin
Copy link
Contributor Author

Thanks for catching the typo in the YAML above.

@fred3m, since your initial review, I've also added a step0 subset to the new YAML file, to keep continuity with the existing step0 terminology. Additionally, I've updated the test_pipelines unit-test to account for this new YAML file.

@leeskelvin leeskelvin merged commit be71bff into main Sep 25, 2024
3 checks passed
@leeskelvin leeskelvin deleted the tickets/DM-46425 branch September 25, 2024 02:59
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.

3 participants