Avoid sequence expansion in customizeHLTTrackingForPhaseI2017 #17406

makortel · 2017-02-03T09:18:19Z

The automatic expansion of sequences in customizeHLTTrackingForPhaseI2017.py after #17170 can, in some cases, lead to necessary modules missing in the sequences. While the full menu apparently works (since the 2017 workflows have run successfully after the integration of #17170), a recipe provided by @silviodonato fails

hltGetConfiguration /dev/CMSSW_9_0_0/HLT --path HLTFirstPath,HLT_PFHT125_v5,HLTEndPath \
  --globaltag  auto:phase1_2017_realistic \
  --input /store/relval/CMSSW_9_0_0_pre2/RelValTTbar_13/GEN-SIM-DIGI-RAW/PU25ns_90X_upgrade2017_realistic_v0_HLT2017Trk-v1/10000/0480A4C5-F7C2-E611-9FE0-0CC47A4D767A.root  \
  --no-output  > hlt1.py 

# in hlt1.py replace
# process = cms.Process( "HLTX")
# with
# from Configuration.StandardSequences.Eras import eras 
# process = cms.Process( "HLTX", eras.phase1Pixel)

This PR fixes the problem by avoiding the expansion of sequences caused by setattr(process, ...) by using a a cms.Modifier to replace modules/sequences "in place".

