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

Fix ordering of egamma HLT modules #44592

Merged
merged 1 commit into from
Apr 4, 2024

Conversation

swagata87
Copy link
Contributor

PR description:

Recently phase2 HLT menu abandoned ConditionalTask and adopted a fully Sequence-based approach (via #44025). It was noticed that, in this process, ordering of some egamma modules got shuffled unintentionally. That was partly fixed by #44523.
This is a followup PR to further fix the remaining issues, and implement a proper ordering of modules in egamma paths. The ordering is important to keep timing under control. The idea is to run less expensive sequences in the beginning, followed by relevant filters and then then move on to run more expensive sequences. This is what has always been done in egamma, in phase1 and also in phase2 HLT.

As an example, in HLT_Ele32 path (which has the highest rate among all egamma paths), the following ordering is reintroduced via this PR:

Run L1 sequence
L1 filter    
ECAL and HGCAL local reco + clustering + superclustering
Cut on supercluster ET
Produce ECAL sigmaIEtaIEta, and cut on it 
Produce HGCAL ID variables,  and cut on them  
HCAL local reco
FastJet Sequence (needed for rho correction)
Produce H/E in barrel, cut on that
Produce ECAL isolation, cut on that
Produce HGCAL isolation, cut on that
Run PF clustering in HCAL
Produce HCAL PF cluster based isolation, and cut on that
Run Pixel Match Sequence
Pixel matching related filters
GsfElectron Sequence
Cut on GSF related variables
Produce Track isolation using L1 tracks, and cut on that
Run general tracking
Produce track isolation using HLT tracks, and cut on that

PR validation:

Phase2 menu runs.

@cmsbuild
Copy link
Contributor

cmsbuild commented Apr 2, 2024

cms-bot internal usage

@cmsbuild
Copy link
Contributor

cmsbuild commented Apr 2, 2024

+code-checks

Logs: https://cmssdt.cern.ch/SDT/code-checks/cms-sw-PR-44592/39766

  • This PR adds an extra 24KB to repository

@cmsbuild
Copy link
Contributor

cmsbuild commented Apr 2, 2024

A new Pull Request was created by @swagata87 for master.

It involves the following packages:

  • HLTrigger/Configuration (hlt)

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

cms-bot commands are listed here

@mmusich
Copy link
Contributor

mmusich commented Apr 2, 2024

The ordering is important to keep timing under control

@swagata87 do you have timing measurements before / after ?

@mmusich
Copy link
Contributor

mmusich commented Apr 2, 2024

type egamma, bug-fix

@swagata87
Copy link
Contributor Author

no I haven't done any explicit measurement for timing.

@mmusich
Copy link
Contributor

mmusich commented Apr 2, 2024

@cmsbuild, please test

@cmsbuild
Copy link
Contributor

cmsbuild commented Apr 2, 2024

+1

Summary: https://cmssdt.cern.ch/SDT/jenkins-artifacts/pull-request-integration/PR-8018ad/38559/summary.html
COMMIT: 6da50ec
CMSSW: CMSSW_14_1_X_2024-04-02-1100/el8_amd64_gcc12
User test area: For local testing, you can use /cvmfs/cms-ci.cern.ch/week1/cms-sw/cmssw/44592/38559/install.sh to create a dev area with all the needed externals and cmssw changes.

Comparison Summary

Summary:

  • You potentially removed 1 lines from the logs
  • Reco comparison results: 44 differences found in the comparisons
  • DQMHistoTests: Total files compared: 48
  • DQMHistoTests: Total histograms compared: 3297469
  • DQMHistoTests: Total failures: 3
  • DQMHistoTests: Total nulls: 0
  • DQMHistoTests: Total successes: 3297446
  • DQMHistoTests: Total skipped: 20
  • DQMHistoTests: Total Missing objects: 0
  • DQMHistoSizes: Histogram memory added: 0.0 KiB( 47 files compared)
  • Checked 202 log files, 165 edm output root files, 48 DQM output files
  • TriggerResults: found differences in 5 / 46 workflows

@swagata87
Copy link
Contributor Author

About timing measurements, it's difficult to do a meaningful measurement using the RelVal MC samples of Phase2. I found only PU=0 relVal samples. Anyways, I still ran it before and after this PR and got following results. I ran the timing menu in lxplus809 machine. I ran on 10K events, which were a mix of MinBias, TTBar-inclusive and WtoLNu, and they were all PU=0 samples.
Before PR: EGamma 3 ms
After PR: EGamma 2.9 ms
Total time for full menu was in the ballpark of 210-211 ms, so things run quite fast with 0 PU sample, and it's not a useful way to draw conclusion about phase2 menu where the main timing challenge comes from high PU.

@Sam-Harper
Copy link
Contributor

Swagata. thank you for doing this!

On the timing, it probably wont change anything when the Ele5 open path is being run but it should dramatically decrease the timing for the actual paths. So important to have if we do timing studies.

@swagata87
Copy link
Contributor Author

Thank you for looking into this PR, Sam.
So, although I fixed the ordering of Ele5 open path also, while doing timing study I did not run any open path. In fact, we now have a realistic menu for timing study, which is this: https://github.com/cms-sw/cmssw/blob/master/HLTrigger/Configuration/python/HLT_75e33_timing_cff.py, this is what I ran.
So, it seems that timing remain unchanged before/after this PR, mainly because of 0 PU sample. After all, the expensive sequences like pixel-matching are expensive when there is atleast some PU. W/o any PU, its just too clean an environment to see the gain coming from proper ordering.

@Sam-Harper
Copy link
Contributor

I was more thinking "CPU reduction for general MC" no effect, actual timing studies large effect.

And it makes sense about the 0PU. I agree

@Martin-Grunewald
Copy link
Contributor

+1

@cmsbuild
Copy link
Contributor

cmsbuild commented Apr 4, 2024

This pull request is fully signed and it will be integrated in one of the next master IBs (tests are also fine). This pull request will now be reviewed by the release team before it's merged. @antoniovilela, @sextonkennedy, @rappoccio (and backports should be raised in the release meeting by the corresponding L2)

@antoniovilela
Copy link
Contributor

+1

@cmsbuild cmsbuild merged commit e46c99d into cms-sw:master Apr 4, 2024
11 checks passed
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