Migrate module configuration in RecoTracker{SpecialSeedGenerators} to use default cfipython #34009

jeongeun · 2021-06-08T08:55:54Z

PR description:

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

RecoTracker/SpecialSeedGenerators/python/CombinatorialSeedGeneratorForBeamHalo_cff.py
RecoTracker/SpecialSeedGenerators/python/CombinatorialSeedGeneratorForBeamHalo_cfi.py
RecoTracker/SpecialSeedGenerators/python/CombinatorialSeedGeneratorForCosmics_cfi.py
RecoTracker/SpecialSeedGenerators/python/CombinatorialSeedGeneratorForCosmicsP5_cff.py
RecoTracker/SpecialSeedGenerators/python/CombinatorialSeedGeneratorForCosmicsRegionalReconstruction_cfi.py
RecoTracker/SpecialSeedGenerators/python/CombinatorialSeedGeneratorForCosmicsRegionalReconstruction_cff.py
RecoTracker/SpecialSeedGenerators/python/SimpleCosmicBONSeeder_cfi.py
RecoTracker/SpecialSeedGenerators/python/SimpleCosmicBONSeeder_cff.py

(The previous PRs were PR#33207, PR#33307, PR#33352, PR#33543, PR#33563, PR#33671, PR#33901, PR#34007)

PR validation:

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

cmsbuild · 2021-06-08T09:02:42Z

+code-checks

Logs: https://cmssdt.cern.ch/SDT/code-checks/cms-sw-PR-34009/23134

This PR adds an extra 16KB to repository

cmsbuild · 2021-06-08T09:03:04Z

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

It involves the following packages:

RecoTracker/SpecialSeedGenerators

@perrotta, @jpata, @cmsbuild, @slava77 can you please review it and eventually sign? Thanks.
@makortel, @felicepantaleo, @GiacomoSguazzoni, @JanFSchulte, @rovere, @VinInn, @gpetruc, @ebrondol, @mtosi, @dgulhan 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-06-08T09:13:54Z

RecoTracker/SpecialSeedGenerators/python/SimpleCosmicBONSeeder_cfi.py

@@ -29,8 +27,34 @@ def makeSimpleCosmicSeedLayers(*layers):
    #print "SEEDING LAYER LIST = ", layerList
    return layerList

-layerInfo = RecoTracker.SpecialSeedGenerators.CombinatorialSeedGeneratorForCosmics_cfi.layerInfo.clone(
-    TEC = dict(useSimpleRphiHitsCleaner = False)
+layerInfo = dict(


Why not keeping cloning from RecoTracker.SpecialSeedGenerators.CombinatorialSeedGeneratorForCosmics_cfi, instead of all this copy/paste?

@perrotta : I got an error when I keep L32-33 as below, so I did simply copy/paste the layerInfo in this SimpleCosmicVONSeeder_cfi.py file.
Please let me know if there's the most suitable way you suggest here.

File "/afs/cern.ch/work/j/jelee/ServiceWork/NewTask/0607/CMSSW_12_0_X_2021-06-06-2300/python/RecoTracker/SpecialSeedGenerators/SimpleCosmicBONSeeder_cfi.py", line 30, in <module> layerInfo = RecoTracker.SpecialSeedGenerators.CombinatorialSeedGeneratorForCosmics_cfi.layerInfo.clone( AttributeError: 'dict' object has no attribute 'clone'

Then probably the simplest solution is to revert the change in https://github.com/cms-sw/cmssw/pull/34009/files#diff-0dbbc624124b7539ebca54a0a8940ad7ca30d06b687c5bbb825c30a51e16dd7eR9

@makortel , perhaps you can help here...

I would like to avoid copy/pasting all the PSet's below, which were already defined in RecoTracker/SpecialSeedGenerators/python/CombinatorialSeedGeneratorForCosmics_cfi.py. This would be inconvenient in case of modifications in the source config, that couldn't be automatically propagated to the copy/pasted ones.

After investigating a bit how do dictionaries and unpacking operatoris work in python, I came to the conclusion that the simplest way to avoid it is to simply revert the changes for layerInfo here and in https://github.com/cms-sw/cmssw/pull/34009/files#diff-0dbbc624124b7539ebca54a0a8940ad7ca30d06b687c5bbb825c30a51e16dd7eR9

But perhaps python allows some more elegant and correct solution, that allows keep defining layerInfo as a dict in the source file, and propagate to the derived config with the possibility to modify a parameter (which is what causes the issue here). Do you have any suggestion?

With dict you can use copy module, i.e. along

import copy dict_copy = copy.deepcopy(dict_orig)

For this specific case I'd like to remind that we recently allowed passing PSet as the first argument(s) for clone(). I think you could keep the layerInfo definition here as it is, and change SimpleCosmicBONSeeder_cff.py to something along

simpleCosmicBONSeedingLayers = _mod.seedingLayersEDProducer.clone( layerInfo, layerList = layerList )

Thank you @makortel
And what about modifying a parameter in an included PSet, as it was done originally in https://github.com/cms-sw/cmssw/pull/34009/files#diff-f633f65b880b125f9f08753ab0640fef555f76d83ce539433c8bcfeb3baecdc5L33 ?

I'm not sure I follow your question. The original code has

layerInfo = RecoTracker.SpecialSeedGenerators.CombinatorialSeedGeneratorForCosmics_cfi.layerInfo.clone( TEC = dict(useSimpleRphiHitsCleaner = False) )

that could stay as it is (the modification is in a clone() call after all).

Ok, Matti, thank you!

Then, if I understand it correctly, the suggestion for @jeongeun is:

Revert the change in RecoTracker/SpecialSeedGenerators/python/CombinatorialSeedGeneratorForCosmics_cfi.py and let the layerInfo as a PSet

Modify simpleCosmicBONSeedingLayers as you write above

Revert also the definition of layerInfo here in SimpleCosmicBONSeeder_cfi.py, with a clone and only the modified parameter replaced in it

Thank you all!
To be consistent, I will revert (dict -> cms.PSet)all the changes in these 3 cfi.py files RecoTracker/SpecialSeedGenerators/python/CombinatorialSeedGeneratorForBeamHalo_cfi.py,
RecoTracker/SpecialSeedGenerators/python/CombinatorialSeedGeneratorForCosmics_cfi.py, RecoTracker/SpecialSeedGenerators/python/CombinatorialSeedGeneratorForCosmicsRegionalReconstruction_cfi.py
Then, I will revert **layerInfo -> layerInfo as it is.

perrotta · 2021-06-08T09:15:36Z

RecoTracker/SpecialSeedGenerators/python/CombinatorialSeedGeneratorForCosmics_cfi.py

@@ -6,7 +6,7 @@
 from RecoTracker.TkTrackingRegions.GlobalTrackingRegion_cfi import *
 from RecoLocalTracker.SiStripClusterizer.SiStripClusterChargeCut_cfi import *

-layerInfo = cms.PSet(
+layerInfo = dict(


Since you are cleaning, you could further align inside the PSet's in layerInfo

cmsbuild · 2021-06-11T08:04:49Z

-code-checks

Logs: https://cmssdt.cern.ch/SDT/code-checks/cms-sw-PR-34009/23258

ERROR: Build errors found during clang-tidy run.

gmake: *** [config/SCRAM/GMake/Makefile.coderules:129: code-checks] Error 2
gmake: *** [There are compilation/build errors. Please see the detail log above.] Error 2

perrotta · 2021-06-11T08:33:54Z

RecoTracker/SpecialSeedGenerators/python/CombinatorialSeedGeneratorForCosmics_cfi.py

        rphiRecHits = cms.InputTag("siStripMatchedRecHits","rphiRecHit"),
        maxRing = cms.int32(7)
-    ),
+  )


I don't understand the Build errors, and the log does'nt help me (@smuzaffar ?)

In any case, this parenthesis has to be brought back to the previous position to remain aligned. Maybe one can try to fix, and see if the Build error disappears...

Moreover @jeongeun could you please further:

Add an empty line before L56, so that one can better notice where the EDProducer configuration actually starts (and perhaps also remove the empty line now uselessly inserted at L58

Align L78-L89

I don't understand the Build errors, and the log does'nt help me (@smuzaffar ?)

You can find comment from @smuzaffar here: #33863 (comment)

I don't understand the Build errors, and the log does'nt help me (@smuzaffar ?)

You can find comment from @smuzaffar here: #33863 (comment)

Thank you @qliphy !

smuzaffar · 2021-06-11T08:55:07Z

code-checks

cmsbuild · 2021-06-11T09:02:41Z

-code-checks

Logs: https://cmssdt.cern.ch/SDT/code-checks/cms-sw-PR-34009/23263

ERROR: Build errors found during clang-tidy run.

gmake: *** [config/SCRAM/GMake/Makefile.coderules:129: code-checks] Error 2
gmake: *** [There are compilation/build errors. Please see the detail log above.] Error 2

cmsbuild · 2021-06-11T09:20:28Z

+code-checks

Logs: https://cmssdt.cern.ch/SDT/code-checks/cms-sw-PR-34009/23270

This PR adds an extra 20KB to repository

cmsbuild · 2021-06-11T09:20:50Z

Pull request #34009 was updated. @perrotta, @jpata, @cmsbuild, @slava77 can you please check and sign again.

cmsbuild · 2021-06-14T05:50:17Z

+code-checks

Logs: https://cmssdt.cern.ch/SDT/code-checks/cms-sw-PR-34009/23301

This PR adds an extra 16KB to repository

cmsbuild · 2021-06-14T05:50:40Z

Pull request #34009 was updated. @perrotta, @jpata, @cmsbuild, @slava77 can you please check and sign again.

perrotta · 2021-06-14T06:13:19Z

please test

cmsbuild · 2021-06-14T09:28:34Z

+1

Summary: https://cmssdt.cern.ch/SDT/jenkins-artifacts/pull-request-integration/PR-da5ecc/15931/summary.html
COMMIT: d4faa16
CMSSW: CMSSW_12_0_X_2021-06-13-2300/slc7_amd64_gcc900
User test area: For local testing, you can use /cvmfs/cms-ci.cern.ch/week1/cms-sw/cmssw/34009/15931/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: 2862520
DQMHistoTests: Total failures: 6
DQMHistoTests: Total nulls: 1
DQMHistoTests: Total successes: 2862491
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 · 2021-06-14T09:55:39Z

+1

Technical migration, as planned
Jenkins tests pass and show no differences in outputs

cmsbuild · 2021-06-14T09:56:04Z

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-06-14T15:35:25Z

+1

replace EDProducer with clone from cfipy and drop type spec

875f645

cmsbuild added this to the CMSSW_12_0_X milestone Jun 8, 2021

cmsbuild added code-checks-pending orp-pending pending-signatures reconstruction-pending tests-pending labels Jun 8, 2021

cmsbuild added code-checks-approved and removed code-checks-pending labels Jun 8, 2021

perrotta reviewed Jun 8, 2021

View reviewed changes

revert to use PSet

f985502

cmsbuild added code-checks-pending and removed code-checks-approved labels Jun 11, 2021

cmsbuild added code-checks-rejected and removed code-checks-pending labels Jun 11, 2021

perrotta reviewed Jun 11, 2021

View reviewed changes

cmsbuild added code-checks-pending and removed code-checks-rejected labels Jun 11, 2021

cmsbuild added code-checks-rejected and removed code-checks-pending labels Jun 11, 2021

cmsbuild added code-checks-approved and removed code-checks-rejected labels Jun 11, 2021

fix

e19f471

cmsbuild added the code-checks-pending label Jun 14, 2021

cmsbuild added code-checks-approved and removed code-checks-pending labels Jun 14, 2021

cmsbuild added tests-started and removed tests-pending labels Jun 14, 2021

cmsbuild added tests-approved and removed tests-started labels Jun 14, 2021

cmsbuild added fully-signed reconstruction-approved and removed pending-signatures reconstruction-pending labels Jun 14, 2021

cmsbuild added orp-approved and removed orp-pending labels Jun 14, 2021

cmsbuild merged commit c362a61 into cms-sw:master Jun 14, 2021

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 configuration in RecoTracker{SpecialSeedGenerators} to use default cfipython #34009

Migrate module configuration in RecoTracker{SpecialSeedGenerators} to use default cfipython #34009

jeongeun commented Jun 8, 2021 •

edited

cmsbuild commented Jun 8, 2021

cmsbuild commented Jun 8, 2021

perrotta Jun 8, 2021

jeongeun Jun 9, 2021

perrotta Jun 9, 2021

perrotta Jun 10, 2021

makortel Jun 10, 2021

perrotta Jun 10, 2021

makortel Jun 10, 2021

perrotta Jun 10, 2021

jeongeun Jun 11, 2021

perrotta Jun 8, 2021

cmsbuild commented Jun 11, 2021

perrotta Jun 11, 2021 •

edited

qliphy Jun 11, 2021 •

edited

perrotta Jun 11, 2021

smuzaffar commented Jun 11, 2021

cmsbuild commented Jun 11, 2021

cmsbuild commented Jun 11, 2021

cmsbuild commented Jun 11, 2021

cmsbuild commented Jun 14, 2021

cmsbuild commented Jun 14, 2021

perrotta commented Jun 14, 2021

cmsbuild commented Jun 14, 2021

perrotta commented Jun 14, 2021

cmsbuild commented Jun 14, 2021

qliphy commented Jun 14, 2021

Migrate module configuration in RecoTracker{SpecialSeedGenerators} to use default cfipython #34009

Migrate module configuration in RecoTracker{SpecialSeedGenerators} to use default cfipython #34009

Conversation

jeongeun commented Jun 8, 2021 • edited

PR description:

PR validation:

cmsbuild commented Jun 8, 2021

cmsbuild commented Jun 8, 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

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 Jun 11, 2021

perrotta Jun 11, 2021 • edited

Choose a reason for hiding this comment

qliphy Jun 11, 2021 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

smuzaffar commented Jun 11, 2021

cmsbuild commented Jun 11, 2021

cmsbuild commented Jun 11, 2021

cmsbuild commented Jun 11, 2021

cmsbuild commented Jun 14, 2021

cmsbuild commented Jun 14, 2021

perrotta commented Jun 14, 2021

cmsbuild commented Jun 14, 2021

Comparison Summary

perrotta commented Jun 14, 2021

cmsbuild commented Jun 14, 2021

qliphy commented Jun 14, 2021

jeongeun commented Jun 8, 2021 •

edited

perrotta Jun 11, 2021 •

edited

qliphy Jun 11, 2021 •

edited