Moved fastSim reconstruction to use era #20884

Dr15Jones · 2017-10-10T20:05:23Z

When trying to convert the reconstruction configurations from Sequences to Tasks, we found that the fastsim was greatly complicating the transition. One reason was the fastsim was directly rewriting Configuration.StandardSequences.Reconstruction_cff from within FastSimulation.Configuration.Reconstruction_AftMix_cff which was leading to very 'brittle' configuration code. For example, if someone accidently loaded Reconstruction_cff into the Process before Reconstruction_AftMix_cff the configuration would no longer work. Switching to fully using the fastSim era avoids many problems and makes fastsim behave like the other configuration modification workflows.

cmsbuild · 2017-10-10T20:08:10Z

The code-checks are being triggered in jenkins.

Dr15Jones · 2017-10-10T20:08:38Z

I used release validation workflow 25402.0 as the test bed. I used the following simple configuration for testing

from TTbar_13TeV_TuneCUETP8M1_cfi_GEN_SIM_RECOBEFMIX_DIGI_L1_DIGI2RAW_L1Reco_RECO_EI_VALIDATION_DQM_PU import process
process.prune()
print process.dumpPython()

With that configuration, I found the my change returns a configuration with exactly the same module configuration as before. In addition, I checked that the Paths reconstruction_step and reconstruction_befmix_step contain the exact same list of modules.

Dr15Jones · 2017-10-10T20:09:35Z

@gartung here is the fastsim change

cmsbuild · 2017-10-10T20:10:06Z

+code-checks

Logs: https://cmssdt.cern.ch/SDT/code-checks/PR-20884/1361

cmsbuild · 2017-10-10T20:10:29Z

A new Pull Request was created by @Dr15Jones (Chris Jones) for master.

It involves the following packages:

Configuration/StandardSequences
FastSimulation/Configuration
FastSimulation/Tracking
RecoEcal/EgammaClusterProducers
RecoEgamma/Configuration
RecoEgamma/EgammaPhotonProducers
RecoLocalCalo/Configuration
RecoLocalCalo/EcalRecProducers
RecoMET/Configuration
RecoMuon/Configuration
RecoMuon/GlobalMuonProducer
RecoMuon/GlobalTrackingTools
RecoMuon/MuonIdentification
RecoMuon/TrackingTools
RecoParticleFlow/PFTracking
RecoTracker/Configuration
RecoTracker/FinalTrackSelectors
RecoTracker/IterativeTracking
RecoTracker/MeasurementDet
TrackingTools/GsfTracking
TrackingTools/TrackRefitter

@perrotta, @civanch, @lveldere, @ssekmen, @mdhildreth, @cmsbuild, @franzoni, @slava77, @davidlange6 can you please review it and eventually sign? Thanks.
@ghellwig, @TaiSakuma, @gouskos, @felicepantaleo, @jainshilpi, @abbiendi, @argiro, @Martin-Grunewald, @ahinzmann, @lgray, @varuns23, @seemasharmafnal, @mmarionncern, @battibass, @makortel, @jhgoh, @dgulhan, @jdolen, @HuguesBrun, @trocino, @rociovilar, @Sam-Harper, @GiacomoSguazzoni, @rovere, @VinInn, @jdamgov, @bellan, @nhanvtran, @gkasieczka, @schoef, @mschrode, @ebrondol, @amagitte, @echapon, @calderona, @cbernet, @gpetruc, @matt-komm, @mariadalfonso, @bachtis this is something you requested to watch as well.
@davidlange6, @slava77 you are the release manager for this.

cms-bot commands are listed here

Dr15Jones · 2017-10-10T20:10:37Z

please test

cmsbuild · 2017-10-10T20:10:53Z

The tests are being triggered in jenkins.
https://cmssdt.cern.ch/jenkins/job/ib-any-integration/23655/console Started: 2017/10/10 22:11

cmsbuild · 2017-10-10T23:13:09Z

