Move Sequence to Task Reconstruction_cff #28678

jeongeun · 2019-12-22T15:55:33Z

PR description: Move Sequence to Task for Reconstruction(HeavyIons/Cosmics)_cff cff files.

(An extension of the previous task PR#25163, PR#27474 , PR#27640, PR#27887, PR#28050,PR#28300,PR#28399)

Tested in CMSSW_11_1_X_2019_12_20-1100, the basic test all passed in the CMSSW PR instructions.

cmsbuild · 2019-12-22T15:55:59Z

The code-checks are being triggered in jenkins.

cmsbuild · 2019-12-22T16:01:09Z

+code-checks

Logs: https://cmssdt.cern.ch/SDT/code-checks/cms-sw-PR-28678/13248

This PR adds an extra 24KB to repository
There are other open Pull requests which might conflict with changes you have proposed:
- File Configuration/StandardSequences/python/Reconstruction_cff.py modified in PR(s): Phase out old pre-GED GsfElectron sequence #28429

cmsbuild · 2019-12-22T16:01:35Z

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

It involves the following packages:

Configuration/StandardSequences
RecoCTPPS/Configuration
RecoEgamma/EgammaPhotonProducers
RecoLocalFastTime/Configuration
RecoMTD/Configuration
RecoParticleFlow/PFTracking
RecoVertex/Configuration

@perrotta, @kpedro88, @cmsbuild, @franzoni, @slava77, @fabiocos, @davidlange6 can you please review it and eventually sign? Thanks.
@varuns23, @mmarionncern, @Sam-Harper, @makortel, @felicepantaleo, @jainshilpi, @jan-kaspar, @GiacomoSguazzoni, @rovere, @lgray, @Martin-Grunewald, @sobhatta, @seemasharmafnal, @hatakeyamak, @afiqaize, @dgulhan, @ebrondol, @VinInn, @forthommel, @bachtis, @cbernet this is something you requested to watch as well.
@davidlange6, @slava77, @fabiocos, @silviodonato you are the release manager for this.

cms-bot commands are listed here

perrotta · 2019-12-23T06:50:59Z

please test

cmsbuild · 2019-12-23T06:51:09Z

The tests are being triggered in jenkins.
https://cmssdt.cern.ch/jenkins/job/ib-run-pr-tests/4093/console Started: 2019/12/23 07:57

cmsbuild · 2019-12-23T08:41:13Z

-1

Tested at: 6054885

CMSSW: CMSSW_11_1_X_2019-12-22-2300
SCRAM_ARCH: slc7_amd64_gcc820
You can see the results of the tests here:
https://cmssdt.cern.ch/SDT/jenkins-artifacts/pull-request-integration/PR-57203a/4093/summary.html

I found follow errors while testing this PR

Failed tests: RelVals AddOn

RelVals:

When I ran the RelVals I found an error in the following workflows:
5.1 step1

runTheMatrix-results/5.1_TTbar+TTbarFS+HARVESTFS/step1_TTbar+TTbarFS+HARVESTFS.log

135.4 step1

runTheMatrix-results/135.4_ZEE_13+ZEEFS_13+HARVESTUP15FS+MINIAODMCUP15FS/step1_ZEE_13+ZEEFS_13+HARVESTUP15FS+MINIAODMCUP15FS.log

AddOn:

I found errors in the following addon tests:

cmsDriver.py TTbar_8TeV_TuneCUETP8M1_cfi --conditions auto:run1_mc --fast -n 100 --eventcontent AODSIM,DQM --relval 100000,1000 -s GEN,SIM,RECOBEFMIX,DIGI:pdigi_valid,L1,DIGI2RAW,L1Reco,RECO,EI,VALIDATION --customise=HLTrigger/Configuration/CustomConfigs.L1THLT --datatier GEN-SIM-DIGI-RECO,DQMIO --beamspot Realistic8TeVCollision : FAILED - time: date Mon Dec 23 09:22:52 2019-date Mon Dec 23 09:19:12 2019 s - exit: 17920
cmsDriver.py TTbar_13TeV_TuneCUETP8M1_cfi --conditions auto:run2_mc_l1stage1 --fast -n 100 --eventcontent AODSIM,DQM --relval 100000,1000 -s GEN,SIM,RECOBEFMIX,DIGI:pdigi_valid,L1,DIGI2RAW,L1Reco,RECO,EI,VALIDATION --customise=HLTrigger/Configuration/CustomConfigs.L1THLT --datatier GEN-SIM-DIGI-RECO,DQMIO --beamspot NominalCollision2015 --era Run2_25ns : FAILED - time: date Mon Dec 23 09:23:35 2019-date Mon Dec 23 09:19:27 2019 s - exit: 17920
cmsDriver.py TTbar_13TeV_TuneCUETP8M1_cfi --conditions auto:run2_mc --fast -n 100 --eventcontent AODSIM,DQM --relval 100000,1000 -s GEN,SIM,RECOBEFMIX,DIGI:pdigi_valid,L1,DIGI2RAW,L1Reco,RECO,EI,VALIDATION --customise=HLTrigger/Configuration/CustomConfigs.L1THLT --datatier GEN-SIM-DIGI-RECO,DQMIO --beamspot NominalCollision2015 --era Run2_2016 : FAILED - time: date Mon Dec 23 09:23:31 2019-date Mon Dec 23 09:19:33 2019 s - exit: 17920

cmsbuild · 2019-12-23T08:41:17Z

Comparison not run due to runTheMatrix errors (RelVals and Igprof tests were also skipped)

jeongeun · 2019-12-23T10:16:17Z

Configuration/StandardSequences/python/Reconstruction_cff.py

+globalreco = cms.Sequence(globalrecoTask)
+
+_run3_globalrecoTask = globalrecoTask.copyAndExclude([CastorFullRecoTask])
+run3_common.toReplaceWith(globalrecoTask, _run3_globalrecoTask)

 _fastSim_globalreco = globalreco.copyAndExclude([CastorFullReco,muoncosmicreco])
 # insert the few tracking modules to be run after mixing back in the globalreco sequence
 _fastSim_globalreco.insert(0,newCombinedSeeds+trackExtrapolator+caloTowerForTrk+firstStepPrimaryVerticesUnsorted+ak4CaloJetsForTrk+initialStepTrackRefsForJets+firstStepPrimaryVertices)


Errors were produced from "firstStepPrimaryVerticesUnsorted+ak4CaloJetsForTrk" in this line.
I will check this part again.

perrotta · 2019-12-30T10:33:41Z

Configuration/StandardSequences/python/Reconstruction_cff.py

+                          trackingGlobalRecoTask,
+                          hcalGlobalRecoTask,
+                          vertexrecoTask)
+globalreco_tracking = cms.Sequence(globalreco_trackingTask)


