Migrate tracker local reconstruction to Tasks

[makortel]

Title says it all. I migrated all downstream configurations I managed to find with git grep.

Tested in CMSSW_10_4_X_2018-11-05-1100, no changes expected.

[cmsbuild]

The code-checks are being triggered in jenkins.

[cmsbuild]

+code-checks

Logs: https://cmssdt.cern.ch/SDT/code-checks/cms-sw-PR-25122/7120

[cmsbuild]

A new Pull Request was created by @makortel (Matti Kortelainen) for master.

It involves the following packages:

Configuration/StandardSequences
DQM/Integration
DQM/SiStripMonitorClient
DQM/TrackingMonitor
EventFilter/SiPixelRawToDigi
RecoLocalTracker/Configuration
RecoLocalTracker/SiPixelClusterizer
RecoLocalTracker/SiPixelRecHits
RecoPixelVertexing/PixelTrackFitting
SimTracker/SiPixelDigitizer

@perrotta, @andrius-k, @kmaeshima, @schneiml, @civanch, @mdhildreth, @cmsbuild, @franzoni, @jfernan2, @slava77, @fabiocos, @davidlange6 can you please review it and eventually sign? Thanks.
@echabert, @felicepantaleo, @forthommel, @pieterdavid, @Martin-Grunewald, @fioriNTU, @thomreis, @threus, @mmusich, @venturia, @hdelanno, @yduhm, @GiacomoSguazzoni, @rovere, @VinInn, @jandrea, @alesaggio, @idebruyn, @ebrondol, @mtosi, @dgulhan, @batinkov, @gbenelli, @dkotlins, @gpetruc this is something you requested to watch as well.
@davidlange6, @slava77, @fabiocos you are the release manager for this.

cms-bot commands are listed here

[makortel]

@cmsbuild, please test workflow 135.13,10024.1,10024.5

[cmsbuild]

The tests are being triggered in jenkins.
https://cmssdt.cern.ch/jenkins/job/ib-any-integration/31476/console Started: 2018/11/05 19:56

[cmsbuild]

+1
Tested at: a19c7a7
https://cmssdt.cern.ch/SDT/jenkins-artifacts/pull-request-integration/PR-25122/31476/summary.html

[cmsbuild]

Comparison job queued.

[cmsbuild]

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

@slava77 comparisons for the following workflows were not done due to missing matrix map:

• /build/cmsbld/jenkins/workspace/compare-root-files-short-matrix/results/JR-comparison/PR-25122/10024.1_TTbar_13+TTbar_13TeV_TuneCUETP8M1_2017_GenSimFull+DigiFull_2017+RecoFull_trackingOnly_2017+HARVESTFull_trackingOnly_2017
• /build/cmsbld/jenkins/workspace/compare-root-files-short-matrix/results/JR-comparison/PR-25122/10024.5_TTbar_13+TTbar_13TeV_TuneCUETP8M1_2017_GenSimFull+DigiFull_2017+RecoFull_pixelTrackingOnly_2017+HARVESTFull_pixelTrackingOnly_2017
• /build/cmsbld/jenkins/workspace/compare-root-files-short-matrix/results/JR-comparison/PR-25122/135.13_TTbar_13+TTbarFS_13_trackingOnlyValidation+HARVESTUP15FS_trackingOnly

Comparison Summary:

• No significant changes to the logs found
• Reco comparison results: 0 differences found in the comparisons
• DQMHistoTests: Total files compared: 32
• DQMHistoTests: Total histograms compared: 2993155
• DQMHistoTests: Total failures: 2
• DQMHistoTests: Total nulls: 0
• DQMHistoTests: Total successes: 2992956
• DQMHistoTests: Total skipped: 197
• DQMHistoTests: Total Missing objects: 0
• DQMHistoSizes: Histogram memory added: 0.0 KiB( 31 files compared)
• Checked 134 log files, 14 edm output root files, 32 DQM output files

[andrius-k]

Hi @makortel, are you planning to test this PR in playback?

[makortel]

@andrius-k Umm, I'm not (I have no idea how to do that), but I don't mind if someone else tests.

[boudoul]

My two cents : This is a pure 10_4_X developments, and will be (assuming success of the review) part of a 10_4_X release at some point - I do not see the point of a playback - 10_4_X release will be used in the online DQM (and therefore part of a playback) in ... a year > 2018

[makortel]

