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

Reorganise/refactor the reco1 workflow to reinstate standard_reco1_sbnd.fcl as the standard fcl #460

Merged
merged 40 commits into from
Jul 16, 2024

Conversation

absolution1
Copy link
Contributor

@absolution1 absolution1 commented Apr 19, 2024

As (slowly) promised, here is the first PR to reorganise the fcls. This reorganisation solely focusses on reco1, but also lays the groundwork for how I would reorganise the other fcls.

I've also taken the liberty of removing all 3drift workflows from sbndcode.

So, the basic idea.

  • generic_job_sbnd.fcl creates a skeleton job fcl that sets up the common structure. standard_reco1_sbnd.fcl inherits from this fcl to avoid having to configure generic job stuff. The plan will be for all job fcls to inherit from this fcl
  • workflow_reco1.fcl creates a table of all producers/analyzers, creates all of the sequences and overrides all of the product label names that need overriding
  • drops_reco1.fcl configures what needs to be dropped. This is largely similar to before
  • standard_reco1_sbnd.fcl now just pulls the various configs together

Diff'ing the fcls shows that nothing crazy has changed, and is posted below. The differences you can see are explained by

  • the original drops list tried to drop some data products more than once. The new config does not do that.
  • I've changed the label of the ana sequence, which is why there are supera differences
[sbndbuild01.fnal.gov@expdatalink]$ diff larOriginal.txt larRefactor.txt
10,14d9
<          "drop raw::OpDetWaveforms_*_*_*",
<          "drop *raw::RawDigits*_*_*_*",
<          "drop sim::OpDetBacktrackerRecords_*_*_*",
<          "drop *_fasthit_*_*",
<          "drop *_cluster3d_*_*",
19a15
>          "drop *_fasthit_*_*",
27a24,26
>    ana: [
>       "supera"
>    ]
41c40
<       "superaana"
---
>       "ana"
797,799d795
<    superaana: [
<       "supera"
<    ]
3450c3446
<          fileName: "larOriginal.txt"
---
>          fileName: "larRefactor.txt"

I think this reorganisation for reco1 is largely feature-complete so I am happy for it to be reviewed now.

TODO:

  • remove some last reco1 fcls that aren't needed anymore
  • update the readme to say to use standard_reco1_sbnd.fcl
  • add fcl that does not drop any products

Conflicts:
	sbndcode/JobConfigurations/standard/standard_reco1_sbnd.fcl
@absolution1 absolution1 requested a review from jzennamo April 19, 2024 16:39
@absolution1 absolution1 marked this pull request as ready for review July 15, 2024 11:00
@absolution1
Copy link
Contributor Author

absolution1 commented Jul 15, 2024

Apologies for the slowness on this one.

@marcodeltutto I've addressed your comments (removed the version/name info from the fcls)

@fjnicolas I think the reason you're now seeing drops is probably because we're comparing to two different reference fcls. I'm comparing to reco1_sce_lite.fcl whereas (I guess) the CI compares to reco1_sce.fcl or standard_reco1_sbnd.fcl.
The idea with this PR is that the refactored standard_reco1_sbnd.fcl copies the current standard, which is reco1_sce_lite.fcl.
To mitigate some of this, I've included an extra fcl that does not drop -any- products (called reco1_keepproducts.fcl). In this fcl, the raw information is also not dropped.

@bear-is-asleep I've edited and renamed your reco1 fcl for training cosmics reco1_sce_mpvmpr_lite.fcl -> reco1_mpvmpr.fcl

@bear-is-asleep I've now removed the draft tag. If everyone agrees, I think this can now go in.

Copy link
Contributor

@bear-is-asleep bear-is-asleep left a comment

Choose a reason for hiding this comment

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

My only suggestion is to move the keepproducts fcl to the config directory, but I will approve with or without this change. Thanks for doing this again Dom!

@bear-is-asleep
Copy link
Contributor

trigger build LArSoft/lar*@LARSOFT_SUITE_v09_90_00 SBNSoftware/sbncode@v09_90_00

@FNALbuild
Copy link
Collaborator

✔️ CI build for LArSoft Succeeded on slf7 for c14:prof -- details available through the CI dashboard

@FNALbuild
Copy link
Collaborator

