drop type specs in RecoTracker/{MkFit,MeasurementDet,IterativeTracking} #30671

jeongeun · 2020-07-14T07:14:40Z

PR description: Update the safer syntax for existing parameter

Instead of modifying parameters with full type specs, which can be interpreted as an insertion of a new parameter, it is a safer way to protect from parameter name mistakes and will also help in possible parameter migrations.
(The references were PR#29979, PR#30147, PR#30270 , PR#30271, PR#30353, PR#30556)

In this PR, 19 files updated.

RecoTracker/{MkFit} 1 file
RecoTracker/{MeasurementDet} 2 files
RecoTracker/{IterativeTracking} 16 files

PR validation:

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

cmsbuild · 2020-07-14T07:15:09Z

The code-checks are being triggered in jenkins.

cmsbuild · 2020-07-14T07:20:41Z

+code-checks

Logs: https://cmssdt.cern.ch/SDT/code-checks/cms-sw-PR-30671/16962

This PR adds an extra 56KB to repository

cmsbuild · 2020-07-14T07:21:06Z

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

It involves the following packages:

RecoTracker/IterativeTracking
RecoTracker/MeasurementDet
RecoTracker/MkFit

@perrotta, @jpata, @cmsbuild, @slava77 can you please review it and eventually sign? Thanks.
@makortel, @felicepantaleo, @GiacomoSguazzoni, @JanFSchulte, @rovere, @VinInn, @mschrode, @gpetruc, @ebrondol, @mtosi, @dgulhan this is something you requested to watch as well.
@silviodonato, @dpiparo you are the release manager for this.

cms-bot commands are listed here

perrotta · 2020-07-14T07:51:20Z

please test

cmsbuild · 2020-07-14T07:51:41Z

The tests are being triggered in jenkins.

CMSSW_11_2_X_2020-07-14-1100/slc7_amd64_gcc820: https://cmssdt.cern.ch/jenkins/job/ib-run-pr-tests/7925/console Started: 2020/07/14 14:44

cmsbuild · 2020-07-14T14:03:30Z

+1
Tested at: 78b10e5
https://cmssdt.cern.ch/SDT/jenkins-artifacts/pull-request-integration/PR-80982c/7925/summary.html
CMSSW: CMSSW_11_2_X_2020-07-14-1100
SCRAM_ARCH: slc7_amd64_gcc820

cmsbuild · 2020-07-14T14:03:33Z

Comparison job queued.

cmsbuild · 2020-07-14T16:18:26Z

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

Comparison Summary:

No significant changes to the logs found
Reco comparison results: 0 differences found in the comparisons
DQMHistoTests: Total files compared: 37
DQMHistoTests: Total histograms compared: 2787429
DQMHistoTests: Total failures: 1
DQMHistoTests: Total nulls: 0
DQMHistoTests: Total successes: 2787378
DQMHistoTests: Total skipped: 50
DQMHistoTests: Total Missing objects: 0
DQMHistoSizes: Histogram memory added: 0.0 KiB( 36 files compared)
Checked 154 log files, 17 edm output root files, 37 DQM output files

perrotta · 2020-07-15T12:18:58Z

RecoTracker/IterativeTracking/python/HighPtTripletStep_cff.py

    clusterChargeCut = dict(refToPSet_ = 'SiStripClusterChargeCutLoose'),
    pTChargeCutThreshold = 15.
 )
 trackingPhase2PU140.toModify(highPtTripletStepChi2Est,
-    clusterChargeCut = dict(refToPSet_ = "SiStripClusterChargeCutNone"),
+    clusterChargeCut = dict(refToPSet_ = 'SiStripClusterChargeCutNone'),
    MaxChi2 = cms.double(20.0)


This can also get dropped

perrotta · 2020-07-15T12:22:11Z

RecoTracker/IterativeTracking/python/JetCoreRegionalStep_cff.py

-    ComponentName = cms.string('jetCoreRegionalStepChi2Est'),
-    nSigma = cms.double(3.0),
-    MaxChi2 = cms.double(30.0)
+    #ComponentName = cms.string('jetCoreRegionalStepChi2Est'),


Please remove commented out code

perrotta · 2020-07-15T12:46:44Z

RecoTracker/MkFit/python/mkFitInputConverter_cfi.py

-        refToPSet_ = cms.string('SiStripClusterChargeCutLoose')
-    )
+    minGoodStripCharge = dict(
+        refToPSet_ = cms.string('SiStripClusterChargeCutLoose'))