Tested in 9_0_0_pre3 (and CMSSW_9_0_X_2017-01-25-1100+#17170), the recipe above works again (as well as limited matrix, add addOnTests).

@silviodonato @Martin-Grunewald @mtosi @JanFSchulte

Expanding sequences containing the modules/sequences modified here causes the changes not to propagate everywhere (the expansion is done before the modification).

makortel · 2017-02-03T09:18:31Z

@cmsbuild, please test

cmsbuild · 2017-02-03T09:18:38Z

The tests are being triggered in jenkins.
https://cmssdt.cern.ch/jenkins/job/ib-any-integration/17608/console Started: 2017/02/03 10:20

cmsbuild · 2017-02-03T09:18:40Z

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

It involves the following packages:

HLTrigger/Configuration

@perrotta, @cmsbuild, @silviodonato, @Martin-Grunewald, @fwyzard, @davidlange6 can you please review it and eventually sign? Thanks.
@Martin-Grunewald, @geoff-smith, @jalimena this is something you requested to watch as well.
@davidlange6, @smuzaffar you are the release manager for this.

cms-bot commands are listed here #13028

makortel · 2017-02-03T09:23:33Z

HLTrigger/Configuration/python/customizeHLTTrackingForPhaseI2017.py

@@ -286,10 +295,10 @@ def _copy(old, new, skip=[]):
            if key not in skipSet:
                setattr(new, key, getattr(old, key))
    from RecoTracker.TkSeedGenerator.seedCreatorFromRegionConsecutiveHitsTripletOnlyEDProducer_cfi import seedCreatorFromRegionConsecutiveHitsTripletOnlyEDProducer as _seedCreatorFromRegionConsecutiveHitsTripletOnlyEDProducer
-    process.hltIter2PFlowPixelSeeds = _seedCreatorFromRegionConsecutiveHitsTripletOnlyEDProducer.clone(seedingHitSets="hltIter2PFlowPixelHitTriplets")
+    modifier.toReplaceWith(process.hltIter2PFlowPixelSeeds, _seedCreatorFromRegionConsecutiveHitsTripletOnlyEDProducer.clone(seedingHitSets="hltIter2PFlowPixelHitTriplets"))


This change alone would be sufficient to fix the @silviodonato's recipe, but I implemented/tested the wider solution first and it doesn't hurt (and IMHO makes edmConfigDumps easier to understand/correlate with configuration files/snippets as less sequences get expanded).

makortel · 2017-02-03T09:23:49Z

type bugfix

silviodonato · 2017-02-03T09:40:08Z

I tested again the full HLT menu using phase1 era in 900pre4.
I confirm that out of the box I get error.
I can also confirm that this PR solve the problem.

So I strongly suspect that the RelVals Phase1 900pre4 will crash because of HLT and we will need to include this PR to fix the issue.

makortel · 2017-02-03T09:45:57Z

(as I mentioned in the private e-mail thread) I'm puzzled how the "full menu" gives errors, 2017 workflows in IB/PR tests work fine.

davidlange6 · 2017-02-03T09:54:19Z

me too… .

…

On Feb 3, 2017, at 10:45 AM, Matti Kortelainen ***@***.***> wrote: (as I mentioned in the private e-mail thread) I'm puzzled how the "full menu" gives errors, 2017 workflows in IB/PR tests work fine. — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub, or mute the thread.

silviodonato · 2017-02-03T09:55:41Z

Me too. Among PR tests we have 10021.0, running:
step2 --conditions auto:phase1_2017_realistic -s DIGI:pdigi_valid,L1,DIGI2RAW,**HLT:@relval2016** --datatier GEN-SIM-DIGI-RAW -n 10 --geometry DB:Extended **--era Run2_2017** --eventcontent FEVTDEBUGHLT --filein file:step1.root --fileout file:step2.root

And it should catch the error, as HLT:@relval2016 is GRun (see 1) and GRun is /dev/CMSSW_9_0_0/GRun/V4, including the "modifyForEras" see 2

The only possible reason that I see is that the PR test run only in 10 events and we don't run the HLT PF tracking in none of them. I'm trying to understand it.

silviodonato · 2017-02-03T10:40:03Z

I've printed out the HLTTrackReconstructionForPF sequence (included in all HLT paths with PF) from the step2 configuration of 10021.0, used in the PR tests (see above).

The iter2 modules are: [...] + hltIter1TrackRefsForJets4Iter2 + hltAK4Iter1TrackJets4Iter2 + hltIter1TrackAndTauJets4Iter2 + hltIter2ClustersRefRemoval + hltIter2MaskedMeasurementTrackerEvent + hltIter2PixelLayerPairs + hltIter2PFlowPixelTrackingRegions + hltIter2PFlowPixelClusterCheck + hltIter2PFlowPixelHitDoublets + hltIter2PFlowPixelSeeds + hltIter2PFlowCkfTrackCandidates + hltIter2PFlowCtfWithMaterialTracks + hltIter2PFlowTrackCutClassifier + hltIter2PFlowTrackSelectionHighPurity + hltIter2Merged + [...].

As "hltIter2PFlowPixelHitDoublets" takes as InputTag "hltIter2PixelLayerTriplets" - but it is not run - the IB/PR tests should fail.
The only possible reason why it does not happen is that none of the 10 events generated pass the HLT cuts before HLT PF (I will check it).

cmsbuild · 2017-02-03T10:41:18Z

+1
Tested at: 4ded3bc
https://cmssdt.cern.ch/SDT/jenkins-artifacts/pull-request-integration/PR-17406/17608/summary.html

cmsbuild · 2017-02-03T10:41:21Z

Comparison job queued.

davidlange6 · 2017-02-03T10:41:45Z

what happens if you run more events? does it eventually crash?

…

On Feb 3, 2017, at 11:40 AM, Silvio Donato ***@***.***> wrote: I've printed out the HLTTrackReconstructionForPF sequence (included in all HLT paths with PF) from the step2 configuration of 10021.0, used in the PR tests (see above). The iter2 modules are: [...] + hltIter1TrackRefsForJets4Iter2 + hltAK4Iter1TrackJets4Iter2 + hltIter1TrackAndTauJets4Iter2 + hltIter2ClustersRefRemoval + hltIter2MaskedMeasurementTrackerEvent + hltIter2PixelLayerPairs + hltIter2PFlowPixelTrackingRegions + hltIter2PFlowPixelClusterCheck + hltIter2PFlowPixelHitDoublets + hltIter2PFlowPixelSeeds + hltIter2PFlowCkfTrackCandidates + hltIter2PFlowCtfWithMaterialTracks + hltIter2PFlowTrackCutClassifier + hltIter2PFlowTrackSelectionHighPurity + hltIter2Merged + [...]. As "hltIter2PFlowPixelHitDoublets" takes as InputTag "hltIter2PixelLayerTriplets" - but it is not run - the IB/PR tests should fail. The only possible reason why it does not happen is that none of the 10 events generated pass the HLT cuts before HLT PF (I will check it). — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub, or mute the thread.

makortel · 2017-02-03T10:55:41Z

The problem (or part of it) is that the hltIter2PixelLayerTriplets is missing from HLTTrackReconstructionForPF sequence (there can be others). But the module is correctly inserted in HLTIterativeTrackingIteration2 sequence (that is included in HLTTrackReconstructionForPF, but the it gets expanded before HLTIterativeTrackingIteration2 is corrected).

There seems to be also HLTIterativeTrackingIter02 sequence that also contains HLTIterativeTrackingIteration2, but does not get expanded. I see that some paths use HLTIterativeTrackingIter02. So one possibility is that in 1002[13456] workflows some other path runs hltIter2PixelLayerTriplets before a path with HLTTrackReconstructionForPF.

makortel · 2017-02-03T10:56:27Z

I ran 100 events in 10024 (with re-generated GEN-SIM), no crash. TrigReport shows that hltIter2PixelLayerTriplets was run 16 times.

silviodonato · 2017-02-03T11:04:26Z

Hi Matti,
indeed I was writing the same thing. I tried to run with 100 events, and I didn't get any error.
The reason is that HLTIterativeTrackingIter02 sequence (including hltIter2PixelLayerTriplets) is properly included in three HLT paths: MC_ReducedIterativeTracking_v3, HLT_MET60_IsoTrk35_Loose_v3, HLT_MET90_IsoTrk50_v6.
Hence, hltIter2PixelLayerTriplets is reconstructed by these HLT paths and we don't get error.
As MC_ReducedIterativeTracking_v3 is run over all events, it could happen then we will never get error with MC (it depends on the HLT path order).

silviodonato · 2017-02-03T11:12:36Z

+1

cmsbuild · 2017-02-03T11:12:53Z

This pull request is fully signed and it will be integrated in one of the next CMSSW_9_0_X IBs (tests are also fine). This pull request requires discussion in the ORP meeting before it's merged. @davidlange6, @smuzaffar

makortel · 2017-02-03T11:13:26Z

Thanks Silvio. Maybe part of the problem in your recipe is the use of 900pre2 RAW in a release where the auto-GT has an updated pixel cabling map, and HCAL geometry has changed. And by using only eras.phase1Pixel instead of eras.Run2_2017 (which leads to a fired assert in HCAL code), I have no idea what exactly the HCAL local reco does (for sure it is configured inconsistently wrt. RAW).

So maybe with 900pre2 RAW these paths fail before running the tracking? E.g. MC_ReducedIterativeTracking_v3 seems to include HLTRecoJetSequenceAK4PrePF (containing hltAK4CaloJetsPFEt5 filter) before tracking.

I'm still not sure though if we can really guarantee that all RelVals would succeed.

davidlange6 · 2017-02-03T11:19:43Z

HLTrigger/Configuration/python/customizeHLTTrackingForPhaseI2017.py

+    # In principle setattr(process) can be used to replace a
+    # module/sequence too, but it expands the sequences making life
+    # more complex
+    modifier = cms.Modifier()


HI @makortel - i really don't understand the role of modifier here. Its not the way they were designed to be used (but thats not to say we shouldn't use them this way - rather its hard to naively understand what happens..)

