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.

Sign up for GitHub

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

allow TriggerResultsFilter to throw for invalid HLTPathStatus patterns #39044

Merged
merged 1 commit into from
Aug 24, 2022
Merged

allow TriggerResultsFilter to throw for invalid HLTPathStatus patterns #39044

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
5 changes: 5 additions & 0 deletions HLTrigger/HLTcore/src/TriggerExpressionData.cc
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -48,11 +48,16 @@ namespace triggerExpression {
if (usePathStatus()) {
m_pathStatus.clear();
std::vector<std::string> triggerNames;
m_pathStatus.reserve(m_pathStatusTokens.size());
triggerNames.reserve(m_pathStatusTokens.size());
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 << "\"";
Copy link
Contributor Author

Choose a reason for hiding this comment

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

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

}
}
m_hltUpdated = m_triggerNames != triggerNames;
Expand Down
69 changes: 47 additions & 22 deletions HLTrigger/HLTfilters/plugins/TriggerResultsFilter.cc
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -10,17 +10,15 @@
#include <iomanip>
#include <iostream>
#include <sstream>
#include <string>
#include <regex>
#include <vector>
#include <algorithm>

#include "DataFormats/Common/interface/Handle.h"
#include "DataFormats/Common/interface/TriggerResults.h"
#include "FWCore/Framework/interface/ESHandle.h"
#include "FWCore/MessageLogger/interface/MessageLogger.h"
#include "FWCore/ParameterSet/interface/ConfigurationDescriptions.h"
#include "FWCore/Utilities/interface/Exception.h"
#include "FWCore/Utilities/interface/InputTag.h"
#include "FWCore/Utilities/interface/RegexMatch.h"
#include "FWCore/Utilities/interface/transform.h"
#include "HLTrigger/HLTcore/interface/TriggerExpressionEvaluator.h"
#include "HLTrigger/HLTcore/interface/TriggerExpressionParser.h"

Expand All @@ -34,30 +32,52 @@ TriggerResultsFilter::TriggerResultsFilter(const edm::ParameterSet& config)
std::vector<std::string> const& expressions = config.getParameter<std::vector<std::string>>("triggerConditions");
parse(expressions);
if (m_expression and m_eventCache.usePathStatus()) {
// if the expression was succesfully parsed, join all the patterns corresponding
// to the CMSSW paths in the logical expression into a single regex
std::vector<std::string> patterns = m_expression->patterns();
if (patterns.empty()) {
// if the expression was successfully parsed, store the list of patterns,
// each in both std::string and std::regex format
// and with a flag for having valid matches
hltPathStatusPatterns_ = edm::vector_transform(m_expression->patterns(), [](std::string const& pattern) {
return PatternData(pattern, std::regex(edm::glob2reg(pattern), std::regex::extended));
});

if (hltPathStatusPatterns_.empty()) {
return;
}
std::string str;
for (auto const& pattern : patterns) {
str += edm::glob2reg(pattern);
str += '|';
}
str.pop_back();
std::regex regex(str, std::regex::extended);

// consume all matching paths
callWhenNewProductsRegistered([this, regex](edm::BranchDescription const& branch) {
if (branch.branchType() == edm::InEvent and branch.className() == "edm::HLTPathStatus" and
std::regex_match(branch.moduleLabel(), regex)) {
m_eventCache.setPathStatusToken(branch, consumesCollector());
callWhenNewProductsRegistered([this](edm::BranchDescription const& branch) {
if (branch.branchType() == edm::InEvent and branch.className() == "edm::HLTPathStatus") {
bool consumeBranch = true;
for (auto& pattern : hltPathStatusPatterns_) {
if (std::regex_match(branch.moduleLabel(), pattern.regex)) {
pattern.matched = true;
if (consumeBranch) {
consumeBranch = false;
m_eventCache.setPathStatusToken(branch, consumesCollector());
}
}
}
}
});
}
}

void TriggerResultsFilter::beginStream(edm::StreamID) {
Copy link
Contributor Author

@missirol missirol Aug 12, 2022 •
edited
Loading

Choose a reason for hiding this comment

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

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.

// if throw=True, check if any of the input patterns had zero matches (and if so, throw an exception)
if (not hltPathStatusPatterns_.empty() and m_eventCache.shouldThrow()) {
auto unmatchedPatternsExist = std::any_of(
hltPathStatusPatterns_.cbegin(), hltPathStatusPatterns_.cend(), [](auto foo) { return (not foo.matched); });
if (unmatchedPatternsExist) {
cms::Exception excpt("UnmatchedPatterns");
excpt << "the parameter \"triggerConditions\" contains patterns with zero matches"
<< " for the available edm::HLTPathStatus collections - invalid patterns are:";
for (auto const& pattern : hltPathStatusPatterns_)
if (not pattern.matched)
excpt << "\n\t" << pattern.str;
throw excpt;
}
}
}

void TriggerResultsFilter::fillDescriptions(edm::ConfigurationDescriptions& descriptions) {
edm::ParameterSetDescription desc;
// # use HLTPathStatus results
Expand Down Expand Up @@ -107,8 +127,13 @@ void TriggerResultsFilter::parse(const std::string& expression) {
m_expression.reset(triggerExpression::parse(expression));

// check if the expressions were parsed correctly
if (not m_expression)
edm::LogWarning("Configuration") << "Couldn't parse trigger results expression \"" << expression << "\"";
if (not m_expression) {
if (m_eventCache.shouldThrow()) {
throw cms::Exception("Configuration") << "Couldn't parse trigger-results expression \"" << expression << "\"";
} else {
Comment on lines +131 to +133
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 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.

edm::LogWarning("Configuration") << "Couldn't parse trigger-results expression \"" << expression << "\"";
}
}
}

bool TriggerResultsFilter::filter(edm::Event& event, const edm::EventSetup& setup) {
Expand Down
14 changes: 13 additions & 1 deletion HLTrigger/HLTfilters/plugins/TriggerResultsFilter.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -17,12 +17,12 @@
#include <memory>
#include <string>
#include <vector>
#include <regex>

#include "FWCore/Framework/interface/Event.h"
#include "FWCore/Framework/interface/Frameworkfwd.h"
#include "FWCore/Framework/interface/stream/EDFilter.h"
#include "FWCore/ParameterSet/interface/ParameterSet.h"
#include "FWCore/Utilities/interface/InputTag.h"
#include "HLTrigger/HLTcore/interface/TriggerExpressionData.h"

// forward declaration
Expand All @@ -45,6 +45,8 @@ class TriggerResultsFilter : public edm::stream::EDFilter<> {
bool filter(edm::Event &, const edm::EventSetup &) override;

private:
void beginStream(edm::StreamID) override;

/// parse the logical expression into functionals
void parse(const std::string &expression);
void parse(const std::vector<std::string> &expressions);
Expand All @@ -54,6 +56,16 @@ class TriggerResultsFilter : public edm::stream::EDFilter<> {

/// cache some data from the Event for faster access by the m_expression
triggerExpression::Data m_eventCache;

struct PatternData {
PatternData(std::string const &aStr, std::regex const &aRegex, bool const hasMatch = false)
: str(aStr), regex(aRegex), matched(hasMatch) {}
std::string str;
std::regex regex;
bool matched;
};

std::vector<PatternData> hltPathStatusPatterns_;
};

#endif //TriggerResultsFilter_h
Loading