On the other hand I do agree that testing the online clients (as part of PRs and/or in IBs) would be beneficial, as otherwise it is very easy to accidentally break them in technical migrations like here (as has happened many times in the past). Otherwise we may end up in testing them next time in spring 2021 or so.

[andrius-k]

@makortel, as @boudoul mentioned I didn't realise, that this is for 10_4_X so playback testing doesn't make sense.

[mmusich]

@andrius-k @boudoul @makortel,
as far as I know it is possible to test the client using past RAW data (which is not as good as testing the code in the playback, but for sure it's better than nothing).
My modest proposal is to ask @jandrea and @arossi83 to re-run the client on some past data with these changes to make sure it doens't break the online client (though seems hard to believe it might do so, as offline there is no apparent change)

[slava77]

git grep -l -w trackerlocalreco | grep -c py$ returns 49 hits.
I see only 8 files modified here with trackerlocalreco.

One set of cases is a different definition of trackerlocalreco in e.g. RecoLocalTracker/Configuration/python/RecoLocalTrackerHeavyIons_cff.py and RecoLocalTracker/Configuration/python/RecoLocalTracker_Cosmics_cff.py. Where these not migrated on purpose?

Ignoring /test, /clients, /doc directories, and commented out lines, it looks like everything coming out of RecoLocalTracker/Configuration/python/RecoLocalTracker_cff.py was migrated. Good.

Looking at /test e.g. DQM/BeamMonitor/test/beam_dqm_sourceclient-file_cfg.py and a few similar examples pick the trackerlocalreco from Configuration/StandardSequences/python/Reconstruction_cff.py and then use * or + syntax to add it to a Sequence type.
IIUC, configurations like these will be broken.
So, this PR is incomplete in the context of the python configs available in the release.

A somewhat less invasive migration already used in the past is to define a trackerlocalrecoTask and instantiate a trackerlocalreco cms.Sequence from it and then explicitly change to trackerlocalrecoTask where it is relevant.

[makortel]

@slava77 Thanks, so my grepping has become really rusty...

A somewhat less invasive migration already used in the past is to define a trackerlocalrecoTask and instantiate a trackerlocalreco cms.Sequence from it and then explicitly change to trackerlocalrecoTask where it is relevant.

Yeah, that might be a good idea (maybe for #25110 as well). I did think of that, but recalling my past irritation with e.g. the task+sequence approach in iterative tracking configuration wanted to try to do the full migration as well, but then didn't (naively) realize how cumbersome it would really be. Maybe we then need to have (nearly) all standard sequences migrated to tasks first before removing the then-empty sequences.

[slava77]

On 11/7/18 3:06 PM, Matti Kortelainen wrote: @slava77 Thanks, so my grepping has become really rusty... A somewhat less invasive migration already used in the past is to define a trackerlocalrecoTask and instantiate a trackerlocalreco cms.Sequence from it and then explicitly change to trackerlocalrecoTask where it is relevant. Yeah, that might be a good idea (maybe for #25110 as well). I did think of that, but recalling my past irritation with e.g. the task+sequence approach in iterative tracking configuration wanted to try to do the full migration as well, but then didn't (naively) realize how cumbersome it would really be. Maybe we then need to have (nearly) all standard sequences migrated to tasks first before removing the then-empty sequences.

Now that I've looked more carefully, I see that my signoff of #25110 was premature. The xyzTask option can lead to quite a bit of duplication. What are the dangers of making the * or + on tasks in sequences/paths to be treated as .associate ?

[makortel]

The xyzTask option can lead to quite a bit of duplication.

That was exactly the part I didn't like in it. On the other hand the duplication is limited to Sequence/Task objects (i.e. for each current Sequence there would be a Task as well), but the contents of a Task(/Sequence) would be defined only once, and the Sequence is defined just as foo = Sequence(fooTask) (which could still be tolerable).

[makortel]

@slava77

What are the dangers of making the * or + on tasks in sequences/paths to
be treated as .associate ?

They would introduce the wrong mental model. For example, with Sequence a Path(fooFilter + fooSequence) it is clear that fooSequence will be run for an event only if fooFilter returned true. With a Task, if a similar construct Path(fooFilter + fooTask) would be allowed, it could be mis-interpreted to have the same logic (especially when the Task would not have a "Task" in its name).

[makortel]

Superseded by #25163 (that avoids a massive migration, sorry for the noise).