-1

Tested at: 50c957c

The following merge commits were also included on top of IB + this PR after doing git cms-merge-topic:
dd401df
a2aeaf0
d9807b8
da408a3
e802724
You can see more details here:
https://cmssdt.cern.ch/SDT/jenkins-artifacts/pull-request-integration/PR-20884/23655/git-log-recent-commits
https://cmssdt.cern.ch/SDT/jenkins-artifacts/pull-request-integration/PR-20884/23655/git-merge-result

You can see the results of the tests here:
https://cmssdt.cern.ch/SDT/jenkins-artifacts/pull-request-integration/PR-20884/23655/summary.html

I found follow errors while testing this PR

Failed tests: RelVals

RelVals:

When I ran the RelVals I found an error in the following worklfows:
1000.0 step3

runTheMatrix-results/1000.0_RunMinBias2011A+RunMinBias2011A+TIER0+SKIMD+HARVESTDfst2+ALCASPLIT/step3_RunMinBias2011A+RunMinBias2011A+TIER0+SKIMD+HARVESTDfst2+ALCASPLIT.log

The following merge commits were also included on top of IB + this PR after doing git cms-merge-topic:
dd401df
a2aeaf0
d9807b8
da408a3
e802724
You can see more details here:
https://cmssdt.cern.ch/SDT/jenkins-artifacts/pull-request-integration/PR-20884/23655/git-log-recent-commits
https://cmssdt.cern.ch/SDT/jenkins-artifacts/pull-request-integration/PR-20884/23655/git-merge-result

cmsbuild · 2017-10-10T23:13:11Z

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

Dr15Jones · 2017-10-11T00:10:03Z

The workflow 1000.0 is failing in the IBs with the exact same configuration error.

slava77 · 2017-10-11T03:49:26Z

Changes in this PR should be discussed in some joint meeting with fastsim category managers.
This PR is effectively fusing two categories at the package assignment level and may have to require a joint signature or some other non-obvious solution.

ssekmen · 2017-10-11T03:51:39Z

We have indeed planned to discuss the PR in the next simulation meeting when all simulation conveners are available.

slava77 · 2017-10-25T08:09:43Z

