guard against no L1T uGT digis in `L1TriggerResultsConverter` #40528

missirol · 2023-01-16T10:42:39Z

PR description:

This PR fixes the edge case reported in #40494. It adds a check on the size of the L1T uGT digis, returning false for all L1T algorithms when said input collection has 0 elements for the relevant BX (see #40528 (comment) for details).

PR validation:

Fixes the crash reproduced from the instructions in #40494 (tested in 13_0_X):

cmsDriver.py RECO --conditions 124X_dataRun3_Prompt_v4 --customise Configuration/DataProcessing/Utils.addMonitoring \
--datatier NANOAOD --era Run3 --eventcontent NANOEDMAOD \
--filein /store/data/Run2022G/MinimumBias/MINIAOD/PromptReco-v1/000/362/859/00000/948464c4-75ad-41a3-95d0-82e31c865b10.root \
--fileout "file:ReReco-Run2022G-MinimumBias-PromptNanoAODv10_v1-00001.root" --nThreads 1 --number -1 \
--python_filename "ReReco-Run2022G-MinimumBias-PromptNanoAODv10_v1-00001_0_cfg.py" --scenario pp --step NANO --data

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:

TBD

missirol · 2023-01-16T10:44:07Z

type bugfix

cmsbuild · 2023-01-16T10:51:00Z

+code-checks

Logs: https://cmssdt.cern.ch/SDT/code-checks/cms-sw-PR-40528/33739

This PR adds an extra 12KB to repository

cmsbuild · 2023-01-16T10:51:23Z

A new Pull Request was created by @missirol (Marino Missiroli) for master.

It involves the following packages:

PhysicsTools/NanoAOD (xpog)

@cmsbuild, @swertz, @vlimant can you please review it and eventually sign? Thanks.
@gpetruc this is something you requested to watch as well.
@perrotta, @dpiparo, @rappoccio you are the release manager for this.

cms-bot commands are listed here

missirol · 2023-01-16T11:33:49Z

please test

cmsbuild · 2023-01-16T14:46:37Z

+1

Summary: https://cmssdt.cern.ch/SDT/jenkins-artifacts/pull-request-integration/PR-d919dc/30005/summary.html
COMMIT: 33f7784
CMSSW: CMSSW_13_0_X_2023-01-16-1100/el8_amd64_gcc11
User test area: For local testing, you can use /cvmfs/cms-ci.cern.ch/week0/cms-sw/cmssw/40528/30005/install.sh to create a dev area with all the needed externals and cmssw changes.

Comparison Summary

There are some workflows for which there are errors in the baseline:
11634.15 step 3
The results for the comparisons for these workflows could be incomplete
This means most likely that the IB is having errors in the relvals.The error does NOT come from this pull request

Summary:

No significant changes to the logs found
Reco comparison results: 8 differences found in the comparisons
DQMHistoTests: Total files compared: 49
DQMHistoTests: Total histograms compared: 3555538
DQMHistoTests: Total failures: 6
DQMHistoTests: Total nulls: 0
DQMHistoTests: Total successes: 3555510
DQMHistoTests: Total skipped: 22
DQMHistoTests: Total Missing objects: 0
DQMHistoSizes: Histogram memory added: 0.0 KiB( 48 files compared)
Checked 211 log files, 162 edm output root files, 49 DQM output files
TriggerResults: no differences found

missirol · 2023-01-19T17:00:17Z

@cms-sw/xpog-l2

Can you please review this PR?

vlimant · 2023-01-19T17:32:04Z

see comments above

missirol · 2023-01-19T17:35:49Z

there are no comments, maybe you didnt hit 'finish review'

vlimant · 2023-01-16T11:22:32Z

