-
Notifications
You must be signed in to change notification settings - Fork 4.3k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
guard HLTRecHitInAllL1RegionsProducer<T>
against empty collection of L1T candidates [13_0_X
]
#41467
guard HLTRecHitInAllL1RegionsProducer<T>
against empty collection of L1T candidates [13_0_X
]
#41467
Conversation
A new Pull Request was created by @missirol (Marino Missiroli) for CMSSW_13_0_X. It involves the following packages:
@cmsbuild, @missirol, @Martin-Grunewald can you please review it and eventually sign? Thanks. cms-bot commands are listed here |
type bugfix |
urgent This PR can reduce the number of HLT crashes seen online these days, so it should be integrated quickly. |
please test |
Hi @missirol, I wonder if only outputting a debug message here wouldn't hide serious issues? I may misunderstand, but my assumption is that the code that you're changing is only called if uGT claims that it triggered on an object of the given type. So if it claims that, but didn't correctly fill the input collection that points at a serious issue and we (at least L1T) should be alerted to it. My current feeling is that the BXVector shouldn't allow itself go be configured with zero entries. (It prohibits that in its initialiser, but one can reduce it to size zero afterwards by changing the BX range.. ) Cheers, |
Hi Dinyar, I think it's a fair question, and I don't have a strong opinion. Maybe E/gamma experts can give a more informed comment, since they know this plugin better than me. I decided to go for
Regarding
In practise (in a real HLT), I think this is true. In general (this plugin alone), I don't think it is necessarily the case, as the plugin itself makes no explicit requirements on the L1T-uGT decisions, it just tries to access the L1T objects.
While we can certainly change this [1]
|
Thank you @missirol |
Sounds good, thanks @perrotta. I will check with TSG how urgently they want to deploy this change.
Looking at the logs, yesterday in run 366821 (collisions) we had 986 crashes in the HLT farm. This PR should fix |
-1 Failed Tests: RelVals-INPUT RelVals-INPUT
Comparison SummarySummary:
|
+hlt
|
This pull request is fully signed and it will be integrated in one of the next CMSSW_13_0_X IBs (but tests are reportedly failing) and once validation in the development release cycle CMSSW_13_1_X is complete. This pull request will now be reviewed by the release team before it's merged. @perrotta, @dpiparo, @rappoccio (and backports should be raised in the release meeting by the corresponding L2) |
+1 |
merge |
backport of #41466
PR description:
This PR adds a check to
HLTRecHitInAllL1RegionsProducer<T>
to handle gracefully events where the input collection of L1T candidates is empty (either completely empty, or even empty just for BX=0).For such events, pre-PR the plugin can crash here. This type of crash was observed multiple times online in the last days. The root cause of the problem (i.e. missing L1T candidates) is likely related to issues with the L1T menu deployed a couple of weeks ago (CMSLITOPS-411).
A reproducer is in [1], and it produces the stack trace attached here (which matches the error log seen online).
FYI: @Sam-Harper @swagata87 (as this module is used by E/gamma triggers)
[1]
PR validation:
With the changes in this PR, the reproducer does not crash.
If this PR is a backport, please specify the original PR and why you need to backport that PR. If this PR will be backported, please specify to which release cycle the backport is meant for:
#41466
Bugfix to plugin used at HLT in release cycle used for data-taking in 2023.