✔️ CI build for LArSoft Succeeded on slf7 for e26:prof -- details available through the CI dashboard

@FNALbuild
Copy link
Collaborator

❌ CI build for SBND Failed at phase ci_tests SBND on slf7 for c14:prof - ignored failure for unit_test - ignored warnings for build -- details available through the CI dashboard

🚨 For more details about the failed phase, check the ci_tests SBND phase logs

parent CI build details are available through the CI dashboard

@FNALbuild
Copy link
Collaborator

❌ CI build for SBND Failed at phase ci_tests SBND on slf7 for e26:prof - ignored failure for unit_test - ignored warnings for build -- details available through the CI dashboard

🚨 For more details about the failed phase, check the ci_tests SBND phase logs

parent CI build details are available through the CI dashboard

@absolution1
Copy link
Contributor Author

ah @bear-is-asleep @fjnicolas this is a fun one!
Obviously, my changes are a reorganisation (including deletion) of some fcls. This includes deletion of the fcl that the CI uses (reco1_sce.fcl)
Our options here

  • accept the results of the previous test and update the CI
  • I temporarily add back in reco1_sce.fcl so that the CI can run. I then delete it after the test and the CI is updated.

For what its worth, the previous test should still be completely valid as the only changes I've made recently are deletion of the redundant fcls.

@bear-is-asleep
Copy link
Contributor

ah @bear-is-asleep @fjnicolas this is a fun one! Obviously, my changes are a reorganisation (including deletion) of some fcls. This includes deletion of the fcl that the CI uses (reco1_sce.fcl) Our options here

  • accept the results of the previous test and update the CI
  • I temporarily add back in reco1_sce.fcl so that the CI can run. I then delete it after the test and the CI is updated.

For what its worth, the previous test should still be completely valid as the only changes I've made recently are deletion of the redundant fcls.

The CI should be updated to use the new fcls. Perhaps we can loop in the CI group @RachelCoackley to verify the relevant changes are made so the burden is not solely on you to update it @absolution1 . For example a few simulation pipelines failed where the reco1 stage is ran, which will probably need to be updated with this PR.

It does seem there's a genuine error with reco1_keepproducts.fcl here

36:   detected at or near line 4, character 25, of file "/scratch/workspace/sbnd_ci/label_exp/swarm/label_exp2/swarm/SBND/build_slf7.x86_64/sbndcode/fcl/reco1_keepproducts.fcl"
37:   
38:            "keep *_*_*_*",
39:                           ^
40: ---- Parse error END

This is all I found that needs to be updated with the CI, which may solve these issues.

@absolution1
Copy link
Contributor Author

Hi @bear-is-asleep
I've fixed the keepproducts fcl with this commit: 84c03fb
(sorry about that, that one must have slipped through testing)

I've had a chat with @henrylay97 about the CI and I think we've identified all of the places that need changing. This was done here: 7aaa401

Could we try the CI again?

@absolution1
Copy link
Contributor Author

trigger build LArSoft/lar*@LARSOFT_SUITE_v09_90_00 SBNSoftware/sbncode@v09_90_00

@FNALbuild
Copy link
Collaborator

✔️ CI build for LArSoft Succeeded on slf7 for c14:prof -- details available through the CI dashboard

@FNALbuild
Copy link
Collaborator

✔️ CI build for LArSoft Succeeded on slf7 for e26:prof -- details available through the CI dashboard

@FNALbuild
Copy link
Collaborator

❌ CI build for SBND Failed at phase ci_tests SBND on slf7 for c14:prof - ignored failure for unit_test - ignored warnings for build -- details available through the CI dashboard

🚨 For more details about the failed phase, check the ci_tests SBND phase logs

parent CI build details are available through the CI dashboard

@FNALbuild
Copy link
Collaborator

⚠️ CI build for SBND Warning at phase ci_tests SBND on slf7 for e26:prof - ignored failure for unit_test - ignored warnings for build -- details available through the CI dashboard

🚨 For more details about the warning phase, check the ci_tests SBND phase logs

parent CI build details are available through the CI dashboard

@bear-is-asleep
Copy link
Contributor

Looks like the trigger passed and you have the approvals needed. I'll go ahead and merge, thanks @absolution1 ! Also good shout to @henrylay97 for helping with the CI!

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.

6 participants