I fully agree this is a hack. But this is the simplest way I'm aware of to replace a module/sequence in a cms.Process without causing an expansion of all sequences including the module/sequence.

(ok, not so sure of "all", but at least some sequences get expanded)

i guess I'll need to understand more - but what is the lifetime of these customize functions rather than a contained menu?

@davidlange6 The discussion in these two issues #15425 and #15792 is related to automatic sequence expansion.

cmsbuild · 2017-02-03T12:24:58Z

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

fwyzard · 2017-02-03T13:25:38Z

hold

fwyzard · 2017-02-03T13:25:53Z

give me a couple of hours to underastand the issue...

cmsbuild · 2017-02-03T13:25:53Z

Pull request has been put on hold by @fwyzard
They need to issue an unhold command to remove the hold state or L1 can unhold it for all

fwyzard · 2017-02-03T15:29:33Z

(not) OK, I did not understand why one approach results in expanding the sequences and the other does not, so I simply reimplemented the Modifer.toModifyWith outside of Eras - see #17410 .

The logic is the same, the code is almost the same, the aim is to avoid confusion about the tools.

fwyzard · 2017-02-03T15:40:19Z

(still not) OK, I had even less expansion of Sequences after some more cleanup of HLTrigger/Configuration/python/customizeHLTTrackingForPhaseI2017.py - see #17412 .