this can also get dropped

The aim was to override the existing

mkFitInputConverter.minGoodStripCharge = cms.PSet( value = cms.required.double )

with

mkFitInputConverter.minGoodStripCharge = cms.PSet( refToPSet_ = cms.string('SiStripClusterChargeCutLoose') )

With @jeongeun's change the resulting PSet would be

mkFitInputConverter.minGoodStripCharge = cms.PSet( value = cms.required.double, refToPSet_ = cms.string('SiStripClusterChargeCutLoose') )

which should still work properly because refToPSet_ overrides whatever else exists in the PSet. But I would find it clearer if refToPSet_ would be the only member of the PSet. Therefore I'd suggest to use cms.PSet(refToPSet_ = cms.string('...')) instead of dict(...) everywhere a refToPSet_ is inserted, because the aim is to override the existing PSet instead of overriding a single PSet member.

Dropping cms.string() should not work because the original PSet does not have refToPSet_ member.

perrotta · 2020-07-15T12:47:03Z

RecoTracker/MeasurementDet/python/Chi2ChargeMeasurementEstimator_cfi.py

-Chi2ChargeMeasurementEstimator = Chi2ChargeMeasurementEstimatorDefault.clone()
-Chi2ChargeMeasurementEstimator.clusterChargeCut = cms.PSet(refToPSet_ = cms.string('SiStripClusterChargeCutTight'))
+Chi2ChargeMeasurementEstimator = Chi2ChargeMeasurementEstimatorDefault.clone(
+    clusterChargeCut = dict(refToPSet_ = cms.string('SiStripClusterChargeCutTight'))


this can also get dropped ("string")

The string needs to stay (no refToPSet_ member in the original PSet), and I'd suggest to keep the explicit PSet there as well.

cmsbuild · 2020-07-15T17:02:18Z

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

perrotta · 2020-07-15T17:14:25Z

please test

cmsbuild · 2020-07-15T17:14:51Z

The tests are being triggered in jenkins.

CMSSW_11_2_X_2020-07-14-2300/slc7_amd64_gcc820: https://cmssdt.cern.ch/jenkins/job/ib-run-pr-tests/7974/console Started: 2020/07/15 19:18

cmsbuild · 2020-07-15T18:31:46Z

+1
Tested at: 14c7ef7
https://cmssdt.cern.ch/SDT/jenkins-artifacts/pull-request-integration/PR-80982c/7974/summary.html
CMSSW: CMSSW_11_2_X_2020-07-14-2300
SCRAM_ARCH: slc7_amd64_gcc820

cmsbuild · 2020-07-15T18:31:49Z

Comparison job queued.

cmsbuild · 2020-07-15T20:11:43Z

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

@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-80982c/28234.0_TTbar_14TeV+TTbar_14TeV_TuneCP5_2026D60_GenSimHLBeamSpotFull14+DigiFullTrigger_2026D60+RecoFullGlobal_2026D60+HARVESTFullGlobal_2026D60

Comparison Summary:

No significant changes to the logs found
Reco comparison results: 0 differences found in the comparisons
DQMHistoTests: Total files compared: 35
DQMHistoTests: Total histograms compared: 2612943
DQMHistoTests: Total failures: 1
DQMHistoTests: Total nulls: 0
DQMHistoTests: Total successes: 2612894
DQMHistoTests: Total skipped: 48
DQMHistoTests: Total Missing objects: 0
DQMHistoSizes: Histogram memory added: 0.0 KiB( 34 files compared)
Checked 146 log files, 17 edm output root files, 35 DQM output files

perrotta · 2020-07-16T06:33:13Z

+1

Type specification in cloned /modified python configs dropped as planned
Jenkins tests pass

cmsbuild · 2020-07-16T06:33:38Z

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 (and backports should be raised in the release meeting by the corresponding L2)

silviodonato · 2020-07-16T08:06:00Z

+1

silviodonato · 2020-07-16T21:29:13Z

@jeongeun in the last IB we have three failing workflows (see https://cmssdt.cern.ch/SDT/html/cmssdt-ib/#/relVal/CMSSW_11_2/2020-07-16-1100?selectedArchs=slc7_amd64_gcc820&selectedFlavors=X&selectedStatus=failed). The message is

RAW2DIGI,RECO:reconstruction_trackingOnly,VALIDATION:@trackingOnlyValidation,DQM:@trackingOnlyDQM
We have determined that this is simulation (if not, rerun cmsDriver.py with --data)
entry file:step2.root
Step: RAW2DIGI Spec: 
Step: RECO Spec: ['reconstruction_trackingOnly']
Traceback (most recent call last):
  File "/cvmfs/cms-ib.cern.ch/nweek-02637/slc7_amd64_gcc820/cms/cmssw/CMSSW_11_2_X_2020-07-16-1100/bin/slc7_amd64_gcc820/cmsDriver.py", line 56, in &lt;module&gt;
    run()
  File "/cvmfs/cms-ib.cern.ch/nweek-02637/slc7_amd64_gcc820/cms/cmssw/CMSSW_11_2_X_2020-07-16-1100/bin/slc7_amd64_gcc820/cmsDriver.py", line 28, in run
    configBuilder.prepare()
  File "/cvmfs/cms-ib.cern.ch/nweek-02637/slc7_amd64_gcc820/cms/cmssw/CMSSW_11_2_X_2020-07-16-1100/python/Configuration/Applications/ConfigBuilder.py", line 2119, in prepare
    self.addStandardSequences()
  File "/cvmfs/cms-ib.cern.ch/nweek-02637/slc7_amd64_gcc820/cms/cmssw/CMSSW_11_2_X_2020-07-16-1100/python/Configuration/Applications/ConfigBuilder.py", line 759, in addStandardSequences
    getattr(self,"prepare_"+stepName)(sequence = '+'.join(stepSpec))
  File "/cvmfs/cms-ib.cern.ch/nweek-02637/slc7_amd64_gcc820/cms/cmssw/CMSSW_11_2_X_2020-07-16-1100/python/Configuration/Applications/ConfigBuilder.py", line 1623, in prepare_RECO
    self.loadDefaultOrSpecifiedCFF(sequence,self.RECODefaultCFF)
  File "/cvmfs/cms-ib.cern.ch/nweek-02637/slc7_amd64_gcc820/cms/cmssw/CMSSW_11_2_X_2020-07-16-1100/python/Configuration/Applications/ConfigBuilder.py", line 1195, in loadDefaultOrSpecifiedCFF
    l=self.loadAndRemember(defaultCFF)
  File "/cvmfs/cms-ib.cern.ch/nweek-02637/slc7_amd64_gcc820/cms/cmssw/CMSSW_11_2_X_2020-07-16-1100/python/Configuration/Applications/ConfigBuilder.py", line 325, in loadAndRemember
    self.process.load(includeFile)
  File "/cvmfs/cms-ib.cern.ch/nweek-02637/slc7_amd64_gcc820/cms/cmssw/CMSSW_11_2_X_2020-07-16-1100/python/FWCore/ParameterSet/Config.py", line 683, in load
    module = __import__(moduleName)
  File "/cvmfs/cms-ib.cern.ch/nweek-02637/slc7_amd64_gcc820/cms/cmssw/CMSSW_11_2_X_2020-07-16-1100/python/Configuration/StandardSequences/Reconstruction_cff.py", line 9, in &lt;module&gt;
    from RecoTracker.Configuration.RecoTracker_cff import *
  File "/cvmfs/cms-ib.cern.ch/nweek-02637/slc7_amd64_gcc820/cms/cmssw/CMSSW_11_2_X_2020-07-16-1100/python/RecoTracker/Configuration/RecoTracker_cff.py", line 5, in &lt;module&gt;
    from RecoTracker.IterativeTracking.iterativeTk_cff import *
  File "/cvmfs/cms-ib.cern.ch/nweek-02637/slc7_amd64_gcc820/cms/cmssw/CMSSW_11_2_X_2020-07-16-1100/python/RecoTracker/IterativeTracking/iterativeTk_cff.py", line 10, in &lt;module&gt;
    from RecoTracker.IterativeTracking.PixelLessStep_cff import *
  File "/cvmfs/cms-ib.cern.ch/nweek-02637/slc7_amd64_gcc820/cms/cmssw/CMSSW_11_2_X_2020-07-16-1100/python/RecoTracker/IterativeTracking/PixelLessStep_cff.py", line 182, in &lt;module&gt;
    ClusterShapeCacheSrc      = 'siPixelClusterShapeCache' # not really needed here since FilterPixelHits=False
  File "/cvmfs/cms-ib.cern.ch/nweek-02637/slc7_amd64_gcc820/cms/cmssw/CMSSW_11_2_X_2020-07-16-1100/python/FWCore/ParameterSet/Config.py", line 1616, in toModify
    Modifier._toModify(obj,func,**kw)
  File "/cvmfs/cms-ib.cern.ch/nweek-02637/slc7_amd64_gcc820/cms/cmssw/CMSSW_11_2_X_2020-07-16-1100/python/FWCore/ParameterSet/Config.py", line 1623, in _toModify
    temp(obj)
  File "/cvmfs/cms-ib.cern.ch/nweek-02637/slc7_amd64_gcc820/cms/cmssw/CMSSW_11_2_X_2020-07-16-1100/python/FWCore/ParameterSet/Config.py", line 1517, in __call__
    _modifyParametersFromDict(params, self.__args, self._raiseUnknownKey)
  File "/cvmfs/cms-ib.cern.ch/nweek-02637/slc7_amd64_gcc820/cms/cmssw/CMSSW_11_2_X_2020-07-16-1100/python/FWCore/ParameterSet/Mixins.py", line 711, in _modifyParametersFromDict
    ("%s.%s" if isinstance(key, str) else "%s[%s]")%(keyDepth,key))
  File "/cvmfs/cms-ib.cern.ch/nweek-02637/slc7_amd64_gcc820/cms/cmssw/CMSSW_11_2_X_2020-07-16-1100/python/FWCore/ParameterSet/Mixins.py", line 739, in _modifyParametersFromDict
    errorRaiser(key)
  File "/cvmfs/cms-ib.cern.ch/nweek-02637/slc7_amd64_gcc820/cms/cmssw/CMSSW_11_2_X_2020-07-16-1100/python/FWCore/ParameterSet/Config.py", line 1526, in _raiseUnknownKey
    raise KeyError("Unknown parameter name "+key+" specified while calling Modifier")
KeyError: 'Unknown parameter name ClusterShapeCacheSrc specified while calling Modifier'

I suspect it is related to this PR which applies changes to RecoTracker/IterativeTracking/python/PixelLessStep_cff.py.
Could you check and eventually provide a fix? Thanks.

drop type specs where original parameter exists

78b10e5

cmsbuild added this to the CMSSW_11_2_X milestone Jul 14, 2020

cmsbuild added code-checks-pending comparison-pending orp-pending pending-signatures reconstruction-pending tests-pending labels Jul 14, 2020

cmsbuild added code-checks-approved and removed code-checks-pending labels Jul 14, 2020

cmsbuild added tests-started and removed tests-pending labels Jul 14, 2020

cmsbuild added tests-approved and removed tests-started labels Jul 14, 2020

cmsbuild added comparison-available and removed comparison-pending labels Jul 14, 2020

perrotta reviewed Jul 15, 2020

View reviewed changes

to override the existing PSet

14c7ef7

cmsbuild added code-checks-pending and removed code-checks-approved comparison-available tests-approved labels Jul 15, 2020

cmsbuild added code-checks-approved and removed code-checks-pending labels Jul 15, 2020

cmsbuild added tests-started and removed tests-pending labels Jul 15, 2020

cmsbuild added tests-approved and removed tests-started labels Jul 15, 2020

cmsbuild added comparison-available and removed comparison-pending labels Jul 15, 2020

cmsbuild added fully-signed reconstruction-approved and removed pending-signatures reconstruction-pending labels Jul 16, 2020

cmsbuild added orp-approved and removed orp-pending labels Jul 16, 2020

cmsbuild merged commit 6edba65 into cms-sw:master Jul 16, 2020

silviodonato mentioned this pull request Jul 16, 2020

Unknown parameter name ClusterShapeCacheSrc specified while calling Modifier #30766

Closed

This was referenced Jul 17, 2020

Fix Unknown parameter name ClusterShapeCacheSrc specified while calling Modifier #30767

Merged

drop type specs in RecoTracker/{TkSeedingLayers,TrackProducer} #30827

Merged

jeongeun mentioned this pull request Jul 28, 2020

drop type specs in RecoTracker #30947

Merged

jeongeun mentioned this pull request Jan 20, 2021

drop type specifications in RecoTracker #32694

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

drop type specs in RecoTracker/{MkFit,MeasurementDet,IterativeTracking} #30671

drop type specs in RecoTracker/{MkFit,MeasurementDet,IterativeTracking} #30671

jeongeun commented Jul 14, 2020 •

edited

Loading

cmsbuild commented Jul 14, 2020

cmsbuild commented Jul 14, 2020

cmsbuild commented Jul 14, 2020

perrotta commented Jul 14, 2020

cmsbuild commented Jul 14, 2020 •

edited

Loading

cmsbuild commented Jul 14, 2020

cmsbuild commented Jul 14, 2020

cmsbuild commented Jul 14, 2020

perrotta Jul 15, 2020

perrotta Jul 15, 2020

perrotta Jul 15, 2020

makortel Jul 15, 2020

perrotta Jul 15, 2020

makortel Jul 15, 2020

cmsbuild commented Jul 15, 2020

perrotta commented Jul 15, 2020

cmsbuild commented Jul 15, 2020 •

edited

Loading

cmsbuild commented Jul 15, 2020

cmsbuild commented Jul 15, 2020

cmsbuild commented Jul 15, 2020

perrotta commented Jul 16, 2020

cmsbuild commented Jul 16, 2020

silviodonato commented Jul 16, 2020

silviodonato commented Jul 16, 2020 •

edited

Loading

drop type specs in RecoTracker/{MkFit,MeasurementDet,IterativeTracking} #30671

drop type specs in RecoTracker/{MkFit,MeasurementDet,IterativeTracking} #30671

Conversation

jeongeun commented Jul 14, 2020 • edited Loading

PR description: Update the safer syntax for existing parameter

PR validation:

cmsbuild commented Jul 14, 2020

cmsbuild commented Jul 14, 2020

cmsbuild commented Jul 14, 2020

perrotta commented Jul 14, 2020

cmsbuild commented Jul 14, 2020 • edited Loading

cmsbuild commented Jul 14, 2020

cmsbuild commented Jul 14, 2020

cmsbuild commented Jul 14, 2020

perrotta Jul 15, 2020

Choose a reason for hiding this comment

perrotta Jul 15, 2020

Choose a reason for hiding this comment

perrotta Jul 15, 2020

Choose a reason for hiding this comment

makortel Jul 15, 2020

Choose a reason for hiding this comment

perrotta Jul 15, 2020

Choose a reason for hiding this comment

makortel Jul 15, 2020

Choose a reason for hiding this comment

cmsbuild commented Jul 15, 2020

perrotta commented Jul 15, 2020

cmsbuild commented Jul 15, 2020 • edited Loading

cmsbuild commented Jul 15, 2020

cmsbuild commented Jul 15, 2020

cmsbuild commented Jul 15, 2020

perrotta commented Jul 16, 2020

cmsbuild commented Jul 16, 2020

silviodonato commented Jul 16, 2020

silviodonato commented Jul 16, 2020 • edited Loading

jeongeun commented Jul 14, 2020 •

edited

Loading

cmsbuild commented Jul 14, 2020 •

edited

Loading

cmsbuild commented Jul 15, 2020 •

edited

Loading

silviodonato commented Jul 16, 2020 •

edited

Loading