On 10/25/17 9:58 AM, Matti Kortelainen wrote: Below are some comments regarding changes in tracking configurations after some more thought. We are not particularly happy of making the already-complex iterative tracking configuration files more complicated. Said that, this PR alone would be acceptable (even if barely) for us. But we have concerns what will happen when FastSim will start to support phase1 pixel+tracking (will 2016 support continue?) or phase2 at some point. Were any alternative strategies thought (even if they would more or less break the era design "modify module where it is defined")? E.g. would something along the following work out in light of the Task migration (#20879 <#20879>) # In FastSimulation/Configuration/python/Reconstruction_cff.py from Configuration.StandardSequences.Reconstruction_cff import * <move all reco fastsim era customizations here> ? (basically following the current separation of FastSim and Reco but doing the reco customizations using the |fastsim| era)

Design of eras works best if the modifiers are applied in the same file where the original modules are defined. Perhaps it's time to think of refactoring the Iter_cff files? Best would be to see the same configuration used for fastsim and fullsim reco. How far are we from that point?

…

— You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub <#20884 (comment)>, or mute the thread <https://github.com/notifications/unsubscribe-auth/AEdcbqHXpm0AiI9Pb7ren0woAPO1-o5Vks5svuo0gaJpZM4P0f5x>.

makortel · 2017-10-25T08:58:41Z

Perhaps it's time to think of refactoring the Iter_cff files?

Do you mean something like for each iterations splitting the seeding, pattern recognition+final fit, and selection to their own cff files imported in the main iteration cff file that also defines the iteration Task(/Sequence)?

slava77 · 2017-10-25T09:19:56Z

On 10/25/17 10:58 AM, Matti Kortelainen wrote: Perhaps it's time to think of refactoring the Iter_cff files? Do you mean something like for each iterations splitting the seeding, pattern recognition+final fit, and selection to their own cff files imported in the main iteration cff file that also defines the iteration Task(/Sequence)?

I haven't thought of a specific change pattern. It was more of a general statement

…

— You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub <#20884 (comment)>, or mute the thread <https://github.com/notifications/unsubscribe-auth/AEdcbgHwjhwDJ4bgWW9UhTruM8A5vSUnks5svvhBgaJpZM4P0f5x>.

Dr15Jones · 2017-10-25T13:26:35Z

I agree with Slava

Design of eras works best if the modifiers are applied in the same file where the original modules are defined.

and I will go further. Not putting the modifier in the original module can lead to unforseen side-effects. E.g. clones that do not match or sequences that break.

perrotta · 2017-10-30T15:53:51Z

I have just verified that this PR interferes with #20666: @ssekmen please let us know which are the plans for the integration of the two PRs, priorities, etc.

ssekmen · 2017-10-30T15:58:18Z

@Perrota, discussing with the FastSim tracking experts. We will come with a more concrete plan at our presentation at the tracking meeting on Nov 6.

perrotta · 2017-10-30T16:13:47Z

Thank you Sezen. At the same time, I have to correct myself, and apologize: #20884 and #20666 do NOT interfere each other, in the sense that I was able to merge them one on top of the other, without conflicts, starting from 940pre3. I only badly interpreted the git output at a first glance. Anyhow, having a concrete plan is good: let us know after nov 6. (At this point, I must imagine that they are both only meant to 10_0_X and the 2018+ releases. Cheers, Andrea Sezen Sekmen <notifications@github.com> ha scritto:

…

@Perrota, discussing with the FastSim experts. We will come with a more concrete plan at our presentation at the tracking meeting on Nov 6. -- You are receiving this because you were mentioned. Reply to this email directly or view it on GitHub: #20884 (comment)

perrotta · 2017-10-31T14:28:51Z

+1

Reco part of this PR was reviewed and approved
Effect of this PR will likely be that the maintenance of the fastsim modules goes towards DPG/POGs instead of being kept separated in the fastsim package
Several modules/sequences are marked with comments like "not commissioned and not relevant in FastSim": during the fastsim review it can be worth going through all those components and simply remove whatever not needed, thus simplifying and improving the future maintenance

davidlange6 · 2017-11-08T17:10:30Z

hi @civanch - this is the PR discussed in the ORP yesterday

ssekmen · 2017-11-08T22:42:11Z

+1

davidlange6 · 2017-11-09T10:59:18Z

merge

kpedro88 · 2017-11-10T15:57:53Z

@Dr15Jones I noticed some failures in fastsim premixing workflows in the IB tests (https://cms-sw.github.io/relvalLogDetail.html#slc7_aarch64_gcc700;CMSSW_10_0_X_2017-11-10-1100):

----- Begin Fatal Exception 10-Nov-2017 14:52:24 CET-----------------------
An exception of category 'Configuration' occurred while
   [0] Constructing the EventProcessor
Exception Message:
EDAlias does not match data
There are no products of type 'recoTracks'
with module label 'mix' and instance name 'generalTracks'.
----- End Fatal Exception -------------------------------------------------

Might be caused by this PR, can you take a look?

kpedro88 · 2017-11-10T15:59:03Z

ah, I see you were faster than me with #21263.

Moved fastSim reconstruction to use era

50c957c

cmsbuild added this to the CMSSW_9_4_X milestone Oct 10, 2017

cmsbuild added code-checks-pending comparison-pending fastsim-pending operations-pending orp-pending pending-signatures reconstruction-pending tests-pending labels Oct 10, 2017

cmsbuild added code-checks-approved and removed code-checks-pending labels Oct 10, 2017

cmsbuild added tests-started and removed tests-pending labels Oct 10, 2017

cmsbuild added comparison-notrun tests-rejected and removed comparison-pending tests-started labels Oct 10, 2017

smuzaffar modified the milestones: CMSSW_9_4_X, CMSSW_10_0_X Oct 26, 2017

cmsbuild modified the milestones: CMSSW_10_0_X, CMSSW_9_4_X Oct 26, 2017

cmsbuild added reconstruction-approved and removed reconstruction-pending labels Oct 31, 2017

cmsbuild added fastsim-approved and removed fastsim-pending labels Nov 8, 2017

cmsbuild added orp-approved and removed orp-pending labels Nov 9, 2017

cmsbuild merged commit 7a54396 into cms-sw:master Nov 9, 2017

slava77 mentioned this pull request Nov 9, 2017

cms.Sequence->cms.Task conversion for reconstruction #20879

Merged

Dr15Jones deleted the moveFastSimRecoToEras branch November 10, 2017 15:31

makortel mentioned this pull request Nov 10, 2017

Fix MultiTrackValidator seeding layers configuration for FastSim #21276

Merged

ssekmen mentioned this pull request Aug 25, 2018

Move fast sim reco to eras - backport to 94X #24391

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Moved fastSim reconstruction to use era #20884

Moved fastSim reconstruction to use era #20884

Dr15Jones commented Oct 10, 2017

cmsbuild commented Oct 10, 2017

Dr15Jones commented Oct 10, 2017

Dr15Jones commented Oct 10, 2017

cmsbuild commented Oct 10, 2017

cmsbuild commented Oct 10, 2017

Dr15Jones commented Oct 10, 2017

cmsbuild commented Oct 10, 2017 •

edited

cmsbuild commented Oct 10, 2017

cmsbuild commented Oct 10, 2017

Dr15Jones commented Oct 11, 2017

slava77 commented Oct 11, 2017

ssekmen commented Oct 11, 2017

slava77 commented Oct 25, 2017 via email

makortel commented Oct 25, 2017

slava77 commented Oct 25, 2017 via email

Dr15Jones commented Oct 25, 2017

perrotta commented Oct 30, 2017

ssekmen commented Oct 30, 2017 •

edited

perrotta commented Oct 30, 2017 via email

perrotta commented Oct 31, 2017

davidlange6 commented Nov 8, 2017

ssekmen commented Nov 8, 2017

davidlange6 commented Nov 9, 2017

kpedro88 commented Nov 10, 2017

kpedro88 commented Nov 10, 2017

Moved fastSim reconstruction to use era #20884

Moved fastSim reconstruction to use era #20884

Conversation

Dr15Jones commented Oct 10, 2017

cmsbuild commented Oct 10, 2017

Dr15Jones commented Oct 10, 2017

Dr15Jones commented Oct 10, 2017

cmsbuild commented Oct 10, 2017

cmsbuild commented Oct 10, 2017

Dr15Jones commented Oct 10, 2017

cmsbuild commented Oct 10, 2017 • edited

cmsbuild commented Oct 10, 2017

cmsbuild commented Oct 10, 2017

Dr15Jones commented Oct 11, 2017

slava77 commented Oct 11, 2017

ssekmen commented Oct 11, 2017

slava77 commented Oct 25, 2017 via email

makortel commented Oct 25, 2017

slava77 commented Oct 25, 2017 via email

Dr15Jones commented Oct 25, 2017

perrotta commented Oct 30, 2017

ssekmen commented Oct 30, 2017 • edited

perrotta commented Oct 30, 2017 via email

perrotta commented Oct 31, 2017

davidlange6 commented Nov 8, 2017

ssekmen commented Nov 8, 2017

davidlange6 commented Nov 9, 2017

kpedro88 commented Nov 10, 2017

kpedro88 commented Nov 10, 2017

cmsbuild commented Oct 10, 2017 •

edited

ssekmen commented Oct 30, 2017 •

edited