PhysicsTools/NanoAOD/plugins/L1TriggerResultsConverter.cc

  const std::vector<bool>* wordp = nullptr;
  bool unprefireable_bit = false;
  if (!legacyL1_) {
    const auto& resultsProd = iEvent.get(token_);
-    wordp = &resultsProd.at(0, 0).getAlgoDecisionFinal();
+    if (resultsProd.size() > 0) {


can it happened that resultsProd.size() is non zero and resultsProd.size(0) is ?

Good point.. I don't know (maybe @cms-sw/l1-l2 knows?), and I had the same doubt.

I used size to be consistent with

cmssw/PhysicsTools/NanoAOD/plugins/L1TriggerResultsConverter.cc

Line 137 in df24460

if (handleExtResults->size() != 0) {

and similar cases seen elsewhere

cmssw/HLTrigger/HLTfilters/plugins/HLTL1TSeed.cc

Line 479 in 7315768

if (uGtAlgoBlocks->size() == 0) {

but now I also see cases where the 0 is explicitly used

cmssw/DQM/HLTEvF/plugins/TriggerRatesMonitor.cc

Line 361 in 7315768

if (not bxvector.isEmpty(0)) {

I think the correct call is size(0) (actually, isEmpty(0) is sufficient and a bit cheaper), so I switched to that, for both resultsProd and handleExtResults.

cmsbuild · 2023-01-19T18:31:45Z

+code-checks

Logs: https://cmssdt.cern.ch/SDT/code-checks/cms-sw-PR-40528/33812

This PR adds an extra 12KB to repository

cmsbuild · 2023-01-19T18:32:06Z

Pull request #40528 was updated. @cmsbuild, @swertz, @vlimant can you please check and sign again.

missirol · 2023-01-19T19:57:45Z

please test

cmsbuild · 2023-01-19T22:24:24Z

+1

Summary: https://cmssdt.cern.ch/SDT/jenkins-artifacts/pull-request-integration/PR-d919dc/30085/summary.html
COMMIT: c3d66ef
CMSSW: CMSSW_13_0_X_2023-01-19-1100/el8_amd64_gcc11
User test area: For local testing, you can use /cvmfs/cms-ci.cern.ch/week0/cms-sw/cmssw/40528/30085/install.sh to create a dev area with all the needed externals and cmssw changes.

Comparison Summary

Summary:

No significant changes to the logs found
Reco comparison results: 12 differences found in the comparisons
DQMHistoTests: Total files compared: 49
DQMHistoTests: Total histograms compared: 3555479
DQMHistoTests: Total failures: 6
DQMHistoTests: Total nulls: 0
DQMHistoTests: Total successes: 3555451
DQMHistoTests: Total skipped: 22
DQMHistoTests: Total Missing objects: 0
DQMHistoSizes: Histogram memory added: 0.0 KiB( 48 files compared)
Checked 211 log files, 162 edm output root files, 49 DQM output files
TriggerResults: no differences found

missirol · 2023-01-23T07:22:53Z

@cms-sw/xpog-l2

Do you have other comments on this PR?

vlimant · 2023-01-24T16:12:11Z

+1

cmsbuild · 2023-01-24T16:12:32Z

This pull request is fully signed and it will be integrated in one of the next master IBs (tests are also fine). 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)

vlimant · 2023-01-24T16:12:54Z

a backport might be required, although data taking will move on to 13.0 AFAIK

missirol · 2023-01-24T16:23:03Z

a backport might be required, although data taking will move on to 13.0 AFAIK

The 12_6_X backport is in place, and tested, at #40588.

perrotta · 2023-01-24T16:53:01Z

+1

cmsbuild added this to the CMSSW_13_0_X milestone Jan 16, 2023

cmsbuild added code-checks-pending orp-pending pending-signatures tests-pending xpog-pending labels Jan 16, 2023

missirol mentioned this pull request Jan 16, 2023

check size of collection at BX=0 before getting the product #40527

Closed

cmsbuild added the bug-fix label Jan 16, 2023

cmsbuild added code-checks-approved and removed code-checks-pending labels Jan 16, 2023

cmsbuild added tests-started and removed tests-pending labels Jan 16, 2023

missirol mentioned this pull request Jan 16, 2023

Failure in 2022 data promptNano (L1) #40494

Closed

cmsbuild added tests-approved and removed tests-started labels Jan 16, 2023

vlimant reviewed Jan 19, 2023

View reviewed changes

guard against no L1T uGT digis in L1TriggerResultsConverter

c3d66ef

missirol force-pushed the devel_fix40494 branch from 33f7784 to c3d66ef Compare January 19, 2023 18:25

cmsbuild added code-checks-pending tests-pending and removed tests-approved code-checks-approved labels Jan 19, 2023

cmsbuild added code-checks-approved and removed code-checks-pending labels Jan 19, 2023

cmsbuild added tests-started and removed tests-pending labels Jan 19, 2023

cmsbuild added tests-approved and removed tests-started labels Jan 19, 2023

missirol mentioned this pull request Jan 23, 2023

guard against no L1T uGT digis in L1TriggerResultsConverter [12_6_X] #40588

Merged

cmsbuild added fully-signed xpog-approved and removed pending-signatures xpog-pending labels Jan 24, 2023

cmsbuild added orp-approved and removed orp-pending labels Jan 24, 2023

cmsbuild merged commit 3af08d7 into cms-sw:master Jan 24, 2023

missirol deleted the devel_fix40494 branch January 24, 2023 17:04

cmsbuild mentioned this pull request Jan 25, 2023

[BuildRules] Allow packages to export lib even if there are no cxx files cms-sw/cmsdist#8264

Merged

This was referenced Jan 31, 2023

guard against no L1T uGT digis in L1TriggerResultsConverter [12_5_X] #40650

Merged

guard against no L1T uGT digis in L1TriggerResultsConverter [12_4_X] #40651

Merged

improve check on size of L1-uGT digis in HLTL1TSeed plugin #40655

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

guard against no L1T uGT digis in `L1TriggerResultsConverter` #40528

guard against no L1T uGT digis in `L1TriggerResultsConverter` #40528

missirol commented Jan 16, 2023 •

edited

Loading

missirol commented Jan 16, 2023

cmsbuild commented Jan 16, 2023

cmsbuild commented Jan 16, 2023

missirol commented Jan 16, 2023

cmsbuild commented Jan 16, 2023

missirol commented Jan 19, 2023

vlimant commented Jan 19, 2023

missirol commented Jan 19, 2023

vlimant Jan 16, 2023

missirol Jan 19, 2023

cmsbuild commented Jan 19, 2023

cmsbuild commented Jan 19, 2023

missirol commented Jan 19, 2023

cmsbuild commented Jan 19, 2023

missirol commented Jan 23, 2023

vlimant commented Jan 24, 2023

cmsbuild commented Jan 24, 2023

vlimant commented Jan 24, 2023

missirol commented Jan 24, 2023

perrotta commented Jan 24, 2023

guard against no L1T uGT digis in L1TriggerResultsConverter #40528

guard against no L1T uGT digis in L1TriggerResultsConverter #40528

Conversation

missirol commented Jan 16, 2023 • edited Loading

PR description:

PR validation:

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:

missirol commented Jan 16, 2023

cmsbuild commented Jan 16, 2023

cmsbuild commented Jan 16, 2023

missirol commented Jan 16, 2023

cmsbuild commented Jan 16, 2023

Comparison Summary

missirol commented Jan 19, 2023

vlimant commented Jan 19, 2023

missirol commented Jan 19, 2023

vlimant Jan 16, 2023

Choose a reason for hiding this comment

missirol Jan 19, 2023

Choose a reason for hiding this comment

cmsbuild commented Jan 19, 2023

cmsbuild commented Jan 19, 2023

missirol commented Jan 19, 2023

cmsbuild commented Jan 19, 2023

Comparison Summary

missirol commented Jan 23, 2023

vlimant commented Jan 24, 2023

cmsbuild commented Jan 24, 2023

vlimant commented Jan 24, 2023

missirol commented Jan 24, 2023

perrotta commented Jan 24, 2023

guard against no L1T uGT digis in `L1TriggerResultsConverter` #40528

guard against no L1T uGT digis in `L1TriggerResultsConverter` #40528

missirol commented Jan 16, 2023 •

edited

Loading