This can be removed, as not used enywhere

perrotta · 2019-12-30T10:35:57Z

Configuration/StandardSequences/python/Reconstruction_cff.py

+globalreco = cms.Sequence(globalrecoTask)
+
+_run3_globalrecoTask = globalrecoTask.copyAndExclude([CastorFullRecoTask])
+run3_common.toReplaceWith(globalrecoTask, _run3_globalrecoTask)

 _fastSim_globalreco = globalreco.copyAndExclude([CastorFullReco,muoncosmicreco])


Modifications should be done at the level of Task, as in the other cases

perrotta · 2020-01-09T16:45:34Z

+1

This concludes the migration of the reconstruction Sequences into Tasks
Jenkins tests pass and show no differences, as they should

silviodonato · 2020-01-09T17:16:00Z

+operations

perrotta · 2020-01-13T14:29:56Z

@kpedro88 could you please have a look, and either sign for upgrade or propose updates?

kpedro88 · 2020-01-13T15:12:45Z

+upgrade

cmsbuild · 2020-01-13T15:13:11Z

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

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

+1

silviodonato · 2020-01-14T09:53:45Z

@jeongeun this PR is breaking a RelVal test (workflow 11634.7) of CMSSW_11_1_X_2020-01-13-2300.
See https://cmssdt.cern.ch/SDT/html/cmssdt-ib/#/relVal/CMSSW_11_1/2020-01-13-2300?selectedArchs=slc7_amd64_gcc820&selectedFlavors=X&selectedStatus=failed

I tried the step3 cmsDriver command with and without this PR in CMSSW_11_1_X_2020-01-13-1100.
For some reason, this PR removes "initialStepTrackCandidatesMkFitInput" from process.reconstruction_step. This causes the error:

Exception Message:
Principal::getByToken: Found zero products matching all criteria
Looking for type: MkFitInputWrapper
Looking for module label: initialStepTrackCandidatesMkFitInput
Looking for productInstanceName:

Could you check and provide a fix? Thanks.

You can easily see this issue using the following commands