@makortel what do you think ?

@silviodonato can you run the usual checks for those PRs ?

makortel · 2017-02-03T16:01:45Z

@fwyzard Do I understand correctly that the only change (wrt. this PR) relevant to the sequence expansion is at the end (loop over sequences and paths)?

My first (unsuccessful) fix attempt (to reduce expansion) was actually on that loop but in different direction 7ddd285. To my understanding the ModuleNodeVisitor visits all sub-sequences, and Sequence.remove() leads to expansion. In my commit the only the direct contents of a sequence are checked, so there could be less expansion with that approach. This is the very same approach I already used in HLTrigger/Configuration/python/customizeHLTforCMSSW.py#L321-L345.

makortel · 2017-02-03T16:06:33Z

Otherwise I'm not sure how beneficial it is copy the implementation of Modifier.toReplaceWith() (but I guess that's mostly for framework to comment). On the other hand, the added function fulfills perfectly the use case I described in #15425, so I don't object.

fwyzard · 2017-02-03T17:00:38Z

Do I understand correctly that the only change (wrt. this PR) relevant to the sequence expansion is at the end (loop over sequences and paths)?

The replace_with should give identical results to what you did - the only difference is that making it available as a free function instead of using the Modifer class might make it less confusing.

I did not try to make the change at the end without the one about replace_with - so I don't know if that might actually be enough.

My first (unsuccessful) fix attempt (to reduce expansion) was actually on that loop but in different direction 7ddd285. To my understanding the ModuleNodeVisitor visits all sub-sequences, and Sequence.remove() leads to expansion. In my commit the only the direct contents of a sequence are checked, so there could be less expansion with that approach. This is the very same approach I already used in HLTrigger/Configuration/python/customizeHLTforCMSSW.py#L321-L345.

I see. Probably the best approach would be check the Sequences "by hand" instead of using the visitor. In the early years of CMSSW using a visitor made senza, because the Sequence was implemented via recursion. Not it is implemented as a list, so IMHO a loop is a better interface.

Otherwise I'm not sure how beneficial it is copy the implementation of Modifier.toReplaceWith() (but I guess that's mostly for framework to comment). On the other hand, the added function fulfills perfectly the use case I described in #15425, so I don't object.

Ideally one would move replace_with to a central place (or as a method of Sequence) and change the Modifier class to use that.

silviodonato · 2017-02-03T17:00:48Z

@fwyzard I tested #17412 with GRun and it is fine. Now I'm running all the HLT tests.

Avoid sequence expansion in customizeHLTTrackingForPhaseI2017

4ded3bc

Expanding sequences containing the modules/sequences modified here causes the changes not to propagate everywhere (the expansion is done before the modification).

cmsbuild added this to the Next CMSSW_9_0_X milestone Feb 3, 2017

cmsbuild added comparison-pending hlt-pending orp-pending pending-signatures tests-pending labels Feb 3, 2017

cmsbuild added tests-started and removed tests-pending labels Feb 3, 2017

makortel commented Feb 3, 2017

View reviewed changes

cmsbuild added the bug-fix label Feb 3, 2017

cmsbuild added tests-approved and removed tests-started labels Feb 3, 2017

cmsbuild removed the hlt-pending label Feb 3, 2017

