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

Switch to heavy-ion jets in HI miniAOD #30898

Merged
merged 20 commits into from Oct 3, 2020
Merged

Conversation

mandrenguyen
Copy link
Contributor

PR description:

Replaces pp jets in HI miniAOD with HI jets, using the Cs UE subtraction algorithm (as updated in #30057 ).

PR validation:

Tested with workflows 158.01 and 140.5611

if this PR is a backport please specify the original PR and why you need to backport that PR:

Before submitting your pull requests, make sure you followed this checklist:

@cmsbuild
Copy link
Contributor

The code-checks are being triggered in jenkins.

@cmsbuild
Copy link
Contributor

+code-checks

Logs: https://cmssdt.cern.ch/SDT/code-checks/cms-sw-PR-30898/17291

@cmsbuild
Copy link
Contributor

A new Pull Request was created by @mandrenguyen (Matthew Nguyen) for master.

It involves the following packages:

PhysicsTools/PatAlgos
RecoHI/HiTracking

@perrotta, @jpata, @cmsbuild, @santocch, @slava77 can you please review it and eventually sign? Thanks.
@rappoccio, @gouskos, @schoef, @emilbols, @echapon, @peruzzim, @seemasharmafnal, @mmarionncern, @ahinzmann, @smoortga, @dgulhan, @jdolen, @ferencek, @yetkinyilmaz, @jdamgov, @nhanvtran, @gkasieczka, @hatakeyamak, @mariadalfonso, @clelange, @riga, @jazzitup, @JyothsnaKomaragiri, @mverzett, @yenjie, @kurtejung, @gpetruc, @andrzejnovak this is something you requested to watch as well.
@silviodonato, @dpiparo, @qliphy you are the release manager for this.

cms-bot commands are listed here

@slava77
Copy link
Contributor

slava77 commented Jul 24, 2020

@cmsbuild please test

@cmsbuild
Copy link
Contributor

cmsbuild commented Jul 24, 2020

The tests are being triggered in jenkins.

@cmsbuild
Copy link
Contributor

+1
Tested at: 9aaff11
https://cmssdt.cern.ch/SDT/jenkins-artifacts/pull-request-integration/PR-cc9d6c/8273/summary.html
CMSSW: CMSSW_11_2_X_2020-07-23-2300
SCRAM_ARCH: slc7_amd64_gcc820

@cmsbuild
Copy link
Contributor

Comparison job queued.

@cmsbuild
Copy link
Contributor

Comparison is ready
https://cmssdt.cern.ch/SDT/jenkins-artifacts/pull-request-integration/PR-cc9d6c/8273/summary.html

Comparison Summary:

  • No significant changes to the logs found
  • ROOTFileChecks: Some differences in event products or their sizes found
  • Reco comparison results: 131 differences found in the comparisons
  • DQMHistoTests: Total files compared: 34
  • DQMHistoTests: Total histograms compared: 2526188
  • DQMHistoTests: Total failures: 797
  • DQMHistoTests: Total nulls: 0
  • DQMHistoTests: Total successes: 2525344
  • DQMHistoTests: Total skipped: 47
  • DQMHistoTests: Total Missing objects: 0
  • DQMHistoSizes: Histogram memory added: 0.0 KiB( 33 files compared)
  • Checked 144 log files, 17 edm output root files, 34 DQM output files

@cmsbuild
Copy link
Contributor

cmsbuild commented Oct 1, 2020

Comparison is ready
https://cmssdt.cern.ch/SDT/jenkins-artifacts/pull-request-integration/PR-cc9d6c/9694/summary.html

Comparison Summary:

  • No significant changes to the logs found
  • ROOTFileChecks: Some differences in event products or their sizes found
  • Reco comparison results: 515 differences found in the comparisons
  • DQMHistoTests: Total files compared: 35
  • DQMHistoTests: Total histograms compared: 2542225
  • DQMHistoTests: Total failures: 970
  • DQMHistoTests: Total nulls: 0
  • DQMHistoTests: Total successes: 2541233
  • DQMHistoTests: Total skipped: 22
  • DQMHistoTests: Total Missing objects: 0
  • DQMHistoSizes: Histogram memory added: 0.0 KiB( 34 files compared)
  • Checked 149 log files, 22 edm output root files, 35 DQM output files

Copy link
Contributor

@slava77 slava77 left a comment

Choose a reason for hiding this comment

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

I was looking at the results in wf 140.5611. Most of the changes are as expected, however there are still some top level oddities or points of concern

  • slimmedJets_tagInfos take up 25% of the file size ( increases the total file size by 18%) ; are they really worth it? Note that the cost in less busy events of wf 158.01 is only 4% of the disk size. Some preselection may be useful, likely for a follow up PR.
  • puppi is still running ; see the comment inline for patPhotons addPuppiIsolation
  • isolatedTracks module is a bit heavy (9% CPU, second slowest module, after akCs4PFJets); with only 1% disk use though. Note that in 158.01 it's pretty low on the list (0.4% of CPU). So, it's probably more in the land of general CPU optimization with a bit more interest/motivation for HI needs.

Additionally, in wf 158.0 the pfJetsEI are now filled (these run but are not written in AOD). The reason is in the ak4PFJets (see a comment for that).

I think that most of these if not more can be followed up in #31647

@@ -120,20 +124,25 @@ def miniAOD_customizeCommon(process):
from Configuration.Eras.Modifier_phase2_muon_cff import phase2_muon
Copy link
Contributor

Choose a reason for hiding this comment

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

around line 93 I suggest to add

_hiGeneral.toModify(process.patPhotons, addPuppiIsolation = False)

it looks like the only reason why "puppi" module is still running

pp_on_AA_2018.toModify(ak4PFJets,src = "pfNoPileUpJMEHI", inputEtMin = 9999)
pp_on_AA_2018.toModify(ak4PFJetsCHS,src = "pfNoPileUpJMEHI", inputEtMin = 9999)
from Configuration.Eras.Modifier_pp_on_PbPb_run3_cff import pp_on_PbPb_run3
(pp_on_AA_2018 | pp_on_PbPb_run3).toModify(ak4PFJets, src = "pfEmptyCollection")
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
(pp_on_AA_2018 | pp_on_PbPb_run3).toModify(ak4PFJets, src = "pfEmptyCollection")
(pp_on_AA_2018 | pp_on_PbPb_run3).toModify(ak4PFJets, src = "pfEmptyCollection", inputEtMin = 9999)

I think that the threshold should still be applied; better yet, a more thorough cleanup is needed in CommonTools/ParticleFlow/python/EITopPAG_cff.py

@slava77
Copy link
Contributor

slava77 commented Oct 2, 2020

+1

for #30898 0fcdc06

  • default jets for HI are switched to akCs4PFJets, PAT is adapted to use these jets in the default patJets; miniAOD (miniAOD_tools.py) is updated/refactored to not configure standard pp parts that are not needed in HI (this approach can be extended to other possible special miniAOD workflows)
  • jenkins tests pass and comparisons with the baseline show differences only in the HI workflows using the modified pp reco/miniAOD.

A few items that somewhat extend beyond the scope of just having the jets configured for miniAOD can be followed up in #31647

@mandrenguyen
Copy link
Contributor Author

@silviodonato Can you merge this one, please? There are other PRs I need to make on top of it that are needed for the HI-miniAOD in 11_2_X.

@silviodonato
Copy link
Contributor

merge

@cmsbuild cmsbuild merged commit 9691783 into cms-sw:master Oct 3, 2020
@santocch
Copy link

santocch commented Oct 4, 2020

+1

@cmsbuild
Copy link
Contributor

cmsbuild commented Oct 4, 2020

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 be automatically merged.

@makortel
Copy link
Contributor

makortel commented Oct 5, 2020

Apparently this PR broke workflows 140.0, 145.0, and 150.0 with

Traceback (most recent call last):
  File "/cvmfs/cms-ib.cern.ch/nweek-02649/slc7_amd64_gcc820/cms/cmssw/CMSSW_11_2_X_2020-10-04-0000/bin/slc7_amd64_gcc820/cmsDriver.py", line 56, in <module>
    run()
  File "/cvmfs/cms-ib.cern.ch/nweek-02649/slc7_amd64_gcc820/cms/cmssw/CMSSW_11_2_X_2020-10-04-0000/bin/slc7_amd64_gcc820/cmsDriver.py", line 28, in run
    configBuilder.prepare()
  File "/cvmfs/cms-ib.cern.ch/week1/slc7_amd64_gcc820/cms/cmssw-patch/CMSSW_11_2_X_2020-10-04-2300/python/Configuration/Applications/ConfigBuilder.py", line 2135, in prepare
    self.addStandardSequences()
  File "/cvmfs/cms-ib.cern.ch/week1/slc7_amd64_gcc820/cms/cmssw-patch/CMSSW_11_2_X_2020-10-04-2300/python/Configuration/Applications/ConfigBuilder.py", line 757, in addStandardSequences
    getattr(self,"prepare_"+stepName)(sequence = getattr(self,stepName+"DefaultSeq"))
  File "/cvmfs/cms-ib.cern.ch/week1/slc7_amd64_gcc820/cms/cmssw-patch/CMSSW_11_2_X_2020-10-04-2300/python/Configuration/Applications/ConfigBuilder.py", line 1773, in prepare_VALIDATION
    self.loadDefaultOrSpecifiedCFF(sequence,self.VALIDATIONDefaultCFF)
  File "/cvmfs/cms-ib.cern.ch/week1/slc7_amd64_gcc820/cms/cmssw-patch/CMSSW_11_2_X_2020-10-04-2300/python/Configuration/Applications/ConfigBuilder.py", line 1198, in loadDefaultOrSpecifiedCFF
    l=self.loadAndRemember(defaultCFF)
  File "/cvmfs/cms-ib.cern.ch/week1/slc7_amd64_gcc820/cms/cmssw-patch/CMSSW_11_2_X_2020-10-04-2300/python/Configuration/Applications/ConfigBuilder.py", line 325, in loadAndRemember
    self.process.load(includeFile)
  File "/cvmfs/cms-ib.cern.ch/week1/slc7_amd64_gcc820/cms/cmssw-patch/CMSSW_11_2_X_2020-10-04-2300/python/FWCore/ParameterSet/Config.py", line 684, in load
    self.extend(sys.modules[moduleName])
  File "/cvmfs/cms-ib.cern.ch/week1/slc7_amd64_gcc820/cms/cmssw-patch/CMSSW_11_2_X_2020-10-04-2300/python/FWCore/ParameterSet/Config.py", line 731, in extend
    self.__setattr__(name, task)
  File "/cvmfs/cms-ib.cern.ch/week1/slc7_amd64_gcc820/cms/cmssw-patch/CMSSW_11_2_X_2020-10-04-2300/python/FWCore/ParameterSet/Config.py", line 507, in __setattr__
    newValue._place(name,self)
  File "/cvmfs/cms-ib.cern.ch/week1/slc7_amd64_gcc820/cms/cmssw-patch/CMSSW_11_2_X_2020-10-04-2300/python/FWCore/ParameterSet/SequenceTypes.py", line 1441, in _place
    proc._placeTask(name,self)
  File "/cvmfs/cms-ib.cern.ch/week1/slc7_amd64_gcc820/cms/cmssw-patch/CMSSW_11_2_X_2020-10-04-2300/python/FWCore/ParameterSet/Config.py", line 650, in _placeTask
    self._validateTask(task, name)
  File "/cvmfs/cms-ib.cern.ch/week1/slc7_amd64_gcc820/cms/cmssw-patch/CMSSW_11_2_X_2020-10-04-2300/python/FWCore/ParameterSet/Config.py", line 884, in _validateTask
    raise RuntimeError("An entry in task " + label + ' has not been attached to the process')
RuntimeError: An entry in task recoJetsHIpostAODTask has not been attached to the process

see e.g. https://cmssdt.cern.ch/SDT/cgi-bin/logreader/slc7_amd64_gcc820/CMSSW_11_2_X_2020-10-04-2300/pyRelValMatrixLogs/run/140.0_HydjetQ_B12_5020GeV_2011+HydjetQ_B12_5020GeV_2011+DIGIHI2011+RECOHI2011+HARVESTHI2011/step3_HydjetQ_B12_5020GeV_2011+HydjetQ_B12_5020GeV_2011+DIGIHI2011+RECOHI2011+HARVESTHI2011.log#/

@mandrenguyen
Copy link
Contributor Author

@makortel I tracked down the issue, which is a naming conflict with a simple fix.
Should I make a new PR, since this one is already merged?

@makortel
Copy link
Contributor

makortel commented Oct 5, 2020

Should I make a new PR, since this one is already merged?

Yes, please make a new PR.

@makortel
Copy link
Contributor

makortel commented Oct 5, 2020

This PR appears to also cause many HI workflows to crash (see #31670). I hope the fix will fix those as well.

@mandrenguyen
Copy link
Contributor Author

@makortel Ah, there's another issue affecting 158.1, but I can see the fix for that as well.
I will make a combined PR to fix both of them. ETA is late tonight CERN time though.

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.

None yet