Fix prodlike workflow to use DIGI instead of DIGI:pdigi_valid #33847

srimanob · 2021-05-26T12:58:31Z

PR description:

Currently, Phase-2 prod-like workflow uses DIGI:pdigi_valid instead of DIGI. This PR is to make a clear cut between production and relvals with DIGI and DIGI:pdigi_valid respectively. To run in production mode, we need to adapt digitizers_cfi.py to include Calo MCTruth for HGCAL in DIGI.

No change is expected for relvals as pdigi_valid will be used as before. The change is only the size of events in production mode. With PU200 ttbar, it can save more than 30MB/events as MergeTrackTruth will not be stored.

PR validation:

Get proper cmsDriver(s) when use
runTheMatrix.py --what upgrade -l 23234.0,23234.21,23434.21,23434.9921 --dryRun

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

Not a backport, and no need for backport

cmsbuild · 2021-05-26T13:05:22Z

+code-checks

Logs: https://cmssdt.cern.ch/SDT/code-checks/cms-sw-PR-33847/22869

This PR adds an extra 20KB to repository
There are other open Pull requests which might conflict with changes you have proposed:
- File Configuration/PyReleaseValidation/python/upgradeWorkflowComponents.py modified in PR(s): [DD4hep] start on geometry XML payload producer #33548, Add run3 tracking low pu era #33532, Update MkFit to support multiple iterations #33802

cmsbuild · 2021-05-26T13:05:44Z

A new Pull Request was created by @srimanob (Phat Srimanobhas) for master.

It involves the following packages:

Configuration/PyReleaseValidation

@jordan-martins, @chayanit, @wajidalikhan, @kpedro88, @cmsbuild, @srimanob can you please review it and eventually sign? Thanks.
@makortel, @Martin-Grunewald, @fabiocos, @slomeo 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

srimanob · 2021-05-26T13:08:52Z

Please test

kpedro88 · 2021-05-26T13:18:30Z

@srimanob this was a deliberate choice to match the Phase 2 production campaigns which also use DIGI:pdigi_valid, e.g. https://twiki.cern.ch/twiki/bin/view/CMS/Phase2HLTTDRWinter20DR

kpedro88 · 2021-05-26T13:20:13Z

Configuration/PyReleaseValidation/python/upgradeWorkflowComponents.py

+            d = merge([stepDict[self.getStepName(step)][k]])
+            tmpsteps = []
+            for s in d["-s"].split(","):
+                if s == "DIGI:pdigi_valid" in s:


I don't think this condition is correct

Hmm.. Could you please guide? I get -s DIGI,DATAMIX,L1TrackTrigger,L1,DIGI2RAW,HLT:@fake2 as expected.

Either if s == "DIGI:pdigi_valid" or if "DIGI:pdigi_valid" in s would potentially make sense. Right now, this is comparing a string to a bool at some point.

Thanks, Fixed.

srimanob · 2021-05-26T13:24:48Z

@srimanob this was a deliberate choice to match the Phase 2 production campaigns which also use DIGI:pdigi_valid, e.g. https://twiki.cern.ch/twiki/bin/view/CMS/Phase2HLTTDRWinter20DR

I understand that was a (my) mistake in the campaign setting. I think we should keep the proper cmsDriver, and no clue on the need of pdigi_valid in the production (with MC truth information). Is it really need for Phase-2 currently?

kpedro88 · 2021-05-26T13:32:40Z

There are a number of Phase 2 customizations in theDigitizersValid, and I'm not sure if we can go without these yet:

cmssw/SimGeneral/MixingModule/python/digitizers_cfi.py

Lines 102 to 127 in 6d2f660

    
           theDigitizersValid = cms.PSet(theDigitizers) 
        
           theDigitizers.mergedtruth.select.signalOnlyTP = True 
        
           from Configuration.ProcessModifiers.run3_ecalclustering_cff import run3_ecalclustering 
        
           (run3_ecalclustering | phase2_hgcal).toModify( theDigitizersValid, 
        
                                  calotruth = cms.PSet( caloParticles ) ) # Doesn't HGCal need these also without validation? 
        
           (premix_stage2 & phase2_hgcal).toModify(theDigitizersValid, calotruth = dict(premixStage1 = True)) 
        
           phase2_timing.toModify( theDigitizersValid.mergedtruth, 
        
                                   createInitialVertexCollection = cms.bool(True) ) 
        
           from Configuration.ProcessModifiers.premix_stage1_cff import premix_stage1 
        
           def _customizePremixStage1(mod): 
        
               # To avoid this if-else structure we'd need an "_InverseModifier" 
        
               # to customize pixel/strip for everything else than fastSim. 
        
               if hasattr(mod, "pixel"): 
        
                   if hasattr(mod.pixel, "AlgorithmCommon"): 
        
                       mod.pixel.AlgorithmCommon.makeDigiSimLinks = True 
        
                   else: 
        
                       mod.pixel.makeDigiSimLinks = True 
        
               if hasattr(mod, "strip"): 
        
                   mod.strip.makeDigiSimLinks = True 
        
               mod.mergedtruth.select.signalOnlyTP = False 
        
           premix_stage1.toModify(theDigitizersValid, _customizePremixStage1)

