improve `HLTMuonValidator` and `HLTMuonPlotter` (no UB, consider only `HLTFilter` modules) #40753

missirol · 2023-02-13T13:47:10Z

PR description:

This PR aims to improve the plugin HLTMuonValidator, and its utility HLTMuonPlotter.

This update was triggered by #40708 (comment), where it was noticed that HLTMuonPlotter has a bug that can lead to undefined behaviour [1].

Further checks showed other shortcomings of HLTMuonValidator/HLTMuonPlotter (see review comments for more details); these shortcomings can lead to the DQM outputs of HLTMuonValidator being filled incorrectly in the current implementation. This PR tries to fix this (on top of fixing the original UB issue).

LogTrace calls are added to ease debugging. In addition, technical changes to modernise both classes are applied (e.g. const correctness).

Expect changes in the DQM outputs of HLTMuonValidator (and nowhere else).

[1]
The value of hltStep becomes (size_t) -1 when step == 2 and nSteps == 6, leading to undefined behaviour in

cmssw/HLTriggerOffline/Muon/src/HLTMuonPlotter.cc

Line 221 in 6f5b833

if (matches[j].candHlt[hltStep] == nullptr)

This was originally reported in #12056, but apparently not fixed back then.

PR validation:

Private tests.

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:

To be backported to CMSSW_13_0_X if merged.

…Filter modules)

missirol

Review comments with further details on this PR.

@cms-sw/dqm-l2, please consider notifying the experts of this DQM plugin and/or the HLT-MUO validators, so they can contribute to the review.

missirol · 2023-02-13T13:50:41Z

HLTriggerOffline/Muon/src/HLTMuonValidator.cc


