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

include Scouting packers and collections in MC production [8_0_X] #40708

Merged

Conversation

missirol
Copy link
Contributor

@missirol missirol commented Feb 6, 2023

PR description:

This PR is a partial backport of #21297 (and #22943), to address the use case described in #40691. It allows to produce the HLT Scouting collections when running the HLT as a 'step' of cmsDriver.py, which is what is done in MC productions.

The EventContents related to HLT are updated accordingly, adding the Scouting collections. This extends the content of the RAW data tier for MCs in 8_0_X (because of the addition of the HLT Scouting collections). A similar update was done for MCs in 9_4_X when #21297 was backported to that cycle with #21295.

Extra details for HLT.

  • An additional ad-hoc change is needed for TSG menus of 8_0_X, to remove the smart-prescale modules from the Scouting (End)Paths when converting said menus to cff fragments.
  • The update of the subtables from V253 to V254 is unconsequential; there are no differences between V253 menus and V254 menus.

Summary.

  • There are no changes to the HLT menus in ConfDB.
  • This PR includes the backport of a fix to (1) the routine that converts the HLT menus to cff python files (and the HLT-menu configs are updated accordingly), and (2) to the HLT EventContents.
  • The consequence is that the cff python files of HLT menus with Scouting will now include 2 more Paths with the Scouting "packers" (i.e. the producers of the Scouting objects).
  • A 2nd, more important, consequence is that this fix extends the data tiers involving HLT outputs, incl. the RAW data tier. The only change is the addition of the Scouting objects.
  • The same type of fix was backported years ago to CMSSW_9_4_X with Include "Scouting" packers and collections in MC production #21295 for 2017 MCs (this PR is effectively the same fix for 2016 MCs, but done much, much, later).

PR validation:

TSG tests.

If this PR is a backport, please specify the original PR and why you need to backport that PR. If this PR will be backported, please specify to which release cycle the backport is meant for:

Partial backport of #21297 (and #22943).

Use case: future 2016 ultra-legacy MC productions (central or private).

@cmsbuild
Copy link
Contributor

cmsbuild commented Feb 6, 2023

A new Pull Request was created by @missirol (Marino Missiroli) for CMSSW_8_0_X.

It involves the following packages:

  • HLTrigger/Configuration (hlt)

@cmsbuild, @missirol, @Martin-Grunewald can you please review it and eventually sign? Thanks.
@Martin-Grunewald, @silviodonato this is something you requested to watch as well.
@perrotta, @dpiparo, @rappoccio you are the release manager for this.

cms-bot commands are listed here

@missirol
Copy link
Contributor Author

missirol commented Feb 6, 2023

Summary.

  • There are no changes to the HLT menus in ConfDB.
  • This PR includes the backport of a fix to (1) the routine that converts the HLT menus to cff python files (and the HLT-menu configs are updated accordingly), and (2) to the HLT EventContents.
  • The consequence is that the cff python files of HLT menus with Scouting will now include 2 more Paths with the Scouting "packers" (i.e. the producers of the Scouting objects).
  • A 2nd, more important, consequence is that this fix extends the data tiers involving HLT outputs, incl. the RAW data tier. The only change is the addition of the Scouting objects.
  • The same type of fix was backported years ago to CMSSW_9_4_X with Include "Scouting" packers and collections in MC production #21295 for 2017 MCs (this PR is effectively the same fix for 2016 MCs, but done much, much, later).

@cms-sw/orp-l2 @cms-sw/core-l2

If you consider this update too intrusive for such an old cycle, @cms-sw/hlt-l2 would be okay with not merging it (opening the PR makes it at least available to users).

What is your take on this? What options do you see?