srimanob · 2021-05-26T13:37:23Z

There are a number of Phase 2 customizations in theDigitizersValid, and I'm not sure if we can go without these yet:

cmssw/SimGeneral/MixingModule/python/digitizers_cfi.py

Lines 102 to 127 in 6d2f660

theDigitizersValid = cms.PSet(theDigitizers)

theDigitizers.mergedtruth.select.signalOnlyTP = True

from Configuration.ProcessModifiers.run3_ecalclustering_cff import run3_ecalclustering

(run3_ecalclustering | phase2_hgcal).toModify( theDigitizersValid,

calotruth = cms.PSet( caloParticles ) ) # Doesn't HGCal need these also without validation?

(premix_stage2 & phase2_hgcal).toModify(theDigitizersValid, calotruth = dict(premixStage1 = True))

phase2_timing.toModify( theDigitizersValid.mergedtruth,

createInitialVertexCollection = cms.bool(True) )

from Configuration.ProcessModifiers.premix_stage1_cff import premix_stage1

def _customizePremixStage1(mod):

# To avoid this if-else structure we'd need an "_InverseModifier"

# to customize pixel/strip for everything else than fastSim.

if hasattr(mod, "pixel"):

if hasattr(mod.pixel, "AlgorithmCommon"):

mod.pixel.AlgorithmCommon.makeDigiSimLinks = True

else:

mod.pixel.makeDigiSimLinks = True

if hasattr(mod, "strip"):

mod.strip.makeDigiSimLinks = True

mod.mergedtruth.select.signalOnlyTP = False

premix_stage1.toModify(theDigitizersValid, _customizePremixStage1)

OK, thanks. I will look into it.

cmsbuild · 2021-05-26T14:21:05Z

-1

Failed Tests: RelVals
Summary: https://cmssdt.cern.ch/SDT/jenkins-artifacts/pull-request-integration/PR-61760c/15321/summary.html
COMMIT: c84e36f
CMSSW: CMSSW_12_0_X_2021-05-26-1100/slc7_amd64_gcc900
User test area: For local testing, you can use /cvmfs/cms-ci.cern.ch/week0/cms-sw/cmssw/33847/15321/install.sh to create a dev area with all the needed externals and cmssw changes.

RelVals

cmsbuild · 2021-05-29T15:33:29Z

+1

Summary: https://cmssdt.cern.ch/SDT/jenkins-artifacts/pull-request-integration/PR-61760c/15411/summary.html
COMMIT: d1a025c
CMSSW: CMSSW_12_0_X_2021-05-28-2300/slc7_amd64_gcc900
User test area: For local testing, you can use /cvmfs/cms-ci.cern.ch/week0/cms-sw/cmssw/33847/15411/install.sh to create a dev area with all the needed externals and cmssw changes.

Comparison Summary

@slava77 comparisons for the following workflows were not done due to missing matrix map:

/data/cmsbld/jenkins/workspace/compare-root-files-short-matrix/data/PR-61760c/23234.21_TTbar_14TeV+2026D49_ProdLike+TTbar_14TeV_TuneCP5_GenSimHLBeamSpot14+DigiTrigger+RecoGlobal+MiniAOD
/data/cmsbld/jenkins/workspace/compare-root-files-short-matrix/data/PR-61760c/23434.9921_TTbar_14TeV+2026D49PU_PMXS1S2ProdLike+TTbar_14TeV_TuneCP5_GenSimHLBeamSpot14+PREMIX_PremixHLBeamSpot14PU+DigiTriggerPU+RecoGlobalPU+MiniAODPU

Summary:

No significant changes to the logs found
Reco comparison results: 10 differences found in the comparisons
DQMHistoTests: Total files compared: 37
DQMHistoTests: Total histograms compared: 2650486
DQMHistoTests: Total failures: 18
DQMHistoTests: Total nulls: 1
DQMHistoTests: Total successes: 2650445
DQMHistoTests: Total skipped: 22
DQMHistoTests: Total Missing objects: 0
DQMHistoSizes: Histogram memory added: -0.004 KiB( 36 files compared)
DQMHistoSizes: changed ( 312.0 ): -0.004 KiB MessageLogger/Warnings
Checked 155 log files, 37 edm output root files, 37 DQM output files
TriggerResults: no differences found

civanch · 2021-05-30T10:19:02Z

+1

I think, that saving of MC truth volume for HGCal is necessary.

srimanob · 2021-05-30T10:28:17Z

+Upgrade

Plan to backport to 11_3.

qliphy · 2021-05-31T03:11:01Z

@cms-sw/pdmv-l2 do you have any comment?

chayanit · 2021-05-31T03:40:30Z

+1

cmsbuild · 2021-05-31T03:40:54Z

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. @silviodonato, @dpiparo, @qliphy (and backports should be raised in the release meeting by the corresponding L2)

qliphy · 2021-05-31T04:36:12Z

+1

kpedro88 · 2021-06-01T14:00:25Z

SimGeneral/MixingModule/python/digitizers_cfi.py

@@ -104,10 +106,9 @@

 from Configuration.ProcessModifiers.run3_ecalclustering_cff import run3_ecalclustering
 (run3_ecalclustering | phase2_hgcal).toModify( theDigitizersValid,


this could just be run3_ecalclustering now (phase2_hgcal modification is now redundant)

kpedro88 · 2021-06-01T14:01:21Z

SimGeneral/MixingModule/python/digitizers_cfi.py

@@ -104,10 +106,9 @@

 from Configuration.ProcessModifiers.run3_ecalclustering_cff import run3_ecalclustering
 (run3_ecalclustering | phase2_hgcal).toModify( theDigitizersValid,
-                       calotruth = cms.PSet( caloParticles ) ) # Doesn't HGCal need these also without validation?
+                       calotruth = cms.PSet( caloParticles ) )
 (premix_stage2 & phase2_hgcal).toModify(theDigitizersValid, calotruth = dict(premixStage1 = True))


this line is also now redundant with the above changes

kpedro88 · 2021-06-01T14:04:06Z

SimGeneral/MixingModule/python/digitizers_cfi.py

 (premix_stage2 & phase2_hgcal).toModify(theDigitizersValid, calotruth = dict(premixStage1 = True))

