New seeding framework #16635

cmsbuild · 2016-11-16T09:49:00Z

Despite of submitting this PR to 81X, this PR really targets (early) 90X (I'm submitting it now in the hope of gaining more time for the review in case L2s would have time to look already now).

This PR provides a new way to plug the seeding code to CMSSW. It takes the TrackingRegion creation, cluster number check, and doublet/triplet/quadruplet generation out of the seed generator, and makes each of them an EDProducer. The regions, decision of the cluster number check, and the hit doublets/triplets are delivered to subsequent components via edm::Event.

The implementation is fully backwards compatible, i.e. all existing configurations should work out of the box. Instances of SeedGeneratorFromRegionHitsEDProducer can be migrated one at a time. Here the offline iterative tracking is fully migrated (including, with minimal effort, knock-on effects in FastSim and HI), but nothing else (e.g. HLT) is not (yet).

Pros

Much more flexibility in interfaces (not more constrained by "common denominator")
fillDescriptions fully supported
For PixelQuadrupletGenerator, removes repetitive doublet generation of certain layer pairs
For CAHit*Generator
- Allows fast regional ntuplet finding
  - No need to do everything per region, but fill CA from all doublets and find ntuplets for all regions simultaneously on one go
- Allows (easily) to put (e.g.) quadruplets and triplets, found simulateneously on one go, to Event as separate collections
  - Can be exploited to reduce repetitive hit triplet generation between iterations
Allows "medium-grained" parallelization
Opens further optimization possibilities

Cons

Additional data structures to deliver information between modules
More modules to manage in configuration

Subsequent steps

Do similar transformation to PixelTrackProducer
- Additional complication from adding fillDescriptions support for fitter, filter, and cleaner
Migrate all seeding and pixel track modules in HLT (in customizeHLTforCMSSW.py)

Expected effect

Tested in CMSSW_8_1_X_2016-10-04-1100 (rebased on top of CMSSW_8_1_X_2016-11-08-2300), no changes expected in monitored quantities. The expected speedup in 10224 (2017 ttbar+PU) seeding is ~2-3 %. The expected memory increase is ~10 MB (based on IgProf LIVE, SimpleMemoryCheck on 6 threads gives similar order of magnitude although across-event variations are larger).

@rovere @VinInn @mtosi @felicepantaleo

Automatically ported from CMSSW_8_1_X #16202 (original by @makortel).

cmsbuild · 2016-11-16T09:49:49Z

A new Pull Request was created by @cmsbuild for CMSSW_9_0_X.

It involves the following packages:

DataFormats/TrackerRecHit2D
FastSimulation/Tracking
RecoHI/HiMuonAlgos
RecoHI/HiTracking
RecoMuon/GlobalTrackingTools
RecoPixelVertexing/PixelLowPtUtilities
RecoPixelVertexing/PixelTriplets
RecoTauTag/HLTProducers
RecoTracker/Configuration
RecoTracker/ConversionSeedGenerators
RecoTracker/IterativeTracking
RecoTracker/SpecialSeedGenerators
RecoTracker/TkHitPairs
RecoTracker/TkSeedGenerator
RecoTracker/TkSeedingLayers
RecoTracker/TkTrackingRegions
TrackingTools/TransientTrackingRecHit
Validation/RecoTrack

@perrotta, @cmsbuild, @civanch, @lveldere, @silviodonato, @cvuosalo, @fwyzard, @ssekmen, @mdhildreth, @dmitrijus, @Martin-Grunewald, @slava77, @vanbesien, @davidlange6 can you please review it and eventually sign? Thanks.
@ghellwig, @abbiendi, @Martin-Grunewald, @battibass, @makortel, @jhgoh, @dgulhan, @HuguesBrun, @trocino, @yetkinyilmaz, @GiacomoSguazzoni, @rovere, @VinInn, @bellan, @wmtford, @mschrode, @amagitte, @echapon, @mandrenguyen, @MiheeJo, @jazzitup, @calderona, @yenjie, @kurtejung, @gpetruc, @matt-komm, @rociovilar, @bachtis this is something you requested to watch as well.
@slava77, @smuzaffar you are the release manager for this.

cms-bot commands are listed here #13028

VinInn · 2016-11-16T09:53:28Z

I assume all discussion moves here now..
@slava77 , could you please summarize your concerns w/r/t the current version?

VinInn · 2016-11-16T09:53:44Z

@cmsbuild , please test

cmsbuild · 2016-11-16T09:54:05Z

The tests are being triggered in jenkins.
https://cmssdt.cern.ch/jenkins/job/ib-any-integration/16366/console

slava77 · 2016-11-16T14:01:31Z

On 11/16/16 1:53 AM, Vincenzo Innocente wrote:

I assume all discussion moves here now..
@slava77 https://github.com/slava77 , could you please summarize your
concerns w/r/t the current version?

The main concern is the memory increase: its lifetime increase as well
as general accumulation (per-element temporaries are now computed and
kept for the full event).
For the memory lifetime increase we do not have good support in the
framework to reduce it.

If the new data structures can be reduced in size to roughly the old
total, we can proceed.

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

VinInn · 2016-11-16T14:08:11Z

We can work on the memory from now to May. We need to find a way to get rid of unused event-product: building our own mini-framework is not an option.
I assume that will the help of the FWCore team we will manage.

So I propose to proceed not to stop critical development for PhaseI.

@Dr15Jones @arizzi @venturia

VinInn · 2016-11-16T14:10:06Z

Btw unused event-products affect only VSS. They will be migrated away from RSS if memory is required. Deleting them at the end of the event will take of course longer.

slava77 · 2016-11-16T14:22:39Z

On 11/16/16 6:08 AM, Vincenzo Innocente wrote:

We can work on the memory from now to May. We need to find a way to get
rid of unused event-product: building our own mini-framework is not an
option.
I assume that will the help of the FWCore team we will manage.

So I propose to proceed not to stop critical development for PhaseI.

@Dr15Jones https://github.com/Dr15Jones @arizzi
https://github.com/arizzi @venturia https://github.com/venturia

Unfortunately, the memory issue is blocking phase-2 simulations.
So, we can not move on with this version.

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

Dr15Jones · 2016-11-16T14:23:41Z

@mdhildreth @davidlange6 opinions?

VinInn · 2016-11-16T14:27:24Z

On 16 Nov, 2016, at 3:22 PM, Slava Krutelyov [email protected] wrote:

Unfortunately, the memory issue is blocking phase-2 simulations.
So, we can not move on with this version.
This is simply not acceptable.
PhaseII issue shal be addressed in phaseII.
It cannot jeopardize phaseI critical development.

makortel · 2016-11-16T14:28:25Z

As I mentioned in #16202 (comment), it is relatively straightforward to craft a customize function that employs the existing early-delete mechanism to remove the memory increase.

(I'm not saying it should be done, just that something can be done with existing framework support)

slava77 · 2016-11-16T14:29:30Z

On 11/16/16 6:28 AM, Matti Kortelainen wrote:

As I mentioned in #16202 (comment)
#16202 (comment), it
is relatively straightforward to craft a customize function that employs
the existing early-delete mechanism to remove the memory increase.

(I'm not saying it should be done, just that something can be done with
existing framework support)

I was told that the existing early delete support is not stable.

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

makortel · 2016-11-16T14:30:58Z

I was told that the existing early delete support is not stable.

Ok. It would be interesting hear more details.

Dr15Jones · 2016-11-16T14:32:26Z

I wouldn't think it is not stable, more that it is hard to maintain.

VinInn · 2016-11-16T14:36:50Z

@Dr15Jones, could you please more specific w/r/t "hard to maintain"?
From a Framework side or a config side?
My understanding is that, for these specific products, we (well Matti) can code a python functions that will take care of all the "magic" and be fully portable to any future evolution of iterative tracking

Dr15Jones · 2016-11-16T14:38:09Z

@Dr15Jones, could you please more specific w/r/t "hard to maintain"?
From a Framework side or a config side?

Config side.

VinInn · 2016-11-16T14:57:15Z

My understanding is that @makortel is confident that it will be no more hard to maintain than current iterative tracking...
(and surely lighter than a home-made mini-edm solution)

makortel · 2016-11-16T15:09:41Z

My understanding is that @makortel is confident that will be no more hard than to maintain that current iterative tracking...

That is correct. It should be enough to know

producer C++ types whose products are supposed to be early-deleted
the types of aforementioned products
all consumers use InputTags

to programmatically set up everything in a generic way in a customize function (and for this case we know).

slava77 · 2016-11-16T15:28:50Z

Can this PR wait for 3-6 months? (until the phase-2 settles; the current large combinatorial problem is partly temporary, at least the extreme end of it with up to 3GB or per-event products there now and processing times of almost 4 hours in 10HS06 units).

It's not introducing a new algorithm, it's just refactoring that simplifies some configuration work with some potential reduction of repetitive work done currently internally.

davidlange6 · 2016-11-23T18:01:58Z

RecoTracker/Configuration/python/customiseEarlyDeleteForSeeding.py

+        _resolve(keys, name)
+
+    # Find the consumers
+    for moduleType in [process.producers_(), process.filters_(), process.analyzers_()]:


and can this loop move the Configuration/StandardSequences so we do it just once (when the next use case comes along..)

Sure. Then the customiseEarlyDeleteForSeeding needs to return the products dictionary (string -> list of strings) instead of process.

cmsbuild · 2016-11-23T18:44:53Z

+1
Tested at: 0fd1027
https://cmssdt.cern.ch/SDT/jenkins-artifacts/pull-request-integration/PR-16635/16567/summary.html

cmsbuild · 2016-11-23T18:44:56Z

Comparison job queued.

cmsbuild · 2016-11-23T20:53:51Z

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

cmsbuild · 2016-11-23T22:02:19Z

+1
Tested at: 71a871a
https://cmssdt.cern.ch/SDT/jenkins-artifacts/pull-request-integration/PR-16635/16570/summary.html

cmsbuild · 2016-11-23T22:02:21Z

Comparison job queued.

cmsbuild · 2016-11-23T23:54:18Z

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

makortel · 2016-11-24T09:59:24Z

@davidlange6 Are your comments for a subsequent PR or do you want updates here?

davidlange6 · 2016-11-24T10:52:21Z

its ok to be a followup (that makes pre1)

…

On Nov 24, 2016, at 10:59 AM, Matti Kortelainen ***@***.***> wrote: @davidlange6 Are your comments for a subsequent PR or do you want updates here? — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub, or mute the thread.

davidlange6 · 2016-11-24T10:52:41Z

Hi all - i'm propose to merge this tonight unless there are further comments to the contrary...

slava77 · 2016-11-24T13:16:59Z

+1

for #16635 71a871a

code change is in line with the needs and the review so far, modulo the expected updates in the upcoming PR
jenkins tests pass and comparisons with baseline show no relevant differences
local tests with higher stat show expected behavior: no differences in monitored physics quantities and small changes in the CPU or memory costs
- PU200 test in D4 (CMSSW_8_1_X_2016-11-10-1100) shows a net decrease in peak RSS of 0.37 GiB on 50 events based on 10-thread run (the first ~1/3 of the run this PR had a higher RSS by about 0.1 GiB per thread on average); CPU is reshuffled from the old style modules to the new ones for about the same total (I observe a small increase here)

cmsbuild added this to the Next CMSSW_9_0_X milestone Nov 16, 2016

cmsbuild added comparison-pending dqm-pending fastsim-pending hlt-pending orp-pending pending-signatures reconstruction-pending tests-pending labels Nov 16, 2016

cmsbuild added tests-started and removed tests-pending labels Nov 16, 2016

davidlange6 reviewed Nov 23, 2016

View reviewed changes

cmsbuild added comparison-available and removed comparison-pending labels Nov 23, 2016

cmsbuild added comparison-pending tests-approved and removed comparison-available tests-started labels Nov 23, 2016

cmsbuild added comparison-available and removed comparison-pending labels Nov 23, 2016

cmsbuild added reconstruction-approved and removed reconstruction-pending labels Nov 24, 2016

davidlange6 merged commit 547ec9c into cms-sw:CMSSW_9_0_X Nov 24, 2016

This was referenced Dec 5, 2016

Remove PixelTracksForL3Isolation configurations and IsolationRegionAroundL3Muon as obsolete #16863

Merged

Remove PixelTripletLowPtGenerator as obsolete #16886

Closed

makortel mentioned this pull request Jan 13, 2017

Migrate PixelTrackProducer and HLT to new seeding framework #17170

Merged

makortel mentioned this pull request Mar 14, 2017

Add CA hit ntuplets for early deletion #17918

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

New seeding framework #16635

New seeding framework #16635

cmsbuild commented Nov 16, 2016

cmsbuild commented Nov 16, 2016

VinInn commented Nov 16, 2016

VinInn commented Nov 16, 2016

cmsbuild commented Nov 16, 2016 •

edited

Loading

slava77 commented Nov 16, 2016

VinInn commented Nov 16, 2016

VinInn commented Nov 16, 2016 •

edited

Loading

slava77 commented Nov 16, 2016

Dr15Jones commented Nov 16, 2016

VinInn commented Nov 16, 2016

makortel commented Nov 16, 2016

slava77 commented Nov 16, 2016

makortel commented Nov 16, 2016

Dr15Jones commented Nov 16, 2016

VinInn commented Nov 16, 2016 •

edited

Loading

Dr15Jones commented Nov 16, 2016

VinInn commented Nov 16, 2016 •

edited

Loading

makortel commented Nov 16, 2016

slava77 commented Nov 16, 2016

davidlange6 Nov 23, 2016

makortel Nov 24, 2016

cmsbuild commented Nov 23, 2016

cmsbuild commented Nov 23, 2016

cmsbuild commented Nov 23, 2016

cmsbuild commented Nov 23, 2016

cmsbuild commented Nov 23, 2016

cmsbuild commented Nov 23, 2016

makortel commented Nov 24, 2016

davidlange6 commented Nov 24, 2016 via email

davidlange6 commented Nov 24, 2016

slava77 commented Nov 24, 2016

New seeding framework #16635

New seeding framework #16635

Conversation

cmsbuild commented Nov 16, 2016

Pros

Cons

Subsequent steps

Expected effect

cmsbuild commented Nov 16, 2016

VinInn commented Nov 16, 2016

VinInn commented Nov 16, 2016

cmsbuild commented Nov 16, 2016 • edited Loading

slava77 commented Nov 16, 2016

VinInn commented Nov 16, 2016

VinInn commented Nov 16, 2016 • edited Loading

slava77 commented Nov 16, 2016

Dr15Jones commented Nov 16, 2016

VinInn commented Nov 16, 2016

makortel commented Nov 16, 2016

slava77 commented Nov 16, 2016

makortel commented Nov 16, 2016

Dr15Jones commented Nov 16, 2016

VinInn commented Nov 16, 2016 • edited Loading

Dr15Jones commented Nov 16, 2016

VinInn commented Nov 16, 2016 • edited Loading

makortel commented Nov 16, 2016

slava77 commented Nov 16, 2016

davidlange6 Nov 23, 2016

Choose a reason for hiding this comment

makortel Nov 24, 2016

Choose a reason for hiding this comment

cmsbuild commented Nov 23, 2016

cmsbuild commented Nov 23, 2016

cmsbuild commented Nov 23, 2016

cmsbuild commented Nov 23, 2016

cmsbuild commented Nov 23, 2016

cmsbuild commented Nov 23, 2016

makortel commented Nov 24, 2016

davidlange6 commented Nov 24, 2016 via email

davidlange6 commented Nov 24, 2016

slava77 commented Nov 24, 2016

cmsbuild commented Nov 16, 2016 •

edited

Loading

VinInn commented Nov 16, 2016 •

edited

Loading

VinInn commented Nov 16, 2016 •

edited

Loading

VinInn commented Nov 16, 2016 •

edited

Loading