Migrate module config in RecoMuon to use automatically generated cfipython default #33563

jeongeun · 2021-04-28T08:57:55Z

PR description:

Optimization of the python configurations: Improve maintainability by cleaning up the duplicated and cloning from the default/reference configurations.

In this PR, 5 files changed.

RecoMuon/L2MuonIsolationProducer
RecoMuon/L2MuonProducer
RecoMuon/L3MuonProducer
RecoMuon/MuonIsolation
RecoMuon/MuonSeedGenerator

(The previous PRs were PR#33207, PR#33307, PR#33352, PR#33543 )

commit 1 :
Replace explicit configuration with a reference from cfipython/. (migrating EDProducer("type", .. -> typeDefault.clone())
commit 2 :
Remove the type specifications already presented in cfipython/fillDescriptions reference for improved syntax safety.
Remove the duplicated parameters that are exactly the same value in cfipython reference.

PR validation:

Event Content comparison check was also done and there is no change with these updates.
Tested in CMSSW_11_3_X, the basic test all passed in the CMSSW PR instructions.

cmsbuild · 2021-04-28T09:04:18Z

+code-checks

Logs: https://cmssdt.cern.ch/SDT/code-checks/cms-sw-PR-33563/22346

This PR adds an extra 20KB to repository

cmsbuild · 2021-04-28T09:04:40Z

A new Pull Request was created by @jeongeun (JeongEun Lee) for master.

It involves the following packages:

RecoMuon/L2MuonIsolationProducer
RecoMuon/L2MuonProducer
RecoMuon/L3MuonProducer
RecoMuon/MuonIsolation
RecoMuon/MuonSeedGenerator

@perrotta, @cmsbuild, @slava77, @Martin-Grunewald, @fwyzard, @jpata can you please review it and eventually sign? Thanks.
@HuguesBrun, @bellan, @abbiendi, @Fedespring, @calderona, @sscruz, @jhgoh, @trocino, @cericeci, @rociovilar 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

perrotta · 2021-04-28T09:29:54Z

please test

cmsbuild · 2021-04-28T13:06:54Z

+1

Summary: https://cmssdt.cern.ch/SDT/jenkins-artifacts/pull-request-integration/PR-b731ec/14666/summary.html
COMMIT: bbb17ba
CMSSW: CMSSW_12_0_X_2021-04-27-2300/slc7_amd64_gcc900
User test area: For local testing, you can use /cvmfs/cms-ci.cern.ch/week0/cms-sw/cmssw/33563/14666/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: 3 differences found in the comparisons
DQMHistoTests: Total files compared: 38
DQMHistoTests: Total histograms compared: 2877605
DQMHistoTests: Total failures: 9
DQMHistoTests: Total nulls: 1
DQMHistoTests: Total successes: 2877573
DQMHistoTests: Total skipped: 22
DQMHistoTests: Total Missing objects: 0
DQMHistoSizes: Histogram memory added: 0.004 KiB( 37 files compared)
DQMHistoSizes: changed ( 312.0 ): 0.004 KiB MessageLogger/Warnings
Checked 160 log files, 37 edm output root files, 38 DQM output files
TriggerResults: no differences found

perrotta

@ArnabPurohit @trocino please notice the following issues that emerged during the configuration clean up work carried on by Jeongeun. It is true that the actual configuration in L3Muons_cfi.py is never used anywhere (and this is probably why the configuration validation didn't catch them), but it is probably worth to have it fixed at some point.

perrotta · 2021-04-28T13:33:15Z

RecoMuon/L3MuonProducer/python/L3Muons_cfi.py

+    L3TrajBuilderParameters = dict(
+        TrackerRecHitBuilder = 'WithTrackAngle',
+        MuonTrackingRegionBuilder = dict(
+            #Eta_fixed = cms.double(0.2), # please check, default type is cms.bool(True)


This was a bug in the original config: as pointed out by @jeongeun this parameter was supposed to be a bool, while it was inserted as a double: as such the defalt Eta_fixed=cms.bool(True) is used instead.
Please check, and suggest a fix, if needed: otherwise the default true value will be retained

Eta_fixed=cms.bool(True) is indeed what is used in the configuration of this producer in the HLT and it should be fixed here to reflect hat.

perrotta · 2021-04-28T13:35:05Z

RecoMuon/L3MuonProducer/python/L3Muons_cfi.py

-            Phi_fixed = cms.double(0.2),
-            DeltaR = cms.double(0.2),
-            EscapePt = cms.double(1.5),
+            #Phi_fixed = cms.double(0.2), # please check, default type is cms.bool(False)


This was a bug in the original config: as pointed out by @jeongeun this parameter was supposed to be a bool, while it was inserted as a double: as such the defalt Eta_fixed=cms.bool(True) is used instead.
Please check, and suggest a fix, if needed: otherwise the default true value will be retained

Phi_fixed=cms.bool(True) is indeed what is used in the configuration of this producer in the HLT and it should be fixed here to reflect hat.

perrotta · 2021-04-28T13:38:52Z

RecoMuon/L3MuonProducer/python/L3Muons_cfi.py

-            EscapePt = cms.double(1.5),
+            #Phi_fixed = cms.double(0.2), # please check, default type is cms.bool(False)
+            DeltaR = 0.2,
+            EscapePt = 1.5,


The EscapePt parameter doesn't seem to be used in L3MuonProducer, MuonTrackingRegionBuilder, or anywhere else in CMSSW. I wasn't even able to retrieve when it was needed last.
Can we clean up and remove it from all configurations?

I think it is safe to remove it, indeed.

trocino · 2021-04-28T19:38:29Z

Thanks JeongEun and Andrea for catching this. We will take a better look and get back to you — even though the situation seems quite clear. Let me add @khaosmos93 @JanFSchulte too, they probably know more about the actual configuration of these parameters in the HLT reconstruction.

perrotta · 2021-04-29T06:33:31Z

RecoMuon/L2MuonIsolationProducer/python/hltL2MuonIsolationsCR_cfi.py

    ),
-    WriteIsolatorFloat = cms.bool(False),
 #    OutputMuIsoDeposits = cms.bool(True),


OutputMuIsoDeposits is not even a parameter for L2MuonIsolationProducer: the commented out line can be removed

perrotta · 2021-04-29T06:38:02Z

RecoMuon/L2MuonProducer/python/L2Muons_cfi.py

-        SeedPropagator = cms.string('SteppingHelixPropagatorL2Any'),
-        NavigationType = cms.string('Standard'),
-        DoBackwardFilter = cms.bool(True),
+        SeedPropagator = 'SteppingHelixPropagatorL2Any',
        # where you want the seed (in,out)


If you keep the comment, you must also keep the corresponding line with SeedPosition

perrotta · 2021-04-29T06:54:29Z

RecoMuon/MuonSeedGenerator/python/ancientMuonSeed_cfi.py

@@ -3,18 +3,15 @@
 from RecoMuon.MuonSeedGenerator.ptSeedParameterization_cfi import *


Suggested change

from RecoMuon.MuonSeedGenerator.ptSeedParameterization_cfi import *

from RecoMuon.MuonSeedGenerator.ptSeedParameterization_cfi import ptSeedParameterization

perrotta · 2021-04-29T06:54:46Z

RecoMuon/MuonSeedGenerator/python/ancientMuonSeed_cfi.py

@@ -3,18 +3,15 @@
 from RecoMuon.MuonSeedGenerator.ptSeedParameterization_cfi import *
 from RecoMuon.MuonSeedGenerator.MuonSeedPtScale_cfi import *


Suggested change

from RecoMuon.MuonSeedGenerator.MuonSeedPtScale_cfi import *

from RecoMuon.MuonSeedGenerator.MuonSeedPtScale_cfi import dphiScale

perrotta · 2021-04-29T07:02:28Z

RecoMuon/MuonSeedGenerator/python/ancientMuonSeed_cfi.py

                                 ptSeedParameterization,
                                 dphiScale,
                                 beamSpotTag = cms.InputTag("offlineBeamSpot"),
                                 scaleDT = cms.bool(True),
                                 CSCRecSegmentLabel = cms.InputTag("cscSegments"),
                                 DTRecSegmentLabel = cms.InputTag("dt4DSegments"),
                                 ME0RecSegmentLabel = cms.InputTag("me0Segments"),
-                                 EnableDTMeasurement = cms.bool(True),


The issue here is that the MuonSeedGenerator has desc.setAllowAnything() in its fillDescriptions method.
As such, only those three parameters which are removed here from the explicit config are defined, while there is no validation for all other ones.
For the moment I'd keep the explicit definition here (taking also into account that EnableME0Measurement is modified twice in the modifier's below), just removing the type specification.
Eventually, all parameters should be defined and validated inside the fillDescriptions method. of MuonSeedGenerator

cmsbuild · 2021-05-04T05:01:18Z

+code-checks

Logs: https://cmssdt.cern.ch/SDT/code-checks/cms-sw-PR-33563/22462

This PR adds an extra 32KB to repository

fwyzard · 2021-05-11T16:43:34Z

looks good to me - but I can't say if @Martin-Grunewald would like to run some ConfDB-parsing test before signing for HLT.

Martin-Grunewald · 2021-05-11T18:38:29Z

Ah no, this is fine with me...

Martin-Grunewald · 2021-05-11T18:38:37Z

+1

cmsbuild · 2021-05-11T18:38:55Z

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-12T00:27:14Z

+1

Martin-Grunewald · 2021-06-03T07:17:50Z

This PR has entered 12_0_0_pre2 and crashes ConfDB parsing! I believe the problem is that:

there is a fillDescriptions generated L2MuonProducer_cfi.py file, and an explicit L2MuonProducer_cfi.py file in the ../python directory.
the latter file ../python/L2MuonProducer_cfi.py includes L2Muons_cff which includes L2Muons_cfi which includes L2MuonProducer_cfi which seems rather circular.

I believe that the problem should be fixed, or at least the cfi logic made consistent, by removing ../python/L2MuonProducer_cfi.py so that only the fillDescriptons one is used on import without any ambiguity.

perrotta · 2021-06-03T20:13:05Z

Thank you @Martin-Grunewald for telling.
In fact, the explicit L2MuonProducer_cfi.py was pre-existent to this PR, and already deprecated since a while already: removing it is the simplest and cleanest way to cleanup the release and help the HLTparser to find the correct config
#33968 removes it

Martin-Grunewald · 2021-06-04T03:43:22Z

Thanks a lot, Andrea!

jeongeun added 2 commits April 27, 2021 18:44

replace EDProducer with clone from the default cfipython

353404a

remove duplicated parameters and cleanup type specifications

bbb17ba

cmsbuild added this to the CMSSW_12_0_X milestone Apr 28, 2021

cmsbuild added code-checks-pending hlt-pending orp-pending pending-signatures reconstruction-pending tests-pending labels Apr 28, 2021

cmsbuild added code-checks-approved and removed code-checks-pending labels Apr 28, 2021

cmsbuild added tests-started and removed tests-pending labels Apr 28, 2021

cmsbuild added tests-approved and removed tests-started labels Apr 28, 2021

perrotta reviewed Apr 28, 2021

View reviewed changes

perrotta reviewed Apr 29, 2021

View reviewed changes

perrotta mentioned this pull request Apr 29, 2021

Configuration parameter validation missing for a few Muon classes and plugins #33572

Open

update - remove deprecated parameter - drop type spec

49a663b

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

cmsbuild removed the code-checks-pending label May 4, 2021

cmsbuild added fully-signed hlt-approved and removed hlt-pending pending-signatures labels May 11, 2021

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

cmsbuild merged commit f60846c into cms-sw:master May 12, 2021

This was referenced May 31, 2021

Migrate module configurations in RecoTracker{ConversionSeedGenerators,DeDx} to use default cfipython #33901

Closed

Migrate module configurations in RecoTracker{IterativeTracking} to use default cfipython #33903

Closed

perrotta mentioned this pull request Jun 3, 2021

Remove explicit L2MuonProducer_cfi #33968

Merged

jeongeun mentioned this pull request Jul 7, 2021

Migrate module configuration in RecoEgamma{EgammaIsolationAlgos} to use default cfipython #34371

Merged

This was referenced Jul 14, 2021

Migrate module config in RecoHI to use default cfipython #34480

Merged

Migrate module configuration in RecoJets to use default cfipython #34543

Merged

Migrate module configuration in RecoMET to use default cfipython #34571

Merged

This was referenced Aug 8, 2021

Migrate module configuration in RecoPixelVertexing to use default cfipython #34817

Merged

Migrate module configuration in RecoVertex to use default cfipython #34818

Merged

Migrate module configuration in RecoTauTag to use default cfipython #34834

Merged

This was referenced Aug 19, 2021

Migrate module configuration in PhysicsTools{PatAlgos} to use default cfipython #34955

Merged

Migrate module configuration in PhysicsTools to use default cfipython #34973

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Migrate module config in RecoMuon to use automatically generated cfipython default #33563

Migrate module config in RecoMuon to use automatically generated cfipython default #33563

jeongeun commented Apr 28, 2021 •

edited

cmsbuild commented Apr 28, 2021

cmsbuild commented Apr 28, 2021

perrotta commented Apr 28, 2021

cmsbuild commented Apr 28, 2021

perrotta left a comment

perrotta Apr 28, 2021

JanFSchulte Apr 29, 2021

perrotta Apr 28, 2021

JanFSchulte Apr 29, 2021

perrotta Apr 28, 2021

JanFSchulte Apr 29, 2021

trocino commented Apr 28, 2021

perrotta Apr 29, 2021

perrotta Apr 29, 2021

perrotta Apr 29, 2021

perrotta Apr 29, 2021

perrotta Apr 29, 2021

cmsbuild commented May 4, 2021

fwyzard commented May 11, 2021

Martin-Grunewald commented May 11, 2021

Martin-Grunewald commented May 11, 2021

cmsbuild commented May 11, 2021

qliphy commented May 12, 2021

Martin-Grunewald commented Jun 3, 2021

perrotta commented Jun 3, 2021

Martin-Grunewald commented Jun 4, 2021

		@@ -3,18 +3,15 @@
		from RecoMuon.MuonSeedGenerator.ptSeedParameterization_cfi import *

		@@ -3,18 +3,15 @@
		from RecoMuon.MuonSeedGenerator.ptSeedParameterization_cfi import *
		from RecoMuon.MuonSeedGenerator.MuonSeedPtScale_cfi import *

Migrate module config in RecoMuon to use automatically generated cfipython default #33563

Migrate module config in RecoMuon to use automatically generated cfipython default #33563

Conversation

jeongeun commented Apr 28, 2021 • edited

PR description:

PR validation:

cmsbuild commented Apr 28, 2021

cmsbuild commented Apr 28, 2021

perrotta commented Apr 28, 2021

cmsbuild commented Apr 28, 2021

Comparison Summary

perrotta left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

trocino commented Apr 28, 2021

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

cmsbuild commented May 4, 2021

fwyzard commented May 11, 2021

Martin-Grunewald commented May 11, 2021

Martin-Grunewald commented May 11, 2021

cmsbuild commented May 11, 2021

qliphy commented May 12, 2021

Martin-Grunewald commented Jun 3, 2021

perrotta commented Jun 3, 2021

Martin-Grunewald commented Jun 4, 2021

jeongeun commented Apr 28, 2021 •

edited