Comment on lines +71408 to +71409
fragment.ScoutingCaloOutput = cms.Path( fragment.hltGtStage2Digis + fragment.hltPreScoutingCaloOutput + fragment.hltFEDSelectorL1 + fragment.hltScoutingCaloPacker )
fragment.ScoutingPFOutput = cms.Path( fragment.hltGtStage2Digis + fragment.hltPreScoutingPFOutput + fragment.hltFEDSelectorL1 + fragment.HLTPFScoutingPackingSequence )
Copy link
Contributor Author

Choose a reason for hiding this comment

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

I have done this PR "as a backport" (meaning, trying not to do new things), but I'm wondering if this change works (it might just be my ignorance of the framework).

Taking hltScoutingCaloPacker as an example, it consumes products that are not in the Path ScoutingCaloOutput, and HLTScoutingCaloProducer will not complain about missing collections (it just produces empty/dummy collections if no inputs are found). The intention would be that hltScoutingCaloPacker 'waits' for those collections, but my understanding is that, if more than one thread is used (which is likely the case in production), there is no guarantee on the order in which the Paths run, so in general this would not work (it may work with 1 single thread, because the Paths are executed in order, and the Paths with the Scouting packers are towards the end of the Schedule). It can also be that (1) my understanding is wrong, and/or (2) 8_0_X behaves differently from recent cycles in this respect.

@makortel, could you please clarify (and correct any of the above) ?

Copy link
Contributor

Choose a reason for hiding this comment

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

The framework behavior has indeed changed since 8_0_X. More specifically, since 9_0_0_pre1, all (End)Paths are processed concurrently (#15882) in non-deterministic order, such that the execution of modules in the Paths follow the order of data dependencies (i.e. hltScoutingCaloPacker is run only after all its input producers either have been run, or known not to be run because of the filtering). (although note that <= 11_1_0_pre3, only one module per Path could be run at a time; this changed for 11_1_0_pre4 onwards with #29024)

Earlier (i.e. in 8_0_X) framework processes the (End)Paths in the order specified by the Schedule. If the Schedule definition below has all the Paths of the input producers before the Path(s?) that contains hltScoutingCaloPacker, the job should behave as you'd expect (or how I understood what you'd expect). Note that I did not check the changes in the Schedule.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Okay, thanks for clarifying (my understanding was wrong).

In case it was unclear, the desired behaviour is that hltScoutingCaloPacker runs only after all the products it consumes ran (my doubt came from the fact that those products are created by Paths different from the one including hltScoutingCaloPacker).

To see if I understand