-vector<string> HLTMuonValidator::moduleLabels(string path) {
-  vector<string> modules = hltConfig_.moduleLabels(path);


moduleLabels is not sufficient in this case, as there are modules called *Filtered* (in muon-related triggers of the HLT menu) which do not produce any TriggerObjects for TriggerEventWithRefs (see for example the module hltIter0IterL3MuonPixelSeedsFromPixelTracksFiltered); such extra modules would be taken into account by HLTMuonPlotter, fail the matching to GEN/RECO objects (because they return no HLT candidates), and thus cause the following HLT steps to fail the matching (even if one of the following HLT filters does have valid HLT candidates).

saveTagsModules (see above in green) should be a better choice (saveTags is a parameter present for all the modules in the HLT menu which are HLTFilters, which do save the TriggerObjects).

missirol · 2023-02-13T13:51:46Z

HLTriggerOffline/Muon/src/HLTMuonValidator.cc

-  vector<string> modules = hltConfig_.moduleLabels(path);
-  vector<string>::iterator iter = modules.begin();
+    auto const step_label = stepLabel(module);
+    if (step_label.empty() or std::find(stepLabels.begin(), stepLabels.end(), step_label) != stepLabels.end())


This requires that there are no duplicates in the stepLabels vector. Afaiu, having duplicates could cause histograms of a certain type, e.g. _L3, to be filled more often than histograms of type _All (no matching requirements), and this could spoil the efficiency plots (numerator with more entries than denominator).

missirol · 2023-02-13T13:52:24Z

HLTriggerOffline/Muon/src/HLTMuonValidator.cc

+    return;
+  }
+
+  if (stepLabels[0] != "L1" and std::find(stepLabels.begin(), stepLabels.end(), "L1") != stepLabels.end()) {


This forbids cases where "L1" is present in stepLabels, but is not the first module found by HLTMuonValidator in the HLT Path (the case where "L1" is not present in stepLabels remains supported).

missirol · 2023-02-13T13:52:47Z

HLTriggerOffline/Muon/src/HLTMuonPlotter.cc

+    edm::LogError("HLTMuonPlotter") << "Invalid inputs: 'moduleLabels_' is empty."
+                                    << "\nMonitorElements for HLT path '" << hltPath_ << "' will not be produced.";
+    isInvalid_ = true;
+  } else if (stepLabels_.size() != moduleLabels_.size() + 1) {


This enforces the requirement that stepLabels_.size() == moduleLabels_.size() + 1 (the difference being the first entry of stepLabels_, which is "All", i.e. no trigger-matching requirements). This, stepLabels_.size() == moduleLabels_.size() + 1 is guaranteed by HLTMuonValidator::fillLabels.

missirol · 2023-02-13T13:53:54Z

HLTriggerOffline/Muon/src/HLTMuonPlotter.cc

+      auto const moduleLabelStripped =
+          edm::path_configuration::removeSchedulingTokensFromModuleLabel(moduleLabels_[idx]);
+      auto const iTag = edm::InputTag(moduleLabelStripped, "", hltProcessName_);


The module 'label' here (i.e. moduleLabels_[idx]) can contain special characters to denote how the module/filter was used in the configuration (e.g. moduleLabels_[idx] == "-hltModule" if the result of the filter was ignored, because used with cms.ignore( hltModule )). In order to find the corresponding trigger objects, the InputTag needs to be built with the 'stripped' version of the module label (meaning, without special characters).

cmsbuild · 2023-02-13T13:54:55Z

+code-checks

Logs: https://cmssdt.cern.ch/SDT/code-checks/cms-sw-PR-40753/34170

This PR adds an extra 20KB to repository

cmsbuild · 2023-02-13T13:55:25Z

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

It involves the following packages:

HLTriggerOffline/Muon (dqm)

@emanueleusai, @cmsbuild, @syuvivida, @rvenditti, @micsucmed, @pmandrik can you please review it and eventually sign? Thanks.
@mtosi, @andrea21z, @abbiendi, @Fedespring, @HuguesBrun, @jhgoh, @CeliaFernandez, @trocino, @cericeci 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-02-13T13:56:01Z

please test

missirol · 2023-02-13T13:57:53Z

type bugfix

missirol · 2023-02-13T13:59:39Z

HLTriggerOffline/Muon/python/hltMuonValidator_cfi.py

@@ -6,10 +6,10 @@

    hltProcessName = cms.string("HLT"),
    hltPathsToCheck = cms.vstring(
-        "HLT_(L[12])?(Iso)?(Tk)?Mu[0-9]*(Open)?(_NoVertex)?(_eta2p1)?(_v[0-9]*)?$",
+        "HLT_(HighPt)?(L[12])?(Iso)?(Tk)?Mu[0-9]*(Open)?(_NoVertex)?(_eta2p1)?(_v[0-9]*)?$",


CMSHLT-2277 renamed HLT_TkMu100 to HLT_HighPtTkMu100, and this change adds monitoring for the latter.

missirol · 2023-02-13T14:53:47Z

please abort

Unrelated error in unit test test-das-selected-lumis, so will restart the tests.

missirol · 2023-02-13T14:54:25Z

please test

cmsbuild · 2023-02-13T17:28:00Z

+1

Summary: https://cmssdt.cern.ch/SDT/jenkins-artifacts/pull-request-integration/PR-4366cc/30608/summary.html
COMMIT: 323bc9a
CMSSW: CMSSW_13_1_X_2023-02-13-1100/el8_amd64_gcc11
User test area: For local testing, you can use /cvmfs/cms-ci.cern.ch/week0/cms-sw/cmssw/40753/30608/install.sh to create a dev area with all the needed externals and cmssw changes.

Comparison Summary

Summary:

You potentially added 29 lines to the logs
Reco comparison results: 7 differences found in the comparisons
DQMHistoTests: Total files compared: 49
DQMHistoTests: Total histograms compared: 3555972
DQMHistoTests: Total failures: 1155
DQMHistoTests: Total nulls: 0
DQMHistoTests: Total successes: 3554795
DQMHistoTests: Total skipped: 22
DQMHistoTests: Total Missing objects: 0
DQMHistoSizes: Histogram memory added: 78.21 KiB( 48 files compared)
DQMHistoSizes: changed ( 11634.0,... ): 13.035 KiB HLT/Muon
Checked 213 log files, 164 edm output root files, 49 DQM output files
TriggerResults: no differences found

emanueleusai · 2023-02-14T10:11:54Z

@missirol do I understand correctly that HLTMuonValidator is responsible for producing all HLT_Mu* and HLT_IsoMu* plots in their respective folders? (e.g. /HLT/Muon/Distributions/HLT_Mu15)

missirol · 2023-02-14T10:21:12Z

@emanueleusai , yes (or at least, that's my understanding).

emanueleusai · 2023-02-14T10:35:20Z

@JanFSchulte @khaosmos93 Could you please help reviewing this PR? (couldn't find the guthub handle of the other HLT Mu DQM/Validation experts)

missirol · 2023-02-23T15:27:17Z

@cms-sw/dqm-l2

What's the ETA for the review of this bugfix ?

Do you (eventually) plan/want to have this backported to 13_0_X ?

emanueleusai · 2023-02-24T08:08:43Z

I was waiting for HLT Mu DQM/Validation expert feedback, but it looks like they're not interested in contributing to this.
So, as far as my understanding of the code goes, the code changes are ok. The differences in DQM comparison, while many, are understood and a direct consequences of the modification of HLTMuonValidator/HLTMuonPlotter.

emanueleusai · 2023-02-24T08:09:47Z

And, yes, we need the 13_0 backport as this is the release to be used for data taking.

emanueleusai · 2023-02-24T08:10:01Z

+1

see above

cmsbuild · 2023-02-24T08:10:14Z

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 · 2023-02-24T08:18:24Z

And, yes, we need the 13_0 backport as this is the release to be used for data taking.

Thanks, @emanueleusai . Okay, I'll go ahead and open the backport.

perrotta · 2023-02-24T08:28:22Z

+1

improve HLTMuonValidator and HLTMuonPlotter (no UB, consider only HLT…

323bc9a

…Filter modules)

cmsbuild added this to the CMSSW_13_1_X milestone Feb 13, 2023

cmsbuild added dqm-pending pending-signatures tests-pending orp-pending code-checks-pending labels Feb 13, 2023

missirol commented Feb 13, 2023

View reviewed changes

cmsbuild added code-checks-approved and removed code-checks-pending labels Feb 13, 2023

cmsbuild added tests-started and removed tests-pending labels Feb 13, 2023

cmsbuild added the bug-fix label Feb 13, 2023

missirol commented Feb 13, 2023

View reviewed changes

cmsbuild added tests-pending and removed tests-started labels Feb 13, 2023

cmsbuild added tests-started and removed tests-pending labels Feb 13, 2023

cmsbuild added tests-approved and removed tests-started labels Feb 13, 2023

cmsbuild added dqm-approved fully-signed and removed dqm-pending pending-signatures labels Feb 24, 2023

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

cmsbuild merged commit ef8d63d into cms-sw:master Feb 24, 2023

missirol mentioned this pull request Feb 24, 2023

improve HLTMuonValidator and HLTMuonPlotter (no UB, consider only HLTFilter modules) [13_0_X] #40871

Merged

missirol deleted the devel_fixUBInHLTMuonPlotter branch February 24, 2023 08:43

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

improve `HLTMuonValidator` and `HLTMuonPlotter` (no UB, consider only `HLTFilter` modules) #40753

improve `HLTMuonValidator` and `HLTMuonPlotter` (no UB, consider only `HLTFilter` modules) #40753

missirol commented Feb 13, 2023 •

edited

Loading

missirol left a comment •

edited

Loading

missirol Feb 13, 2023

missirol Feb 13, 2023

missirol Feb 13, 2023

missirol Feb 13, 2023

missirol Feb 13, 2023

cmsbuild commented Feb 13, 2023

cmsbuild commented Feb 13, 2023

missirol commented Feb 13, 2023

missirol commented Feb 13, 2023

missirol Feb 13, 2023

missirol commented Feb 13, 2023

missirol commented Feb 13, 2023

cmsbuild commented Feb 13, 2023

emanueleusai commented Feb 14, 2023

missirol commented Feb 14, 2023

emanueleusai commented Feb 14, 2023

missirol commented Feb 23, 2023

emanueleusai commented Feb 24, 2023

emanueleusai commented Feb 24, 2023

emanueleusai commented Feb 24, 2023

cmsbuild commented Feb 24, 2023

missirol commented Feb 24, 2023

perrotta commented Feb 24, 2023


		vector<string> HLTMuonValidator::moduleLabels(string path) {
		vector<string> modules = hltConfig_.moduleLabels(path);

improve HLTMuonValidator and HLTMuonPlotter (no UB, consider only HLTFilter modules) #40753

improve HLTMuonValidator and HLTMuonPlotter (no UB, consider only HLTFilter modules) #40753

Conversation

missirol commented Feb 13, 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 left a comment • edited Loading

Choose a reason for hiding this comment

missirol Feb 13, 2023

Choose a reason for hiding this comment

missirol Feb 13, 2023

Choose a reason for hiding this comment

missirol Feb 13, 2023

Choose a reason for hiding this comment

missirol Feb 13, 2023

Choose a reason for hiding this comment

missirol Feb 13, 2023

Choose a reason for hiding this comment

cmsbuild commented Feb 13, 2023

cmsbuild commented Feb 13, 2023

missirol commented Feb 13, 2023

missirol commented Feb 13, 2023

missirol Feb 13, 2023

Choose a reason for hiding this comment

missirol commented Feb 13, 2023

missirol commented Feb 13, 2023

cmsbuild commented Feb 13, 2023

Comparison Summary

emanueleusai commented Feb 14, 2023

missirol commented Feb 14, 2023

emanueleusai commented Feb 14, 2023

missirol commented Feb 23, 2023

emanueleusai commented Feb 24, 2023

emanueleusai commented Feb 24, 2023

emanueleusai commented Feb 24, 2023

cmsbuild commented Feb 24, 2023

missirol commented Feb 24, 2023

perrotta commented Feb 24, 2023

improve `HLTMuonValidator` and `HLTMuonPlotter` (no UB, consider only `HLTFilter` modules) #40753

improve `HLTMuonValidator` and `HLTMuonPlotter` (no UB, consider only `HLTFilter` modules) #40753

missirol commented Feb 13, 2023 •

edited

Loading

missirol left a comment •

edited

Loading