cmsDriver.py step3  --conditions auto:phase1_2021_realistic -n 10 --era Run3 --eventcontent RECOSIM,MINIAODSIM,DQM --runUnscheduled  -s RAW2DIGI,L1Reco,RECO,RECOSIM,EI,PAT,VALIDATION:@standardValidation+@miniAODValidation,DQM:@standardDQM+@ExtraHLT+@miniAODDQM --datatier GEN-SIM-RECO,MINIAODSIM,DQMIO --customise RecoTracker/MkFit/customizeInitialStepToMkFit.customizeInitialStepToMkFit --geometry DB:Extended  --customise Validation/Performance/TimeMemorySummary.customiseWithTimeMemorySummary --prefix '/data/cmsbld/jenkins/workspace/ib-run-relvals/cms-bot/monitor_workflow.py timeout --signal SIGTERM 9000 '  --filein  file:step2.root  --fileout file:step3.root  --suffix "-j JobReport3.xml "  --nThreads 4 --no_exec
edmConfigDump step3_RAW2DIGI_L1Reco_RECO_RECOSIM_EI_PAT_VALIDATION_DQM.py > step3_dump.py
cat step3_dump.py | grep initialStepTrackCandidatesMkFitInput

perrotta · 2020-01-14T11:10:19Z

Thank you @silviodonato, I'm also trying to investigate.

It is far from obvious to me to understand how this PR may have removed the initialStepTrackCandidatesMkFitInput, which is defined in RecoTracker/MkFit/python/customizeInitialStepToMkFit.py (untouched by this PR), which modifies the InitialStepTask defined in RecoTracker/IterativeTracking/python/InitialStep_cff.py (also untouched by this PR)

@makortel , do you have any suggestion?

makortel · 2020-01-14T14:41:19Z

By quick look I have no idea, but I can take care of fixing it.

perrotta · 2020-01-14T15:43:03Z

By quick look I have no idea, but I can take care of fixing it.

Thank you @makortel !

perrotta · 2020-01-14T16:10:19Z

edmConfigDump step3_RAW2DIGI_L1Reco_RECO_RECOSIM_EI_PAT_VALIDATION_DQM.py | fgrep InitialStepTask
process.InitialStepTask = cms.Task(process.caloJetsForTrkTask, process.firstStepPrimaryVertices, process.firstStepPrimaryVerticesUnsorted, process.initialStep, process.initialStepClassifier1, process.initialStepHitDoublets, process.initialStepHitQuadruplets, process.initialStepSeedLayers, process.initialStepSeeds, process.initialStepTrackCandidates, process.initialStepTrackCandidatesMkFit, process.initialStepTrackCandidatesMkFitInput, process.initialStepTrackRefsForJets, process.initialStepTrackingRegions, process.initialStepTracks)

and initialStepTrackCandidatesMkFitInput is correctly there.

Puzzled...

makortel · 2020-01-14T16:17:46Z

Yeah, but process.reconstructionTask does not contain globalreco_trackingTask (that contains InitialStepTask via other Tasks) but a copy that contains an expanded copy of InitialStepTask. So some operation leads to (partial) expansion of reconstructionTask before the customizeInitialStepToMkFit() gets called. I'm trying to find what exactly triggers the expansion (so far I've only gotten more confused).

makortel · 2020-01-14T21:10:25Z

The problem is essentially the same as in #28703. This line

cmssw/RecoTracker/MkFit/python/customizeInitialStepToMkFit.py

