Skip to content
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 TriggerRatesMonitor against unavailable trigger paths #40439

Merged

Conversation

missirol
Copy link
Contributor

@missirol missirol commented Jan 6, 2023

PR description:

This PR includes a small improvement to the plugin TriggerRatesMonitor, in order to support configurations using it with a subset of Paths of a HLT menu. Details on the original issue can be found in CMSHLT-2577.

Minor unrelated changes to the plugin in question are also included.

No changes expected in the outputs of PR tests.

PR validation:

Private tests, e.g.

#!/bin/bash

# tested in CMSSW_13_0_X_2023-01-05-1100

hltGetConfiguration run:360991 \
  --globaltag 124X_dataRun3_HLT_v4 \
  --input root://eoscms.cern.ch//store/data/Run2022F/HLTPhysics/RAW/v1/000/360/991/00000/69592673-b357-4478-b6c8-fb4fb2bcb980.root \
  --paths HLTrigger*,HLT_PFMETNoMu110_* \
  --output minimal \
  > hlt.py

cat <<@EOF >> hlt.py
process.hltLumiMonitor.folderName = process.hltLumiMonitor.FolderName
del process.hltLumiMonitor.FolderName
del process.hltEcalUncalibRecHitGPU.maxNumberHitsEB
del process.hltEcalUncalibRecHitGPU.maxNumberHitsEE
del process.hltHbherecoGPU.maxChannels
process.MessageLogger.cerr.threshold = 'DEBUG'
process.MessageLogger.debugModules = ['hltTriggerRatesMonitor']
@EOF

cmsRun hlt.py &> hlt.log

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:

N/A

Comment on lines +191 to +193
auto const triggerIdx = histograms.hltConfig.triggerIndex(path);
if (triggerIdx < nTriggers)
histograms.datasets[i].push_back(triggerIdx);
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This (i.e. the check on < nTriggers) is the change to fix the error reported in CMSHLT-2577. A similar check can be found in

auto const index = rundata->hltConfig.triggerIndex(datasetPathName);
if (index < triggerNamesSize)
dataset.push_back(index);

The rest of the changes in this PR is about adding a few checks, plus minor cleanup.

Comment on lines +210 to +212
auto const triggerIdx = histograms.hltConfig.triggerIndex(path);
if (triggerIdx < nTriggers)
histograms.streams[i].push_back(triggerIdx);
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This (i.e. the check on < nTriggers) is the change to fix the error reported in CMSHLT-2577. A similar check can be found in

auto const index = rundata->hltConfig.triggerIndex(datasetPathName);
if (index < triggerNamesSize)
dataset.push_back(index);

The rest of the changes in this PR is about adding a few checks, plus minor cleanup.

@cmsbuild
Copy link
Contributor

cmsbuild commented Jan 6, 2023

+code-checks

Logs: https://cmssdt.cern.ch/SDT/code-checks/cms-sw-PR-40439/33579

  • This PR adds an extra 16KB to repository

@cmsbuild
Copy link
Contributor

cmsbuild commented Jan 6, 2023

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

It involves the following packages:

  • DQM/HLTEvF (dqm, hlt)

@Martin-Grunewald, @emanueleusai, @ahmad3213, @cmsbuild, @missirol, @syuvivida, @pmandrik, @micsucmed, @rvenditti can you please review it and eventually sign? Thanks.
@silviodonato, @mtosi 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
Copy link
Contributor Author

missirol commented Jan 6, 2023

please test with #40334

Since I doubt that anything in the PR tests uses TriggerRatesMonitor, I'm first testing this with #40334, as the latter includes at least 1 unit test with that plugin. To be clear, this PR does not depend on #40334. Later on, I will test this PR without #40334.


unsigned int datasets = histograms.hltConfig.datasetNames().size();
unsigned int const datasets = histograms.hltConfig.datasetNames().size();
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Perhaps, in analogy to nTriggers here above, you can profit of this PR and rename this size as nDatasets: as such you'd also aviod giving tha same name to a (vector) class member and to this size, which can be confusing

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@perrotta
Copy link
Contributor

perrotta commented Jan 6, 2023

Later on, I will test this PR without #40334.

Unless you really want to test this PR standalpne, I don't see the reason to test it twice

@missirol
Copy link
Contributor Author

missirol commented Jan 6, 2023

Unless you really want to test this PR standalpne, I don't see the reason to test it twice

The 2nd test would just be a confirmation that this PR could be merged before #40334. If this is obvious to all, I'm happy not to test twice.

@missirol
Copy link
Contributor Author

missirol commented Jan 6, 2023

test parameters:

@missirol
Copy link
Contributor Author

missirol commented Jan 6, 2023

please test

Testing with #40334 to exploit a unit test in the latter, but this PR can be merged regardless of #40334.

@cmsbuild
Copy link
Contributor

cmsbuild commented Jan 6, 2023

+code-checks

Logs: https://cmssdt.cern.ch/SDT/code-checks/cms-sw-PR-40439/33586

  • This PR adds an extra 16KB to repository

@cmsbuild
Copy link
Contributor

cmsbuild commented Jan 6, 2023

Pull request #40439 was updated. @emanueleusai, @ahmad3213, @Martin-Grunewald, @missirol, @syuvivida, @pmandrik, @micsucmed, @rvenditti can you please check and sign again.

@cmsbuild
Copy link
Contributor

cmsbuild commented Jan 6, 2023

+1

Summary: https://cmssdt.cern.ch/SDT/jenkins-artifacts/pull-request-integration/PR-5dc01f/29816/summary.html
COMMIT: 94344d9
CMSSW: CMSSW_13_0_X_2023-01-06-1100/el8_amd64_gcc11
User test area: For local testing, you can use /cvmfs/cms-ci.cern.ch/week0/cms-sw/cmssw/40439/29816/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: 29 differences found in the comparisons
  • DQMHistoTests: Total files compared: 49
  • DQMHistoTests: Total histograms compared: 3555748
  • DQMHistoTests: Total failures: 1194
  • DQMHistoTests: Total nulls: 0
  • DQMHistoTests: Total successes: 3554532
  • 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

@emanueleusai
Copy link
Member

+1

  • technical PR
  • wf 11634.7 differences not connected to this PR

@missirol
Copy link
Contributor Author

missirol commented Jan 7, 2023

+hlt

@cmsbuild
Copy link
Contributor

cmsbuild commented Jan 7, 2023

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)

@missirol
Copy link
Contributor Author

missirol commented Jan 7, 2023

test parameters:

@perrotta
Copy link
Contributor

perrotta commented Jan 7, 2023

+1

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants