allow `TriggerResultsFilter` to throw for invalid `HLTPathStatus` patterns #39044

missirol · 2022-08-12T15:08:50Z

PR description:

This PR updates the TriggerResultsFilter plugin to spot patterns without any valid matches when HLTPathStatus collections are used (i.e. when usePathStatus=True).

Up to now throw=True had no effect if usePathStatus=True, even if triggerConditions contained patterns with no valid matches amongst the Paths in the job.

All TriggerResultsFilter instances in the central HLT menus use throw=True, and since many use usePathStatus=True, those could in principle silently return False due to invalid patterns (in practise, those patterns are often controlled by the ConfDB GUI, so should be correct).

After this PR, invalid patterns for HLTPathStatus collections will lead to an exception if throw=True.

While testing these changes, small updates were applied to the unit-tests for TriggerResultsFilter. In particular, TriggerResultsFilter modules with valid patterns now use throw=True, while those with invalid patterns are those with throw=False.

Edit : in addition, if throw=True, the plugin TriggerResultsFilter will now throw an exception if the parsing of the trigger-results expression fails (query with invalid syntax); up to now, this was leading only to a warning.

Thanks to @fwyzard for much help with the implementation; I tag him to review these changes.

PR validation:

Private tests with the relevant unit-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:

~~A backport to 12_4_X is not necessary, but might be useful.~~ (see #39044 (comment))

CMSSW_12_4_X

missirol · 2022-08-12T15:09:21Z

HLTrigger/HLTcore/src/TriggerExpressionData.cc

      for (auto const& p : m_pathStatusTokens) {
        auto const& handle = event.getHandle(p.second);
        if (handle.isValid()) {
          m_pathStatus.push_back(handle->accept());
          triggerNames.push_back(p.first);
+        } else {
+          edm::LogError("MissingHLTPathStatus")
+              << "invalid handle for requested edm::HLTPathStatus with label \"" << p.first << "\"";


I thought it was good to add here an error message. I don't know if it should even be promoted to an exception.

missirol · 2022-08-12T15:13:23Z

HLTrigger/HLTfilters/plugins/TriggerResultsFilter.cc

      }
    });
  }
 }