Line 12 in 2d3b452

    
           process.initialStepTrackCandidates = mkFitOutputConverter_cfi.mkFitOutputConverter.clone(

starts a replacement operation in all tasks and sequences that may expand Tasks/Sequences, and the order which the replacement occur is not fully deterministic. Before this PR it happened that the reconstruction sequence had a link to InitialStepTask, and now it doesn't. (ok, the cause for this exact case is probably because the tasks are operated before sequences)

I have two options in mind:

in the customize function, instead of adding the two other modules into InitialStepTask, loop over all tasks, and add them into those that have process.initialStepTrackCandidates
replace the customize function with a Modifier (e.g. trackingMkFit in Configuration.ProcessModifiers)

@perrotta @slava77 @mtosi @JanFSchulte Do you have preference between the two?

perrotta · 2020-01-15T09:38:40Z

Thank you Matti for your investigation and proposals.
I'd let those more expert than me about MkFit to add their more qualified thoughts. Personally, I have the impression that the option with the Modifier is cleaner (and in perspective even safer, because of that), and it can more easily scale in case further modifications will be required for using MkFit: my vote is for (2).

makortel · 2020-01-16T20:53:52Z

In absence of further comments I implemented the Modifier solution in #28755.

silviodonato · 2020-01-17T08:35:48Z

Thanks @makortel

mtosi · 2020-01-17T08:36:52Z

ciao apologies for the l delay in replying thanks for the update, it seems to me the best choice here mia

…

On 16 Jan 2020, at 21:54, Matti Kortelainen ***@***.***> wrote: In absence of further comments I implemented the Modifier solution in #28755. — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub, or unsubscribe.

jeongeun added 2 commits December 22, 2019 10:58

Reconstruction_cff

a36e56e

Cosmic HeavyIons Update

6054885

cmsbuild added this to the CMSSW_11_1_X milestone Dec 22, 2019

cmsbuild added code-checks-pending comparison-pending operations-pending orp-pending pending-signatures reconstruction-pending tests-pending upgrade-pending labels Dec 22, 2019

cmsbuild added code-checks-approved and removed code-checks-pending labels Dec 22, 2019

cmsbuild added tests-started and removed tests-pending labels Dec 23, 2019

cmsbuild added comparison-notrun tests-rejected and removed comparison-pending tests-started labels Dec 23, 2019

jeongeun commented Dec 23, 2019

View reviewed changes

perrotta reviewed Dec 30, 2019

View reviewed changes

fix fastsim_globalreco to fastsim_globalrecoTask

685923d

cmsbuild removed the code-checks-approved label Jan 9, 2020

cmsbuild added reconstruction-approved and removed reconstruction-pending labels Jan 9, 2020

cmsbuild added operations-approved and removed operations-pending labels Jan 9, 2020

cmsbuild added fully-signed upgrade-approved and removed pending-signatures upgrade-pending labels Jan 13, 2020

cmsbuild added orp-approved and removed orp-pending labels Jan 13, 2020

cmsbuild merged commit 85b903f into cms-sw:master Jan 13, 2020

makortel mentioned this pull request Jan 16, 2020

Implement mkFit customization in initialStep with a Modifier #28755

Merged

silviodonato mentioned this pull request May 25, 2020

PPS: updates for re-miniAOD (backport of #29770) #29771

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Move Sequence to Task Reconstruction_cff #28678

Move Sequence to Task Reconstruction_cff #28678

jeongeun commented Dec 22, 2019

cmsbuild commented Dec 22, 2019

cmsbuild commented Dec 22, 2019

cmsbuild commented Dec 22, 2019

perrotta commented Dec 23, 2019

cmsbuild commented Dec 23, 2019 •

edited

Loading

cmsbuild commented Dec 23, 2019

cmsbuild commented Dec 23, 2019

jeongeun Dec 23, 2019 •

edited

Loading

perrotta Dec 30, 2019

perrotta Dec 30, 2019

perrotta commented Jan 9, 2020

silviodonato commented Jan 9, 2020

perrotta commented Jan 13, 2020

kpedro88 commented Jan 13, 2020

cmsbuild commented Jan 13, 2020

silviodonato commented Jan 13, 2020

silviodonato commented Jan 14, 2020 •

edited

Loading

perrotta commented Jan 14, 2020

makortel commented Jan 14, 2020

perrotta commented Jan 14, 2020

perrotta commented Jan 14, 2020

makortel commented Jan 14, 2020

makortel commented Jan 14, 2020

perrotta commented Jan 15, 2020

makortel commented Jan 16, 2020

silviodonato commented Jan 17, 2020

mtosi commented Jan 17, 2020 via email

Move Sequence to Task Reconstruction_cff #28678

Move Sequence to Task Reconstruction_cff #28678

Conversation

jeongeun commented Dec 22, 2019

PR description: Move Sequence to Task for Reconstruction(HeavyIons/Cosmics)_cff cff files.

cmsbuild commented Dec 22, 2019

cmsbuild commented Dec 22, 2019

cmsbuild commented Dec 22, 2019

perrotta commented Dec 23, 2019

cmsbuild commented Dec 23, 2019 • edited Loading

cmsbuild commented Dec 23, 2019

cmsbuild commented Dec 23, 2019

jeongeun Dec 23, 2019 • edited Loading

Choose a reason for hiding this comment

perrotta Dec 30, 2019

Choose a reason for hiding this comment

perrotta Dec 30, 2019

Choose a reason for hiding this comment

perrotta commented Jan 9, 2020

silviodonato commented Jan 9, 2020

perrotta commented Jan 13, 2020

kpedro88 commented Jan 13, 2020

cmsbuild commented Jan 13, 2020

silviodonato commented Jan 13, 2020

silviodonato commented Jan 14, 2020 • edited Loading

perrotta commented Jan 14, 2020

makortel commented Jan 14, 2020

perrotta commented Jan 14, 2020

perrotta commented Jan 14, 2020

makortel commented Jan 14, 2020

makortel commented Jan 14, 2020

perrotta commented Jan 15, 2020

makortel commented Jan 16, 2020

silviodonato commented Jan 17, 2020

mtosi commented Jan 17, 2020 via email

cmsbuild commented Dec 23, 2019 •

edited

Loading

jeongeun Dec 23, 2019 •

edited

Loading

silviodonato commented Jan 14, 2020 •

edited

Loading