since 9_0_0_pre1, all (End)Paths are processed concurrently (#15882) in non-deterministic order, such that the execution of modules in the Paths follow the order of data dependencies (i.e. hltScoutingCaloPacker is run only after all its input producers either have been run, or known not to be run because of the filtering)

this (esp. "that the execution of modules in the Paths follow the order of data dependencies") means that this approach leads to the desired behaviour in those release cycles (e.g. #21297 for 10_0_X produces the desired behaviour).

Earlier (i.e. in 8_0_X) framework processes the (End)Paths in the order specified by the Schedule. If the Schedule definition below has all the Paths of the input producers before the Path(s?) that contains hltScoutingCaloPacker,

yes, this is the case (there is only 1 Path with hltScoutingCaloPacker)

the job should behave as you'd expect (or how I understood what you'd expect).

this means this PR happens to create the desired behaviour for 8_0_X.

Copy link
Contributor

Choose a reason for hiding this comment

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

To see if I understand

since 9_0_0_pre1, all (End)Paths are processed concurrently (#15882) in non-deterministic order, such that the execution of modules in the Paths follow the order of data dependencies (i.e. hltScoutingCaloPacker is run only after all its input producers either have been run, or known not to be run because of the filtering)

this (esp. "that the execution of modules in the Paths follow the order of data dependencies") means that this approach leads to the desired behaviour in those release cycles (e.g. #21297 for 10_0_X produces the desired behaviour).

Correct.

@missirol
Copy link
Contributor Author

missirol commented Feb 6, 2023

please test

@makortel
Copy link
Contributor

makortel commented Feb 6, 2023

What is your take on this? What options do you see?

I don't see any particular issues from core point of view (assuming the ordering of Paths in the Schedule makes sense). The EventContent changes, which means that only some releases in 8_0_X would produce RAWSIM files with Scouting data formats while the already existing releases would not produce such files (but such inconsistency is not really core concern).

Also regarding scouting data format compatibility we should be fine (well, we have 8_0_X-collected data with scouting data formats, right?)

@missirol
Copy link
Contributor Author

missirol commented Feb 6, 2023

Also regarding scouting data format compatibility we should be fine (well, we have 8_0_X-collected data with scouting data formats, right?)

(we do, e.g. /Scouting*/Run2016*/RAW)

@cmsbuild
Copy link
Contributor

cmsbuild commented Feb 6, 2023

+1

Summary: https://cmssdt.cern.ch/SDT/jenkins-artifacts/pull-request-integration/PR-0e77a0/30430/summary.html
COMMIT: 883e102
CMSSW: CMSSW_8_0_X_2023-02-05-0000/slc6_amd64_gcc530
User test area: For local testing, you can use /cvmfs/cms-ci.cern.ch/week1/cms-sw/cmssw/40708/30430/install.sh to create a dev area with all the needed externals and cmssw changes.

Comparison Summary

Summary:

  • You potentially added 10 lines to the logs
  • ROOTFileChecks: Some differences in event products or their sizes found
  • Reco comparison results: 3 differences found in the comparisons
  • DQMHistoTests: Total files compared: 15
  • DQMHistoTests: Total histograms compared: 1044635
  • DQMHistoTests: Total failures: 900
  • DQMHistoTests: Total nulls: 4
  • DQMHistoTests: Total successes: 1043623
  • DQMHistoTests: Total skipped: 108
  • DQMHistoTests: Total Missing objects: 0
  • DQMHistoSizes: Histogram memory added: -14 KiB( 14 files compared)
  • Checked 62 log files, 40 edm output root files, 15 DQM output files

@perrotta
Copy link
Contributor

perrotta commented Feb 7, 2023

If you consider this update too intrusive for such an old cycle, @cms-sw/hlt-l2 would be okay with not merging it (opening the PR makes it at least available to users).

This PR adds new collections on HLT MC. Provided it does not affect any other ones, I don't see a priori counterindications for merging it even in such an old release cysle.

@kpedro88
Copy link
Contributor

kpedro88 commented Feb 7, 2023

thanks @missirol !
(I think the main motivation to have this change merged into a release is for potential future central production. For private production, users could indeed merge this branch on their own, though in principle it's a little less overhead for private jobs not to have to send a tarball with rebuilt packages.)

@missirol
Copy link
Contributor Author

missirol commented Feb 7, 2023

Provided it does not affect any other ones, I don't see a priori counterindications for merging it even in such an old release cysle.

Okay, I also don't see a showstopper with merging this PR. I just want to make sure that it works as intended (to close the loop on the discussion in #40708 (comment)), then I can sign it if it works. I will do this before the end of the week.

@cmsbuild
Copy link
Contributor

cmsbuild commented Feb 9, 2023

Pull request #40708 was updated. @cmsbuild, @missirol, @Martin-Grunewald can you please check and sign again.

@missirol
Copy link
Contributor Author

missirol commented Feb 9, 2023

please test

The latest push applies the fix also to (the cff fragment of) the other frozen HLT menu in this cycle, i.e. menu "25ns10e33_v2" (I forgot to update that one when I opened the PR).

I have tested the output of [*] with this PR, and I can see that the Scouting collections are filled as expected (note that, since this is MC, most HLT objects used to build the Scouting collections are produced for every event, because of the Paths named MC_*). I think this agrees with the information in #40708 (comment), because the Paths with scouting "packers" are placed in the schedule after the Paths that produce the collections used as inputs to the Scouting packers (this can be seen in the hlt.py example in [*]).

[*] Command based on a UL16 MC workflow:

cmsDriver.py hlt \
 --eventcontent RAWSIM \
 --datatier GEN-SIM-RAW \
 --inputCommands 'keep *,drop *_*_BMTF_*,drop *PixelFEDChannel*_*_*_*' \
 --outputCommand 'keep *_mix_*_*,keep *_genPUProtons_*_*' \
 --customise_commands 'process.source.bypassVersionCheck = cms.untracked.bool(True)' \
 --customise Configuration/DataProcessing/Utils.addMonitoring \
 --era Run2_2016 --mc --conditions 80X_mcRun2_asymptotic_2016_TrancheIV_v6 --geometry DB:Extended \
 --step HLT:25ns15e33_v4 \
 --filein /store/mc/RunIISummer20UL16DIGIPremix/TTToSemiLeptonic_TuneCP5_13TeV-powheg-pythia8/GEN-SIM-DIGI/PUForTRKv2_TRKv2_106X_mcRun2_asymptotic_v13-v2/2530000/8D8AB13C-33FC-E445-93D1-195BEE140B49.root \
 --fileout file:hlt.root \
 --python_filename hlt.py \
 --nThreads 8 -n 100 --no_exec
cmsRun hlt.py &> hlt.log

@cmsbuild
Copy link
Contributor

cmsbuild commented Feb 9, 2023

+1

Summary: https://cmssdt.cern.ch/SDT/jenkins-artifacts/pull-request-integration/PR-0e77a0/30552/summary.html
COMMIT: 59defc3
CMSSW: CMSSW_8_0_X_2023-02-05-0000/slc6_amd64_gcc530
User test area: For local testing, you can use /cvmfs/cms-ci.cern.ch/week1/cms-sw/cmssw/40708/30552/install.sh to create a dev area with all the needed externals and cmssw changes.

Comparison Summary

Summary:

  • You potentially added 5 lines to the logs
  • ROOTFileChecks: Some differences in event products or their sizes found
  • Reco comparison results: 3 differences found in the comparisons
  • DQMHistoTests: Total files compared: 15
  • DQMHistoTests: Total histograms compared: 1044635
  • DQMHistoTests: Total failures: 854
  • DQMHistoTests: Total nulls: 4
  • DQMHistoTests: Total successes: 1043669
  • DQMHistoTests: Total skipped: 108
  • DQMHistoTests: Total Missing objects: 0
  • DQMHistoSizes: Histogram memory added: -14 KiB( 14 files compared)
  • Checked 62 log files, 40 edm output root files, 15 DQM output files

@missirol
Copy link
Contributor Author

please test

Rerunning the tests once more, to see if some of the DQM differences are reproducible.

@cmsbuild
Copy link
Contributor

+1

Summary: https://cmssdt.cern.ch/SDT/jenkins-artifacts/pull-request-integration/PR-0e77a0/30571/summary.html
COMMIT: 59defc3
CMSSW: CMSSW_8_0_X_2023-02-05-0000/slc6_amd64_gcc530
User test area: For local testing, you can use /cvmfs/cms-ci.cern.ch/week1/cms-sw/cmssw/40708/30571/install.sh to create a dev area with all the needed externals and cmssw changes.

Comparison Summary

Summary:

  • You potentially added 10 lines to the logs
  • ROOTFileChecks: Some differences in event products or their sizes found
  • Reco comparison results: 3 differences found in the comparisons
  • DQMHistoTests: Total files compared: 15
  • DQMHistoTests: Total histograms compared: 1044635
  • DQMHistoTests: Total failures: 861
  • DQMHistoTests: Total nulls: 4
  • DQMHistoTests: Total successes: 1043662
  • DQMHistoTests: Total skipped: 108
  • DQMHistoTests: Total Missing objects: 0
  • DQMHistoSizes: Histogram memory added: -14 KiB( 14 files compared)
  • Checked 62 log files, 40 edm output root files, 15 DQM output files

@missirol
Copy link
Contributor Author

The DQM comparisons show a few differences, but none of them can be attributed to this PR, as expected. Details below.

  • HcalHitsV/SimHitsValidationHcal: many 'failing comparisons' in wf 5.1, but those histograms are somewhat broken: they cannot be opened locally with a TBrowser, as one gets "Warning in TCanvas::ResizePad: Inf/NaN propagated to the pad. Check drawn objects.", and GetSumw2() returns nans. Clearly, this PR does not modify anything in the simulation step, so it cannot affect these histograms from SimHitsValidationHcal.

  • HLT/Higgs/PhotonJet/trigvsnvtx: 1 difference reported in many wfs, but the plot looks empty before and after this PR. Checking the ROOT files, one sees that trigvsnvtx is somewhat broken as GetEntries == 0 but GetBinContent(X,Y) == -nan; the nan likely comes from here. Unrelated to this PR.

  • HLT/Muon/Distributions shows 3 unexpected differences only in wf 1330.0. Checking the output of step2 from baseline and baseline+PR, I verified that the edm::TriggerResults, and the objects in trigger::TriggerEvent, are the same pre/post PR, as expected. I think the issue with the MonitorElements comes from a bug in HLTMuonPlotter (hltStep can become (size_t) -1) which leads to UB here. After fixing the bug locally, the DQM outputs look identical pre/post PR (only a quick check). The bottom line is that the HLT reconstruction is unchanged, as expected.

@missirol
Copy link
Contributor Author

+hlt

@cmsbuild
Copy link
Contributor

This pull request is fully signed and it will be integrated in one of the next CMSSW_8_0_X IBs (tests are also fine) and once validation in the development release cycle CMSSW_13_0_X is complete. This pull request will now be reviewed by the release team before it's merged. @perrotta, @dpiparo, @rappoccio (and backports should be raised in the release meeting by the corresponding L2)

@missirol
Copy link
Contributor Author

type bugfix

@cms-sw/orp-l2 , assuming this PR is merged and IBs are okay, HLT would like to have a new 8_0_X release (and 8_0_X_UL too ?) in the next days.

@cms-sw/pdmv-l2 , the next 8_0_X release will contain this fix for 2016UL MC. This fix only concerns the HLT-Scouting outputs (nothing else changes). Is it sufficient to notify you here, or what else should be done to ensure that this next 8_0_X release is the one used for the HLT step of future 2016-UL samples?

@kskovpen
Copy link
Contributor

type bugfix

@cms-sw/orp-l2 , assuming this PR is merged and IBs are okay, HLT would like to have a new 8_0_X release (and 8_0_X_UL too ?) in the next days.

@cms-sw/pdmv-l2 , the next 8_0_X release will contain this fix for 2016UL MC. This fix only concerns the HLT-Scouting outputs (nothing else changes). Is it sufficient to notify you here, or what else should be done to ensure that this next 8_0_X release is the one used for the HLT step of future 2016-UL samples?

Thanks @missirol! PdmV took a note of this. To mitigate the risk of being forgotten, please don't hesitate to drop us a message when the release is built.

@perrotta
Copy link
Contributor

+1

  • ScoutingCaloOutput and ScoutingPFOutput, togethecr with the related products, are added to the MC outputs
  • All other outputs do not get touched: no objections to the backport

@cmsbuild cmsbuild merged commit 12e2442 into cms-sw:CMSSW_8_0_X Feb 13, 2023
@missirol missirol deleted the devel_addScoutingOutputs_80X branch February 13, 2023 14:14
@missirol
Copy link
Contributor Author

@cms-sw/orp-l2 , thanks for building the new releases.

To mitigate the risk of being forgotten, please don't hesitate to drop us a message when the release is built.

@kskovpen , the new 8_0_X releases are available (I also notified PdmV L2s by email):

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants