Avoid CPE Fast ES Producers Renaming #40215

AdrianoDee · 2022-12-01T16:41:20Z

Following #40206 (comment) this PR fix the issue of PixelCPEFastESProducers renaming.

PR Validation

Tested with runTheMatrix.py -w upgrade -l 20834.501 -t 8, runTheMatrix.py -w upgrade -l 11634.501 -t 8 and with the gist from #40003 by @mmusich.

@makortel

cmsbuild · 2022-12-01T16:49:11Z

+code-checks

Logs: https://cmssdt.cern.ch/SDT/code-checks/cms-sw-PR-40215/33254

This PR adds an extra 12KB to repository

cmsbuild · 2022-12-01T16:49:33Z

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

It involves the following packages:

RecoLocalTracker/SiPixelRecHits (reconstruction)
RecoTracker/TransientTrackingRecHit (reconstruction)

@cmsbuild, @mandrenguyen, @clacaputo can you please review it and eventually sign? Thanks.
@mtosi, @VourMa, @felicepantaleo, @GiacomoSguazzoni, @JanFSchulte, @rovere, @VinInn, @mroguljic, @missirol, @dkotlins, @ferencek, @gpetruc, @mmusich, @threus, @dgulhan, @tvami 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

AdrianoDee · 2022-12-01T16:50:03Z

please test

AdrianoDee · 2022-12-01T16:50:11Z

type bug-fix

cmsbuild · 2022-12-01T20:41:24Z

-1

Failed Tests: AddOn
Summary: https://cmssdt.cern.ch/SDT/jenkins-artifacts/pull-request-integration/PR-44792b/29404/summary.html
COMMIT: a0aa0bd
CMSSW: CMSSW_13_0_X_2022-12-01-1100/el8_amd64_gcc11
User test area: For local testing, you can use /cvmfs/cms-ci.cern.ch/week1/cms-sw/cmssw/40215/29404/install.sh to create a dev area with all the needed externals and cmssw changes.

AddOn Tests

hlt_mc_GRun

----- Begin Fatal Exception 01-Dec-2022 18:16:47 CET-----------------------
An exception of category 'EventSetupConflict' occurred while
   [0] Calling beginJob
Exception Message:
two EventSetup Producers want to deliver type="PixelClusterParameterEstimator" label="PixelCPEFast"
 from record TkPixelCPERecord. The two providers are 
1) type="PixelCPEFastESProducer" label=""
2) type="PixelCPEFastESProducer" label="pixelCPEFastESProducer"
Please either
   remove one of these Producers
   or find a way of configuring one of them so it does not deliver this data
   or use an es_prefer statement in the configuration to choose one.
----- End Fatal Exception -------------------------------------------------

hlt_mc_HIon

----- Begin Fatal Exception 01-Dec-2022 18:15:55 CET-----------------------
An exception of category 'EventSetupConflict' occurred while
   [0] Calling beginJob
Exception Message:
two EventSetup Producers want to deliver type="PixelClusterParameterEstimator" label="PixelCPEFast"
 from record TkPixelCPERecord. The two providers are 
1) type="PixelCPEFastESProducer" label=""
2) type="PixelCPEFastESProducer" label="pixelCPEFastESProducer"
Please either
   remove one of these Producers
   or find a way of configuring one of them so it does not deliver this data
   or use an es_prefer statement in the configuration to choose one.
----- End Fatal Exception -------------------------------------------------

hlt_mc_PIon

----- Begin Fatal Exception 01-Dec-2022 18:15:02 CET-----------------------
An exception of category 'EventSetupConflict' occurred while
   [0] Calling beginJob
Exception Message:
two EventSetup Producers want to deliver type="PixelClusterParameterEstimator" label="PixelCPEFast"
 from record TkPixelCPERecord. The two providers are 
1) type="PixelCPEFastESProducer" label=""
2) type="PixelCPEFastESProducer" label="pixelCPEFastESProducer"
Please either
   remove one of these Producers
   or find a way of configuring one of them so it does not deliver this data
   or use an es_prefer statement in the configuration to choose one.
----- End Fatal Exception -------------------------------------------------

Expand to see more addon errors ...

Comparison Summary

Summary:

No significant changes to the logs found
Reco comparison results: 4 differences found in the comparisons
DQMHistoTests: Total files compared: 48
DQMHistoTests: Total histograms compared: 3421159
DQMHistoTests: Total failures: 3
DQMHistoTests: Total nulls: 0
DQMHistoTests: Total successes: 3421134
DQMHistoTests: Total skipped: 22
DQMHistoTests: Total Missing objects: 0
DQMHistoSizes: Histogram memory added: 0.0 KiB( 47 files compared)
Checked 206 log files, 158 edm output root files, 48 DQM output files
TriggerResults: no differences found

makortel · 2022-12-01T21:30:27Z

I think the problem is that the HLT menu uses the name that starts with a capital letter

fragment.PixelCPEFastESProducer = cms.ESProducer( "PixelCPEFastESProducer",

(from HLTrigger/Configuration/python/HLT_GRun_cff.py), and now that this PR changes it to lower case, a configuration that loads them both gets two copies of the ESProducer.

The current setup (with the import ... as "rename") works in practice as long as no-one imports/loads the cfi file that defines the ESProducer with lower case label, but that sounds fragile.

mmusich · 2022-12-01T21:34:10Z

I think the problem is that the HLT menu uses the name that starts with a capital letter

Aren't all hlt producers expected to to start with hltSomething ? @cms-sw/hlt-l2

missirol · 2022-12-01T22:35:57Z

Aren't all hlt producers expected to to start with hltSomething ?

Yes and no.. for ESProducers, that convention is not always followed.

https://raw.githubusercontent.com/cms-sw/cmssw/d8a49b979b5ed82b5bb79296713fb485ce9b2344/HLTrigger/Configuration/test/OnLine_HLT_GRun.py

The two ESProducers (the HLT and RECO one) in this case use the same parameter values. One possibility (but I'm guessing) is that the HLT module label was intentionally the same as RECO (1) because they are supposed to be identical, and (2) to avoid clashes like this one when we run HLT+RECO together: in the latter case, iiuc, pre-PR one ESProducer effectively overwrites the other one, in the assumption that this is fine because they are identical. If true, this is very fragile, obviously.

One option is to rename the HLT module as in this PR (but then we might need a backport of this PR to 12_6_X, since HLT menus will still be updated in that cycle for a while), or try to find a better solution. I was wondering if we could change (PixelCPEFastESProducer).ComponentName to a different value at HLT (e.g. "HLTPixelCPEFast"), propagating that renaming to a few HLT modules (e.g. hltSiPixelRecHitsFromLegacy.CPE), to avoid any clash with the RECO ESProducer.

Please let me know if you have suggestions. Will have to have a better look tomorrow.

mmusich · 2022-12-01T23:20:36Z

One option is to rename the HLT module as in this PR (but then we might need a backport of this PR to 12_6_X, since HLT menus will still be updated in that cycle for a while), or try to find a better solution. I was wondering if we could change (PixelCPEFastESProducer).ComponentName to a different value at HLT (e.g. "HLTPixelCPEFast"), propagating that renaming to a few HLT modules (e.g. hltSiPixelRecHitsFromLegacy.CPE), to avoid any clash with the RECO ESProducer.

Renaming + changing the component name is IMHO the way to go. See e.g. what is done for the other pixel CPE ESProducers, (see process.hltESPPixelCPEGeneric )

mmusich · 2022-12-02T07:29:56Z

Hi Marino @missirol, thanks for https://its.cern.ch/jira/browse/CMSHLT-2585.
Do I understand correctly that @AdrianoDee can plugin directly your suggestion here:

def customiseFor40215(process):
    process.hltESPPixelCPEFast = process.PixelCPEFastESProducer.clone(
      ComponentName = 'hltESPPixelCPEFast'
    )
    del process.PixelCPEFastESProducer
    process.hltSiPixelRecHitsFromLegacy.CPE = 'hltESPPixelCPEFast'
    process.hltSiPixelRecHitsGPU.CPE = 'hltESPPixelCPEFast'
    process.hltSiPixelRecHitsFromLegacyCPUOnly.CPE = 'hltESPPixelCPEFast'
    return process

in customizeHLTforCMSSW in order to move on with the PR, and then STORM will take care of cleaning the HLT tables?

missirol · 2022-12-02T07:32:59Z

I would be slightly in favour of waiting for the HLT PR that properly updates the menus, if that works for you guys. It could be done by the end of today.

missirol · 2022-12-02T09:06:51Z

test parameters:

pull_requests = HLT menu development for 12_6_X (1/N) [13_0_X] #40220

missirol · 2022-12-02T09:48:34Z

please test

cmsbuild · 2022-12-02T13:36:51Z

+1

Summary: https://cmssdt.cern.ch/SDT/jenkins-artifacts/pull-request-integration/PR-44792b/29422/summary.html
COMMIT: a0aa0bd
CMSSW: CMSSW_13_0_X_2022-12-01-1100/el8_amd64_gcc11
User test area: For local testing, you can use /cvmfs/cms-ci.cern.ch/week1/cms-sw/cmssw/40215/29422/install.sh to create a dev area with all the needed externals and cmssw changes.

Comparison Summary

Summary:

No significant changes to the logs found
Reco comparison results: 0 differences found in the comparisons
DQMHistoTests: Total files compared: 48
DQMHistoTests: Total histograms compared: 3421150
DQMHistoTests: Total failures: 0
DQMHistoTests: Total nulls: 0
DQMHistoTests: Total successes: 3421128
DQMHistoTests: Total skipped: 22
DQMHistoTests: Total Missing objects: 0
DQMHistoSizes: Histogram memory added: -0.002 KiB( 47 files compared)
DQMHistoSizes: changed ( 7.3,... ): -0.001 KiB HLT/Filters
Checked 206 log files, 158 edm output root files, 48 DQM output files
TriggerResults: no differences found

mmusich · 2022-12-05T04:32:22Z

type trk

clacaputo · 2022-12-05T15:22:35Z

+reconstruction

cmsbuild · 2022-12-05T15:22:59Z

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

mmusich · 2022-12-05T16:10:11Z

RecoTracker/TransientTrackingRecHit/python/TTRHBuilders_cff.py

@@ -7,7 +7,7 @@
 from RecoTracker.TkSeedingLayers.TTRHBuilderWithoutAngle4PixelPairs_cfi import *
 from RecoTracker.TkSeedingLayers.TTRHBuilderWithoutAngle4PixelTriplets_cfi import *
 #TransientTRH builder with template
-from RecoLocalTracker.SiPixelRecHits.pixelCPEFastESProducer_cfi import pixelCPEFastESProducer as PixelCPEFastESProducer
+from RecoLocalTracker.SiPixelRecHits.pixelCPEFastESProducer_cfi import *


as a note to self, it would be good to add here also

from RecoLocalTracker.SiPixelRecHits.pixelCPEFastESProducerPhase2_cfi import *

since the producer for phase-2 is different.

rappoccio · 2022-12-05T20:43:51Z

+1

Avoid CPE Fast ES Producers renaming

a0aa0bd

cmsbuild added this to the CMSSW_13_0_X milestone Dec 1, 2022

cmsbuild added code-checks-pending orp-pending pending-signatures reconstruction-pending tests-pending labels Dec 1, 2022

cmsbuild added code-checks-approved and removed code-checks-pending labels Dec 1, 2022

cmsbuild added tests-started and removed tests-pending labels Dec 1, 2022

cmsbuild added the bug-fix label Dec 1, 2022

cmsbuild added tests-rejected and removed tests-started labels Dec 1, 2022

cmsbuild added the requires-external label Dec 2, 2022

cmsbuild added tests-started and removed tests-rejected labels Dec 2, 2022

cmsbuild added tests-approved and removed tests-started labels Dec 2, 2022

missirol mentioned this pull request Dec 2, 2022

HLT menu development for 12_6_X (1/N) [13_0_X] #40220

Merged

cmsbuild added the trk label Dec 5, 2022

cmsbuild added fully-signed reconstruction-approved and removed reconstruction-pending pending-signatures labels Dec 5, 2022

mmusich reviewed Dec 5, 2022

View reviewed changes

cmsbuild added orp-approved and removed orp-pending labels Dec 5, 2022

cmsbuild merged commit 40e0d38 into cms-sw:master Dec 5, 2022

cmsbuild mentioned this pull request Dec 6, 2022

[G4VECGEOM] Update clhep to 2.4.6.0, vecgeom to 1.2.1 cms-sw/cmsdist#8211

Merged

mmusich mentioned this pull request Jan 31, 2023

remove duplicate plugins left by #38761 and #40264 #40646

Merged

missirol mentioned this pull request Feb 14, 2023

remove HLT frozen menu "2022v15" from 13_1_X branch #40761

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Avoid CPE Fast ES Producers Renaming #40215

Avoid CPE Fast ES Producers Renaming #40215

AdrianoDee commented Dec 1, 2022 •

edited

Loading

cmsbuild commented Dec 1, 2022

cmsbuild commented Dec 1, 2022

AdrianoDee commented Dec 1, 2022

AdrianoDee commented Dec 1, 2022

cmsbuild commented Dec 1, 2022

makortel commented Dec 1, 2022

mmusich commented Dec 1, 2022

missirol commented Dec 1, 2022 •

edited

Loading

mmusich commented Dec 1, 2022 •

edited

Loading

mmusich commented Dec 2, 2022

missirol commented Dec 2, 2022

missirol commented Dec 2, 2022

missirol commented Dec 2, 2022

cmsbuild commented Dec 2, 2022

mmusich commented Dec 5, 2022

clacaputo commented Dec 5, 2022

cmsbuild commented Dec 5, 2022

mmusich Dec 5, 2022

rappoccio commented Dec 5, 2022

Avoid CPE Fast ES Producers Renaming #40215

Avoid CPE Fast ES Producers Renaming #40215

Conversation

AdrianoDee commented Dec 1, 2022 • edited Loading

PR Validation

cmsbuild commented Dec 1, 2022

cmsbuild commented Dec 1, 2022

AdrianoDee commented Dec 1, 2022

AdrianoDee commented Dec 1, 2022

cmsbuild commented Dec 1, 2022

AddOn Tests

Comparison Summary

makortel commented Dec 1, 2022

mmusich commented Dec 1, 2022

missirol commented Dec 1, 2022 • edited Loading

mmusich commented Dec 1, 2022 • edited Loading

mmusich commented Dec 2, 2022

missirol commented Dec 2, 2022

missirol commented Dec 2, 2022

missirol commented Dec 2, 2022

cmsbuild commented Dec 2, 2022

Comparison Summary

mmusich commented Dec 5, 2022

clacaputo commented Dec 5, 2022

cmsbuild commented Dec 5, 2022

mmusich Dec 5, 2022

Choose a reason for hiding this comment

rappoccio commented Dec 5, 2022

AdrianoDee commented Dec 1, 2022 •

edited

Loading

missirol commented Dec 1, 2022 •

edited

Loading

mmusich commented Dec 1, 2022 •

edited

Loading