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-32029: Add faro steps to DRP.yaml #393

Merged
merged 5 commits into from Oct 14, 2021
Merged

DM-32029: Add faro steps to DRP.yaml #393

merged 5 commits into from Oct 14, 2021

Conversation

jeffcarlin
Copy link
Contributor

Successful Jenkins run is at https://ci.lsst.codes/blue/organizations/jenkins/stack-os-matrix/detail/stack-os-matrix/35175/pipeline/

The blocker (DM-32059) has been completed and merged - that ticket removed the unused faro tasks from the main pipelines so they wouldn't cause problems in obs_subaru.

Copy link
Member

@arunkannawadi arunkannawadi left a comment

Choose a reason for hiding this comment

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

On the commits themselves - why are there two sets of commits with identical messages? There probably shouldn't be the merge commit either. And commit messages should not end in a period

@@ -160,6 +161,45 @@ subsets:
The multiVisit subset defined in pipe_tasks' DRP.yaml is not safe to
use on HSC for various reasons; use 'step1', 'step2', and 'step3' subsets
instead. It may be re-enabled in the future.
faro:
Copy link
Member

Choose a reason for hiding this comment

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

Would it make sense to categorize faro tasks as faro_step1, faro_step2 etc. based on when they can be run and faro_step1 can be included in step1? If that is not possible to merge before the weekly deadline, please go ahead and merge but you could consider reorganization later. The reason I ask is that it'd make it easier to add new metrics/tasks later on as we add them.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

We could do that - the reason the faro subset exists is so that it's easier to exclude the faro steps from ci_hsc_gen3, which returns almost all NaNs from faro tasks. But we could easily exclude multiple faro subsets from ci_hsc_gen3 processing.

I would rather do that reorganization later (it's a good idea!). I think for now this satisfies the goal of integrating faro into the processing workflow.

@arunkannawadi arunkannawadi self-requested a review October 14, 2021 01:16
@jeffcarlin
Copy link
Contributor Author

jeffcarlin commented Oct 14, 2021

On the commits themselves - why are there two sets of commits with identical messages? There probably shouldn't be the merge commit either. And commit messages should not end in a period

I think something got funky when I rebased. I can squash those commit messages before merging.

@@ -182,6 +231,7 @@ subsets:
- fgcmBuildStarsTable
- fgcmFitCycle
- fgcmOutputProducts
- faro_step2
Copy link
Contributor

Choose a reason for hiding this comment

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

You cannot use subsets inside other subsets at this time

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, too bad. Thanks for the info, though -- I'll put the explicit task labels back in the pipelines.

Copy link
Contributor

Choose a reason for hiding this comment

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

Its a common assumption (and a good use case), but when subsets were created we didnt want to deal with the complexity of things like circular references and such. I have some ideas about this, but for now they cant be used, sorry!

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Totally makes sense. That could easily become circular...

@natelust
Copy link
Contributor

I think some of the faro task labels are missing from the subsets, please make sure all the tasks defined by faro appear inside a subset, otherwise unintended tasks and or failures will occur.

@jeffcarlin
Copy link
Contributor Author

I think some of the faro task labels are missing from the subsets, please make sure all the tasks defined by faro appear inside a subset, otherwise unintended tasks and or failures will occur.

This should be fixed now. I removed all the superfluous tasks from the default faro pipelines in DM-32059.

@arunkannawadi
Copy link
Member

arunkannawadi commented Oct 14, 2021 via email

@arunkannawadi
Copy link
Member

arunkannawadi commented Oct 14, 2021

What you have seems to be a good compromise between what is possible and what would have been ideal (but impossible). And you seem to have picked up some other version of my commits from yesterday which are already merged onto master. Have you rebased it correctly? Please checkout master branch, pull all the changes, and rebase this ticket branch onto the latest master. Drop the merge commits and any other commits that have to do with GAaP

@jeffcarlin jeffcarlin merged commit e427482 into master Oct 14, 2021
@jeffcarlin jeffcarlin deleted the tickets/DM-32029 branch October 14, 2021 23: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.

None yet

3 participants