cmsbuild added fully-signed hlt-approved and removed pending-signatures labels Feb 3, 2017

davidlange6 reviewed Feb 3, 2017

View reviewed changes

cmsbuild added comparison-available and removed comparison-pending labels Feb 3, 2017

cmsbuild added hold pending-signatures and removed fully-signed labels Feb 3, 2017

davidlange6 closed this Feb 8, 2017

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Avoid sequence expansion in customizeHLTTrackingForPhaseI2017 #17406

Avoid sequence expansion in customizeHLTTrackingForPhaseI2017 #17406

makortel commented Feb 3, 2017

makortel commented Feb 3, 2017

cmsbuild commented Feb 3, 2017 •

edited

Loading

cmsbuild commented Feb 3, 2017

makortel Feb 3, 2017

makortel commented Feb 3, 2017

silviodonato commented Feb 3, 2017

makortel commented Feb 3, 2017

davidlange6 commented Feb 3, 2017 via email

silviodonato commented Feb 3, 2017

silviodonato commented Feb 3, 2017

cmsbuild commented Feb 3, 2017

cmsbuild commented Feb 3, 2017

davidlange6 commented Feb 3, 2017 via email

makortel commented Feb 3, 2017

makortel commented Feb 3, 2017

silviodonato commented Feb 3, 2017

silviodonato commented Feb 3, 2017

cmsbuild commented Feb 3, 2017

makortel commented Feb 3, 2017

davidlange6 Feb 3, 2017

makortel Feb 3, 2017

makortel Feb 3, 2017

davidlange6 Feb 3, 2017

makortel Feb 3, 2017

cmsbuild commented Feb 3, 2017

fwyzard commented Feb 3, 2017

fwyzard commented Feb 3, 2017

cmsbuild commented Feb 3, 2017

fwyzard commented Feb 3, 2017

fwyzard commented Feb 3, 2017

makortel commented Feb 3, 2017 •

edited

Loading

makortel commented Feb 3, 2017

fwyzard commented Feb 3, 2017

silviodonato commented Feb 3, 2017

Avoid sequence expansion in customizeHLTTrackingForPhaseI2017 #17406

Avoid sequence expansion in customizeHLTTrackingForPhaseI2017 #17406

Conversation

makortel commented Feb 3, 2017

makortel commented Feb 3, 2017

cmsbuild commented Feb 3, 2017 • edited Loading

cmsbuild commented Feb 3, 2017

makortel Feb 3, 2017

Choose a reason for hiding this comment

makortel commented Feb 3, 2017

silviodonato commented Feb 3, 2017

makortel commented Feb 3, 2017

davidlange6 commented Feb 3, 2017 via email

silviodonato commented Feb 3, 2017

silviodonato commented Feb 3, 2017

cmsbuild commented Feb 3, 2017

cmsbuild commented Feb 3, 2017

davidlange6 commented Feb 3, 2017 via email

makortel commented Feb 3, 2017

makortel commented Feb 3, 2017

silviodonato commented Feb 3, 2017

silviodonato commented Feb 3, 2017

cmsbuild commented Feb 3, 2017

makortel commented Feb 3, 2017

davidlange6 Feb 3, 2017

Choose a reason for hiding this comment

makortel Feb 3, 2017

Choose a reason for hiding this comment

makortel Feb 3, 2017

Choose a reason for hiding this comment

davidlange6 Feb 3, 2017

Choose a reason for hiding this comment

makortel Feb 3, 2017

Choose a reason for hiding this comment

cmsbuild commented Feb 3, 2017

fwyzard commented Feb 3, 2017

fwyzard commented Feb 3, 2017

cmsbuild commented Feb 3, 2017

fwyzard commented Feb 3, 2017

fwyzard commented Feb 3, 2017

makortel commented Feb 3, 2017 • edited Loading

makortel commented Feb 3, 2017

fwyzard commented Feb 3, 2017

silviodonato commented Feb 3, 2017

cmsbuild commented Feb 3, 2017 •

edited

Loading

makortel commented Feb 3, 2017 •

edited

Loading