+void TriggerResultsFilter::beginStream(edm::StreamID) {


The check is done in beginStream, because the lambda passed via callWhenNewProductsRegistered is executed only after the module is constructed.

Core-sw experts clarified that the callback in callWhenNewProductsRegistered is applied serially to the different branches, so .matched was implemented as a simple bool, without resorting to atomic types.

missirol · 2022-08-12T15:14:03Z

HLTrigger/HLTfilters/test/triggerResultsFilter_by_PathStatus.py

-# load conditions from the global tag
-process.load('Configuration.StandardSequences.FrontierConditions_GlobalTag_cff')
-from Configuration.AlCa.GlobalTag import GlobalTag
-process.GlobalTag = GlobalTag(process.GlobalTag, 'auto:run2_mc', '')


In the unit-tests, the use of the GT is not strictly needed, so I thought it would be better to remove it.

cmsbuild · 2022-08-12T15:16:12Z

+code-checks

Logs: https://cmssdt.cern.ch/SDT/code-checks/cms-sw-PR-39044/31534

This PR adds an extra 20KB to repository

cmsbuild · 2022-08-12T15:16:34Z

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

It involves the following packages:

HLTrigger/HLTcore (hlt)
HLTrigger/HLTfilters (hlt)

@cmsbuild, @missirol, @Martin-Grunewald can you please review it and eventually sign? Thanks.
@Martin-Grunewald, @silviodonato, @fwyzard this is something you requested to watch as well.
@perrotta, @dpiparo, @qliphy, @rappoccio you are the release manager for this.

cms-bot commands are listed here

fwyzard · 2022-08-15T07:55:47Z

HLTrigger/HLTfilters/test/triggerResultsFilter_producer.py

+#process.MessageLogger.cerr.INFO = cms.untracked.PSet(
+#    reportEvery = cms.untracked.int32(1), # every!
+#    limit = cms.untracked.int32(-1)       # no limit!
+#    )
+#process.MessageLogger.cerr.FwkReport.reportEvery = 100 # only report every 100th event start
+#process.MessageLogger.cerr_stats.threshold = 'INFO' # also info in statistics


why these commented-out lines ?

In short, convenience. I used L10 to limit printouts while testing, and I thought it might be convenient to leave these lines there similarly to what is in one of the other configs, triggerResultsFilter_by_TriggerResults.py#L13.

I can remove them if you prefer, of course.

Quite the opposite... if they are useful for testing, you could also uncomment them, since this is a test file.
Would there be any downsides ?

I don't think so, let me have a look and update.

We could use the snippet below in the 3 configs for the unit-tests. What do you think?

With this, the event number is reported only 10 times instead of 1000, warnings/errors and the TrigReport messages are reported. The commented-out lines enable all info-level outputs for every event, and are left there for debugging.

process.options.wantSummary = True process.load('FWCore.MessageService.MessageLogger_cfi') process.MessageLogger.cerr.FwkReport.reportEvery = 100 # only report every 100th event start process.MessageLogger.cerr.enableStatistics = False # enable "MessageLogger Summary" message process.MessageLogger.cerr.threshold = 'INFO' # change to 'WARNING' not to show INFO-level messages ## enable reporting of INFO-level messages (default is limit=0, i.e. no messages reported) #process.MessageLogger.cerr.INFO = cms.untracked.PSet( # reportEvery = cms.untracked.int32(1), # every event! # limit = cms.untracked.int32(-1) # no limit! #)

cmsbuild · 2022-08-23T22:53:40Z

+code-checks

Logs: https://cmssdt.cern.ch/SDT/code-checks/cms-sw-PR-39044/31745

This PR adds an extra 20KB to repository

cmsbuild · 2022-08-23T22:54:07Z

Pull request #39044 was updated. @cmsbuild, @missirol, @Martin-Grunewald can you please check and sign again.

missirol · 2022-08-23T22:54:54Z

type bugfix

A small fix, but a fix nonetheless. TSG asked to eventually backport this to 12_4_X.

missirol · 2022-08-23T22:56:57Z

HLTrigger/HLTfilters/plugins/TriggerResultsFilter.cc

+    if (m_eventCache.shouldThrow()) {
+      throw cms::Exception("Configuration") << "Couldn't parse trigger-results expression \"" << expression << "\"";
+    } else {


This is a new addition: I would say TriggerResultsFilter should throw an exception (not just a warning) if throw=True and the parsing of the expression fails.

missirol · 2022-08-23T22:59:22Z

please test

The last push

rebases the PR after the integration of fix for NOT operator in triggerExpression::Parser #39108,
includes the update to test cfgs discussed in allow TriggerResultsFilter to throw for invalid HLTPathStatus patterns #39044 (comment),
and adds one additional update discussed in allow TriggerResultsFilter to throw for invalid HLTPathStatus patterns #39044 (comment).

cmsbuild · 2022-08-24T06:32:22Z

+1

Summary: https://cmssdt.cern.ch/SDT/jenkins-artifacts/pull-request-integration/PR-5b5fbc/27036/summary.html
COMMIT: e355f21
CMSSW: CMSSW_12_5_X_2022-08-23-2300/el8_amd64_gcc10
User test area: For local testing, you can use /cvmfs/cms-ci.cern.ch/week1/cms-sw/cmssw/39044/27036/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: 0 differences found in the comparisons
DQMHistoTests: Total files compared: 51
DQMHistoTests: Total histograms compared: 3693084
DQMHistoTests: Total failures: 2
DQMHistoTests: Total nulls: 0
DQMHistoTests: Total successes: 3693060
DQMHistoTests: Total skipped: 22
DQMHistoTests: Total Missing objects: 0
DQMHistoSizes: Histogram memory added: 0.0 KiB( 50 files compared)
Checked 212 log files, 49 edm output root files, 51 DQM output files
TriggerResults: no differences found

missirol · 2022-08-24T08:44:37Z

+hlt

updates TriggerResultsFilter to throw an exception as expected (if throw=True) if usePathStatus=True and the specified trigger-results patterns have no matches
in addition, if throw=True, TriggerResultsFilter will now throw an exception if the parsing of the trigger-results expression fails (up to now, this only led to a warning)

cmsbuild · 2022-08-24T08:45:03Z

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, @qliphy, @rappoccio (and backports should be raised in the release meeting by the corresponding L2)

perrotta · 2022-08-24T09:15:37Z

+1

cmsbuild added this to the CMSSW_12_5_X milestone Aug 12, 2022

cmsbuild added code-checks-pending hlt-pending orp-pending pending-signatures tests-pending labels Aug 12, 2022

missirol commented Aug 12, 2022

View reviewed changes

cmsbuild added code-checks-approved and removed code-checks-pending labels Aug 12, 2022

fwyzard reviewed Aug 15, 2022

View reviewed changes

missirol mentioned this pull request Aug 19, 2022

fix for NOT operator in triggerExpression::Parser #39108

Merged

allow TriggerResultsFilter to throw for invalid HLTPathStatus patterns

e355f21

missirol force-pushed the devel_hltTriggerResultsFilterThrow branch from 60ac149 to e355f21 Compare August 23, 2022 22:45

cmsbuild added code-checks-pending and removed code-checks-approved labels Aug 23, 2022

cmsbuild added code-checks-approved and removed code-checks-pending labels Aug 23, 2022

cmsbuild added the bug-fix label Aug 23, 2022

missirol commented Aug 23, 2022

View reviewed changes

cmsbuild added tests-started and removed tests-pending labels Aug 23, 2022

cmsbuild added tests-approved and removed tests-started labels Aug 24, 2022

cmsbuild added fully-signed hlt-approved and removed hlt-pending pending-signatures labels Aug 24, 2022

cmsbuild added orp-approved and removed orp-pending labels Aug 24, 2022

cmsbuild merged commit 6b34888 into cms-sw:master Aug 24, 2022

missirol deleted the devel_hltTriggerResultsFilterThrow branch August 24, 2022 12:35

missirol mentioned this pull request Aug 24, 2022

allow TriggerResultsFilter to throw for invalid HLTPathStatus patterns [12_4_X] #39175

Merged

This was referenced Aug 24, 2022

DiMuonMassBiasClient: add protection for empty input histogram in the fitting function #39173

Merged

[GCC][PATCH] libstdc++: Mark non-exported function always_inline cms-sw/cmsdist#8039

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

allow `TriggerResultsFilter` to throw for invalid `HLTPathStatus` patterns #39044

allow `TriggerResultsFilter` to throw for invalid `HLTPathStatus` patterns #39044

missirol commented Aug 12, 2022 •

edited

missirol Aug 12, 2022

missirol Aug 12, 2022 •

edited

missirol Aug 12, 2022

cmsbuild commented Aug 12, 2022

cmsbuild commented Aug 12, 2022

fwyzard Aug 15, 2022

missirol Aug 15, 2022

fwyzard Aug 15, 2022

missirol Aug 15, 2022

missirol Aug 15, 2022

cmsbuild commented Aug 23, 2022

cmsbuild commented Aug 23, 2022

missirol commented Aug 23, 2022

missirol Aug 23, 2022

missirol commented Aug 23, 2022

cmsbuild commented Aug 24, 2022

missirol commented Aug 24, 2022

cmsbuild commented Aug 24, 2022

perrotta commented Aug 24, 2022

allow TriggerResultsFilter to throw for invalid HLTPathStatus patterns #39044

allow TriggerResultsFilter to throw for invalid HLTPathStatus patterns #39044

Conversation

missirol commented Aug 12, 2022 • edited

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:

Choose a reason for hiding this comment

missirol Aug 12, 2022 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

cmsbuild commented Aug 12, 2022

cmsbuild commented Aug 12, 2022

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

cmsbuild commented Aug 23, 2022

cmsbuild commented Aug 23, 2022

missirol commented Aug 23, 2022

Choose a reason for hiding this comment

missirol commented Aug 23, 2022

cmsbuild commented Aug 24, 2022

Comparison Summary

missirol commented Aug 24, 2022

cmsbuild commented Aug 24, 2022

perrotta commented Aug 24, 2022

allow `TriggerResultsFilter` to throw for invalid `HLTPathStatus` patterns #39044

allow `TriggerResultsFilter` to throw for invalid `HLTPathStatus` patterns #39044

missirol commented Aug 12, 2022 •

edited

missirol Aug 12, 2022 •

edited