Discuss adding a clear method to Sequence

[Dr15Jones]

A request has been made to add a clear() method to the python Sequence class.

[cmsbuild]

A new Issue was created by @Dr15Jones (Chris Jones).

@davidlange6, @smuzaffar, @Degano, @davidlt, @Dr15Jones can you please review it and eventually sign/assign? Thanks.

cms-bot commands are listed here

[Dr15Jones]

assign core

[cmsbuild]

New categories assigned: core

@Dr15Jones,@smuzaffar you have been requested to review this Pull request/Issue and eventually sign? Thanks

[Dr15Jones]

@makortel Can you give the use case where you want to do a clear()? Also, is there a reason that just reassigning the variable to a newly made Sequence is insufficient?

[makortel]

It comes from my third prototype for using eras to switch tracking configuration for phase1:

• HN message https://hypernews.cern.ch/HyperNews/CMS/get/edmFramework/3597/1/2/1/1/1/1/1/1/1/1.html
• prototype CMSSW_8_0_X_2016-01-26-1100...makortel:phase1EraMinimal_v3_2

I need to clear and re-use the existing object e.g. here
CMSSW_8_0_X_2016-01-26-1100...makortel:phase1EraMinimal_v3_2#diff-03048ee4a92a77fa73752cfe25831e3fR38. I've tried

def _modifyForPhase1(process):
    global iterTracking
    iterTracking = cms.Sequence()
    ...

but that doesn't do the job at all, i.e. what ends up as the process.iterTracking is the original sequence. Same happens with globals()["iterTracking"] = cms.Sequence().

I also tried

def _modifyForPhase1(process):
    process.iterTracking = cms.Sequence()
    ...
    process.iterTracking += ( ... )

and now process.iterTracking is correct and everything seems to work. But when I apply the same pattern e.g. here CMSSW_8_0_X_2016-01-26-1100...makortel:phase1EraMinimal_v3_2#diff-dbe13006eef842a122c2702ec308ec01R13, something weird happens for process.reconstruction sequence (https://github.com/cms-sw/cmssw/blob/CMSSW_8_0_X/Configuration/StandardSequences/python/Reconstruction_cff.py#L96). Instead of containing process.iterTracking, it now shows (only relevant part shown, line breaks are my own)

$ edmConfigDump step3_reco.py | fgrep "process.reconstruction "
process.reconstruction = cms.Sequence(... +
    process.InitialStepPreSplitting+process.InitialStep+process.DetachedTripletStep+
    process.LowPtTripletStep+process.PixelPairStep+process.MixedTripletStep+
    process.PixelLessStep++process.JetCoreRegionalStep+
    process.earlyGeneralTracksSequence+process.muonSeededStep+
    process.preDuplicateMergingGeneralTracksSequence+
    process.generalTracksSequence+process.ConvStep+process.conversionStepTracks+ ...)

i.e. parts of the original iterTracking, and note the ++ between PixelLessStep and JetCoreRegionalStep.

