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

[absolution1]

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:

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

[fjnicolas]

trigger build LArSoft/lar*@LARSOFT_SUITE_v09_89_01 SBNSoftware/sbndcode@v09_89_01

[FNALbuild]

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

[FNALbuild]

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

[FNALbuild]

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

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

parent CI build details are available through the CI dashboard

[FNALbuild]

:x: 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

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

parent CI build details are available through the CI dashboard

[fjnicolas]

Hi @absolution1 thanks for this! Do you want to remove the draft mode and we include it the next release? The CI didn't complain and the data product outputs and resource usage are the same, so the PR is effectively being transparent.

I just have two comments from what I saw in the CI warnings (apart from the anticipated name difference in the ana sequence):

• Do we want/need to keep SimEnergyDepositLite (sedlite)? This was changed by the PR (they are currently not being dropped). I'm pinging @bear-is-asleep here (since he is the ML link and also taking over as release manager).

869: < G4 | sedlite |  | std::vector<sim::SimEnergyDepositLite> | 21412
870: ---
871: > G4 | sedlite |  | std::vector<sim::SimEnergyDepositLite> | ?

• SimPhotonslite are now dropped after Reco1. I don't know if it was intentional or not, as the drop is coming from the DetSim drop sequence:

  sbnd_reco1_drops: [ 
  @sequence::detsim_drops
  , "drop raw::RawDigits_*_*_*"

  Are we sure we want to do that? (I think we shouldn't but I'm very biased here as a PDS person :) )

[bear-is-asleep]

Hi @absolution1 thanks for this! Do you want to remove the draft mode and we include it the next release? The CI didn't complain and the data product outputs and resource usage are the same, so the PR is effectively being transparent.

I just have two comments from what I saw in the CI warnings (apart from the anticipated name difference in the ana sequence):

• Do we want/need to keep SimEnergyDepositLite (sedlite)? This was changed by the PR (they are currently not being dropped). I'm pinging @bear-is-asleep here (since he is the ML link and also taking over as release manager).

869: < G4 | sedlite |  | std::vector<sim::SimEnergyDepositLite> | 21412
870: ---
871: > G4 | sedlite |  | std::vector<sim::SimEnergyDepositLite> | ?

• SimPhotonslite are now dropped after Reco1. I don't know if it was intentional or not, as the drop is coming from the DetSim drop sequence:

sbnd_reco1_drops: [ 
    @sequence::detsim_drops
  , "drop raw::RawDigits_*_*_*"

Are we sure we want to do that? (I think we shouldn't but I'm very biased here as a PDS person :) )

sedlite is needed for supera, so up until reco1. We're working on phasing it out, so I'll remove the producer when the time comes.

I think we can just drop it at reco1 if that works. @VCLanNguyen noticed that dropping data products effects fcls that do a reco mop up, so this should be kept in mind for those fcls. The standard fcl will be fine if it's dropped at reco1 though.

[VCLanNguyen]

t dropping data products effects fcls that do a reco mop up, so

Hi, sorry for late reply. Not any useful replies but I don't recall running reco mop up myself....Maybe you mistake me for someone else :grimacing:

[bear-is-asleep]

Hi @absolution1 , is this PR ready to be merged? You have approval and I fear if we sit on this too long this PR will be out of date.

[absolution1]

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.

[bear-is-asleep]

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

[FNALbuild]

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

[FNALbuild]

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

[FNALbuild]

:x: 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

:rotating_light: 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]

:x: 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

:rotating_light: 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]

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]

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.

• https://github.com/SBNSoftware/sbndcode/blob/106153ed7885006213eccf43a2d21099a9c145a6/test/JobConfigurations/fast_simulation_reconstruction_test_muons.sh#L13
• https://github.com/SBNSoftware/sbndcode/blob/106153ed7885006213eccf43a2d21099a9c145a6/test/JobConfigurations/fast_simulation_reconstruction_test_electrons.sh#L13
• https://github.com/search?q=repo%3ASBNSoftware%2Fsbndcode%20path%3A%2F%5Etest%5C%2Fci%5C%2F%2F%20reco1&type=code

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]

Hi @bear-is-asleep I've fixed the keepproducts fcl with this commit: https://github.com/SBNSoftware/sbndcode/pull/460/commits/84c03fb0c570ea7b7e19b08eb1f25e5fb3deab6a (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: https://github.com/SBNSoftware/sbndcode/pull/460/commits/7aaa401e6850d6f096ea688ef1a8c48dd479a4dc

Could we try the CI again?

[absolution1]

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

[FNALbuild]

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

[FNALbuild]

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

[FNALbuild]

:x: 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

:rotating_light: 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]

:warning: 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

:rotating_light: 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]

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!