-
 phase2_timing.toModify( theDigitizersValid.mergedtruth,


should check with @cms-sw/mtd-dpg-l2 if this behavior is needed for non-pdigi_valid workflow

srimanob · 2021-06-01T15:15:23Z

Thanks @kpedro88
I will make a fix PR on top of it. It should not affect production. Will wait for feedback from @cms-sw/mtd-dpg-l2 first if in production mode, will they need MC Truth.

fabiocos · 2021-06-03T07:55:05Z

@srimanob @kpedro88 MC Truth is needed for vertex validation, so for any study concerning quality of vertices its presence should be welcome. We are not normally running these studies on large production samples to my knowledge, for smaller test production I believe one should define the target of the test to decide.

srimanob · 2021-06-03T09:41:21Z

@srimanob @kpedro88 MC Truth is needed for vertex validation, so for any study concerning quality of vertices its presence should be welcome. We are not normally running these studies on large production samples to my knowledge, for smaller test production I believe one should define the target of the test to decide.

Thanks @fabiocos, I propose to keep it for pdigi_valid only, as it is, then.

Use DIGI instead of DIGI:pdigi_valid in prodlike wfs

c84e36f

cmsbuild added this to the CMSSW_12_0_X milestone May 26, 2021

cmsbuild added code-checks-pending orp-pending pdmv-pending pending-signatures tests-pending upgrade-pending labels May 26, 2021

cmsbuild added code-checks-approved and removed code-checks-pending labels May 26, 2021

cmsbuild added tests-started and removed tests-pending labels May 26, 2021

kpedro88 reviewed May 26, 2021

View reviewed changes

cmsbuild added tests-rejected and removed tests-started labels May 26, 2021

update digitizer for HGCAL in production mode

7259b4d

cmsbuild added code-checks-pending simulation-pending tests-pending and removed code-checks-approved tests-rejected labels May 29, 2021

fix string comparison

d1a025c

cmsbuild added tests-started and removed tests-approved labels May 29, 2021

cmsbuild added tests-approved and removed tests-started labels May 29, 2021

cmsbuild added simulation-approved and removed simulation-pending labels May 30, 2021

cmsbuild added upgrade-approved and removed upgrade-pending labels May 30, 2021

srimanob mentioned this pull request May 30, 2021

[Backport 11_3] Fix prodlike workflow to use DIGI instead of DIGI:pdigi_valid #33895

Merged

cmsbuild added fully-signed pdmv-approved and removed pdmv-pending pending-signatures labels May 31, 2021

cmsbuild added orp-approved and removed orp-pending labels May 31, 2021

cmsbuild merged commit f77cffe into cms-sw:master May 31, 2021

This was referenced May 31, 2021

Upgrade pythia8 to 305 cms-sw/cmsdist#6948

Closed

Removed unnecessary includes in CalibFormats/*Objects #33913

Merged

kpedro88 reviewed Jun 1, 2021

View reviewed changes

srimanob mentioned this pull request Jun 3, 2021

Cleaning digitizer config file #33963

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fix prodlike workflow to use DIGI instead of DIGI:pdigi_valid #33847

Fix prodlike workflow to use DIGI instead of DIGI:pdigi_valid #33847

srimanob commented May 26, 2021 •

edited

cmsbuild commented May 26, 2021

cmsbuild commented May 26, 2021

srimanob commented May 26, 2021

kpedro88 commented May 26, 2021

kpedro88 May 26, 2021

srimanob May 26, 2021

kpedro88 May 26, 2021

srimanob May 29, 2021

srimanob commented May 26, 2021 •

edited

kpedro88 commented May 26, 2021

srimanob commented May 26, 2021

cmsbuild commented May 26, 2021

cmsbuild commented May 29, 2021

civanch commented May 30, 2021

srimanob commented May 30, 2021

qliphy commented May 31, 2021

chayanit commented May 31, 2021

cmsbuild commented May 31, 2021

qliphy commented May 31, 2021

kpedro88 Jun 1, 2021

kpedro88 Jun 1, 2021

kpedro88 Jun 1, 2021

srimanob commented Jun 1, 2021

fabiocos commented Jun 3, 2021

srimanob commented Jun 3, 2021

		@@ -104,10 +106,9 @@

		from Configuration.ProcessModifiers.run3_ecalclustering_cff import run3_ecalclustering
		(run3_ecalclustering \| phase2_hgcal).toModify( theDigitizersValid,

		(premix_stage2 & phase2_hgcal).toModify(theDigitizersValid, calotruth = dict(premixStage1 = True))


		phase2_timing.toModify( theDigitizersValid.mergedtruth,

Fix prodlike workflow to use DIGI instead of DIGI:pdigi_valid #33847

Fix prodlike workflow to use DIGI instead of DIGI:pdigi_valid #33847

Conversation

srimanob commented May 26, 2021 • edited

PR description:

PR validation:

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

cmsbuild commented May 26, 2021

cmsbuild commented May 26, 2021

srimanob commented May 26, 2021

kpedro88 commented May 26, 2021

kpedro88 May 26, 2021

Choose a reason for hiding this comment

srimanob May 26, 2021

Choose a reason for hiding this comment

kpedro88 May 26, 2021

Choose a reason for hiding this comment

srimanob May 29, 2021

Choose a reason for hiding this comment

srimanob commented May 26, 2021 • edited

kpedro88 commented May 26, 2021

srimanob commented May 26, 2021

cmsbuild commented May 26, 2021

RelVals

cmsbuild commented May 29, 2021

Comparison Summary

civanch commented May 30, 2021

srimanob commented May 30, 2021

qliphy commented May 31, 2021

chayanit commented May 31, 2021

cmsbuild commented May 31, 2021

qliphy commented May 31, 2021

kpedro88 Jun 1, 2021

Choose a reason for hiding this comment

kpedro88 Jun 1, 2021

Choose a reason for hiding this comment

kpedro88 Jun 1, 2021

Choose a reason for hiding this comment

srimanob commented Jun 1, 2021

fabiocos commented Jun 3, 2021

srimanob commented Jun 3, 2021

srimanob commented May 26, 2021 •

edited

srimanob commented May 26, 2021 •

edited