Removing each element of Sequence (as in my prototype) works, but is not too handy (especially as I don't know any other way to do it in this specific case than removing each element explicitly).

Of course if there is a better way to do the job than what I have in my prototype I'm all ears.

[Dr15Jones]

I'm thinking the ++ comes from either a module or a Sequence you added to iterTracking without also binding to process. I'll see if I can track it down and provide a meaningful way of preventing or at least catching such a problem.

[Dr15Jones]

It appears that the problem you were having stems from

process.load("RecoTracker.FinalTrackSelectors.Phase1PU70_preDuplicateMergingGeneralTracks_cfi") # too different from Run2 that patching with era makes no sense

I think the following should have worked

def _modifyForPhase1(process):
   from RecoTracker.FinalTrackSelectors.Phase1PU70_preDuplicateMergingGeneralTracks_cfi import preDuplicateMergingGeneralTracks as _phase1PU70Version
   process.preDuplicateMergingGeneralTracks = _phase1PU70Version

A better solution would be for me to add a replaceWith method in Modifier so that in RecoTracker.FinalTrackSelectors.preDuplicateMergingGeneralTracks_cfi you could have

_phase1PU70Version = ...
eras.phase1Pixel.replaceWith( preDuplicateMergingGeneralTracks, _phase1PU70Version)

[Dr15Jones]

#13198

[makortel]

I understood your suggestion such that instead of wrapping preDuplicateMergingGeneralTracks to a sequence and modifying the sequence, I should do

# in preDuplicateMergingGeneralTracks_cfi.py
def _modifyForPhase1(process):
    from RecoTracker.FinalTrackSelectors.Phase1PU70_preDuplicateMergingGeneralTracks_cfi import preDuplicateMergingGeneralTracks as _phase1PU70Version
    process.preDuplicateMergingGeneralTracks = _phase1PU70Version
... = eras.phase1Pixel.makeProcessModifier(_modifyForPhase1)

# in iterativeTk_cff.py
iterTracking = cms.Sequence(... + preDuplicateMergingGeneralTracks + ...)

def _modifyForPhase1(process):
    process.iterTracking = cms.Sequence()
    ...
    process.iterTracking += (... + process.preDuplicateMergingGeneralTracks + ...)

I tried that, and but the ++ is still there, and also the process.reconstruction still contains sequences from the original iterTracking. (I also tried to apply the same recipe to earlyGeneralTracks_cfi, but no luck).

Could it be that replacing process.iterTracking as a new sequence object makes process.reconstruction to expand the old iterTracking, and then when I load the Phase1PU70 version of TobTecStep the ++ appears (such that it gets triggered only when something imported by iterativeTk_cff has ProcessModifiers)? It is exactly TobTecStep that is missing in there.

[makortel]

Ok, I can give a try with that tomorrow :)

[Dr15Jones]

I'm not trying to put you on the spot. I'm very willing to "roll up my sleeves" and try myself. :). The concrete examples are very helpful for a place to start :).

[makortel]

Thanks. You should be able to repeat what I'm doing by taking CMSSW_8_0_X_2016-01-26-1100...makortel:phase1EraMinimal_v3_2 to any IB later than CMSSW_8_0_X_2016-01-26-1100. I'm using /afs/cern.ch/user/m/mkortela/public/phase1/step3_reco.py as the job configuration, but I guess that in any post-#12806 IB the step3 of runTheMatrix -w upgrade -l 10000.0 should do the job too.

[Dr15Jones]

I tried building the code with your changes but the shear number of packages that git cmd-merge-topic checked out seems to be it will not finish compiling before I go to bed tonight :).

[makortel]

Weird, makortel:phase1EraMinimal_v3_2 should not change that many thins other places depend on.

[makortel]

Based on your private suggestion, I decided to ditch the ProcessModifier approach in the phase1EraMinimal_v3_2, and made another prototype with toReplaceWith() CMSSW_8_0_X...makortel:phase1EraMinimal_v4 (it includes #13198 since there was no IB yet at the time, and then I was too lazy to rebase). I tested in CMSSW_8_0_X_2016-02-04-2300 with -w upgrade -l 10000 that there are no changes in the output (some plots show tiny numerical differences).

I feel this approach was much more straightforward to implement (and debug) than ProcessModifier, although it "pollutes" the module namespaces with Phase1PU70-specific stuff (TobTecStep being the worst https://github.com/makortel/cmssw/blob/790db6791248586df6b65c11b48dc103732e85d5/RecoTracker/IterativeTracking/python/TobTecStep_cff.py ). Thanks to toReplaceWith() I didn't have a need for Sequence.clear().

@rovere @VinInn please take a look this prototype too.

[Dr15Jones]

I've also been exploring not using ProcessModifier based on your initial prototype
Dr15Jones@3178b2b

I'm not all the way through the modifications but I thought people might be interested in the change. It is likely close to what Matti has done.

[Dr15Jones]

I did finish my version of the change
Dr15Jones@3d37b71

[makortel]

Thanks Chris. I see our final results are conceptually the same (with some differences in implementation).

Regarding the related issues #13210 (use dict to modify nested PSet parameters) would indeed be useful, and #13209 (use None to remove a parameter) would also be nice, but not strictly necessary. In my "v3" prototype I did delete certain parameters so they would get their default value, but the same effect can, in this specific case, be achieved by other means too.

The only remaining big question (to me) is then "with phase1Pixel era enabled, how can we run the run2 tracking on phase1 detector?". (as written earlier, I have some ideas for it, but now I leave only the question here since I'm not sure if this issue is the best place to discuss it)

[Dr15Jones]

-1
The Modifier.toReplaceWith can do the work and is more general.