[muon-bugfix] disabled trigger matching in ZmmSkim #23710
Conversation
…become necessary, one needs to update the cmsDriver customization that overrides HLT process, so that it all works properly for reHLT
|
The code-checks are being triggered in jenkins. |
|
+code-checks Logs: https://cmssdt.cern.ch/SDT/code-checks/cms-sw-PR-23710/5360 |
|
A new Pull Request was created by @drkovalskyi for master. It involves the following packages: DPGAnalysis/Skims @GurpreetSinghChahal, @cmsbuild, @prebello, @fabozzi can you please review it and eventually sign? Thanks. cms-bot commands are listed here |
|
I see patMuons in many more places in the release |
|
@cmsbuild please test |
|
The tests are being triggered in jenkins. |
|
@drkovalskyi @slava77 this will work, I tried it myself this morning. But is this really a fix? The idea is that the module works if there is some appropriate information available, and in that workflow this is not true, so in a sense it is a fix. But I agree with Slava that this should be adequately protected |
|
reHLT for patMuons is fixed in 2fdbe18 |
|
On 6/28/18 2:22 PM, drkovalskyi wrote:
reHLT for patMuons is fixed in 2fdbe18
<2fdbe18>
Only clones are affected since no one sets properly HLT process for
them. So do you propose to disable trigger matching in the clones that
are not used in any of the tests? Isn't it extreme conner case that no
one really cares about?
I just want to avoid 10 more PRs which will fix crashes in other places
one by one when they are discovered incrementally
|
|
@fabiocos this can only work if all producers use the same HLT process name. By default it's set to HLT and we override it in camDriver command where it's set consistent right now for patTrigger, slimmedPatTrigger and patMuons. Any clones of these producers will use a wrong HLT process for reHLT. Do we really won't to pollute Configuration/Applications/python/ConfigBuilder.py with all the clones regardless if they need HLT or not? For ZmmSkim I know that we don't need it. |
Me too, but we need to converge on a specific solution. I can find all clones and disable trigger matching for them. Putting them all in the ConfigBuilder is overkill in my opinion. |
|
On 6/28/18 2:33 PM, drkovalskyi wrote:
I just want to avoid 10 more PRs which will fix crashes in other places
one by one when they are discovered incrementally
Me too, but we need to converge on a specific solution. I can find all
clones and disable trigger matching for them. Putting them all in the
ConfigBuilder is overkill in my opinion.
It's possible not to pollute the ConfigBuilder substantially by looping
over all producers with type PATMuonProducer and replace the parameter
there consistently for all.
|
Where? Only ConfigBuilder knows for sure what the user wants, i.e. whether they want HLT or reHLT process. To make a loop there would be a super ugly thing, i.e. the code stored as text to insert code do the change. This won't fix all possible conner cases. |
|
On 6/28/18 2:49 PM, drkovalskyi wrote:
It's possible not to pollute the ConfigBuilder substantially by looping
over all producers with type PATMuonProducer and replace the parameter
there consistently for all.
Where? Only ConfigBuilder knows for sure what the user wants, i.e.
whether they want HLT or reHLT process. To make a loop there would be a
super ugly thing, i.e. the code stored as text to insert code do the
change. This won't fix all possible conner cases.
I can disable matching by default and enable it only for patMuons. This
probably is the safest solution. Do you agree?
I'm not sure what's implied here.
It sounds like the configBuilder will have to set the parameter value to
True to make this work.
This doesn't sound like a good solution.
|
|
note that there is a function in ConfigBuilder that is meant to replace the HLT process name appropriately. (so the pollution is already there). It could be used in this case - eg
https://github.com/cms-sw/cmssw/blob/master/Configuration/Applications/python/ConfigBuilder.py#L1259
|
|
I'm saying I'll set matching OFF by default and re-enable it only for patMuons. In this case no other changes will be needed on top what we already have. Clones will be safe. customizeHLTforCMSSW is not a solution unless it's used for patTrigger, slimmedPatTrigger and patMuons to guarantee consistency and it's not the case at the moment. The failure is caused by inconsistency caused by partial change of the HLT process in ConfigBuilder. |
It's already used for patMuons: 2fdbe18 |
|
as we are already too late for pre6, I think that we need to converge in a safe way by this afternoon. So I suggest:
Then one can test with more calm the best solution |
|
I think disabling it by default and re-enabling only for patMuons is safe and final. By if you want to simply disable it it's certainly safe. So what do we do? |
|
let's disable it globally in muonProducer. Then after pre6 you may check with calm for the most robust solution to switch it on properly where needed |
|
in this way the code will stay available in the release, but it will not harm for the time being |
|
Ok. |
|
-1 Tested at: 07cefc5 You can see the results of the tests here: I found follow errors while testing this PR Failed tests: RelVals
The relvals timed out after 2 hours. |
|
Comparison not run due to runTheMatrix errors (RelVals and Igprof tests were also skipped) |
cmsbuild
added
comparison-notrun
tests-rejected
and removed
comparison-pending
tests-started
labels
This bug fix is for the issue reported in #23704. The fix disables the trigger matching in ZmmSkim, which is not properly configured for reHLT. No trigger matching is needed for the skim, so it's better to disable it rather than pollute the customization file to overrider the HLT process name for reHLT workflows.