fix for `NOT` operator in `triggerExpression::Parser` #39108

missirol · 2022-08-19T10:20:14Z

PR description:

The NOT operator of the parser in the triggerExpression namespace does not work correctly in recent CMSSW releases.

This PR fixes that. All credit to @fwyzard for finding the solution.

The issue was that the parser was mistakenly interpreting "NOT" as the name of an HLT path (parsing it into a TriggerExpressionPathReader object).

Unit tests are improved in order to catch issues with the parser, should they come up in the future:

a unit test with syntax checks on the parser is added to HLTcore;
unit tests related to TriggerResultsFilters in HLTfilters are updated, including a numerical check on the trigger results of the cmsRun jobs.

(The above tests can be used to verify that without this PR the NOT operator would not work correctly in recent CMSSW releases.)

Context:

the triggerExpression parser is used in the TriggerResultsFilter plugin, many instances of which exist in the HLT menus;
at the same time, in recent menus none of these instances use the NOT operator, so this fix will have no impact on any current production use case;
still, I think it would be best to backport this fix to 12_4_X.

This PR has a small overlap with #39044, and takes precedence over it.

PR validation:

Relies on the updated 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:

CMSSW_12_4_X

missirol · 2022-08-19T10:21:54Z

HLTrigger/HLTcore/interface/TriggerExpressionParser.h

@@ -2,7 +2,7 @@
 #define HLTrigger_HLTfilters_TriggerExpressionParser_h

 // Note: this requires Boost 1.41 or higher, for Spirit 2.1 or higher
-#include <boost/spirit/include/phoenix.hpp>
+#include <boost/phoenix.hpp>


This change quiets the following compile-time warning:

In file included from /cvmfs/cms.cern.ch/slc7_amd64_gcc10/external/boost/1.78.0-12075919175e8d078539685f9234134a/include/boost/config/header_deprecated.hpp:18, from /cvmfs/cms.cern.ch/slc7_amd64_gcc10/external/boost/1.78.0-12075919175e8d078539685f9234134a/include/boost/spirit/include/phoenix.hpp:11, from CMSSW_12_5_0_pre4/src/HLTrigger/HLTcore/interface/TriggerExpressionParser.h:5, from CMSSW_12_5_0_pre4/src/HLTrigger/HLTcore/test/test_catch2_TriggerExpressionParser.cc:7: /cvmfs/cms.cern.ch/slc7_amd64_gcc10/external/boost/1.78.0-12075919175e8d078539685f9234134a/include/boost/spirit/include/phoenix.hpp:12:1: note: '#pragma message: This header is deprecated. Use <boost/phoenix.hpp> instead.' 12 | BOOST_HEADER_DEPRECATED("<boost/phoenix.hpp>") | ^~~~~~~~~~~~~~~~~~~~~~~

It seemed safe to do, but I'm not 100% sure. @fwyzard , what do you think?

If it works and silences the warnings, go for it

missirol · 2022-08-19T10:22:50Z

HLTrigger/HLTcore/interface/TriggerExpressionParser.h

@@ -37,7 +37,7 @@ namespace triggerExpression {

      operand %= (prescale | element);

-      unary = (operand[qi::_val = qi::_1] | (qi::lit("NOT") >> operand)[qi::_val = new_<OperatorNot>(qi::_1)]);
+      unary = ((qi::lit("NOT") >> operand)[qi::_val = new_<OperatorNot>(qi::_1)] | operand[qi::_val = qi::_1]);


This 1-line change is the fix.

missirol · 2022-08-19T10:23:14Z

HLTrigger/HLTcore/test/test_catch2_TriggerExpressionParser.cc

+  SECTION("CorrectExpressions") {
+    REQUIRE(testExpression("TRUE"));
+    REQUIRE(testExpression("FALSE"));
+    REQUIRE(testExpression("NOT (FALSE)"));
+    REQUIRE(testExpression("(NOT FALSE) OR TRUE"));
+    REQUIRE(testExpression("NOTThisHLTPath AND TRUE AND NOT L1_A?_*"));
+    REQUIRE(testExpression("NOT NOTThisHLTPath"));
+    REQUIRE(testExpression("ThisHLTANDNOTThatORTheOther"));
+    REQUIRE(testExpression("NOT L1_SEED1 AND L1_SEED2*"));
+    REQUIRE(testExpression("NOT L1_SEED2 AND (HLT_PATH_? AND NOT HLT_PATH2_??_*)"));
+    REQUIRE(testExpression("NOT (HLT_Path1 AND HLT_Path2)"));
+    REQUIRE(testExpression("NOT (NOTHLT_Path OR HLT_Path2)"));
+    REQUIRE(testExpression("((L1_A AND HLT_B) OR Dataset_C) AND NOT (Status_D OR Name_E OR HLT_F*) AND L1_??_?_?"));
+    REQUIRE(testExpression("NOT (NOT (HLT_Path1 AND HLT_Path_*))"));
+  }
+
+  // examples of expressions not supported by the triggerExpression parser
+  SECTION("IncorrectExpressions") {
+    REQUIRE(not testExpression("A | B"));
+    REQUIRE(not testExpression("A && B"));
+    REQUIRE(not testExpression("NOT L1_SEED1 ANDD L1_SEED2*"));
+    REQUIRE(not testExpression("NOT (NOTHLT_Path OR HLT_Path2))"));
+    REQUIRE(not testExpression("NOT NOT (HLT_Path1 AND L1_Seed_?? OR HLT_Path_*)"));
+    REQUIRE(not testExpression("HLT_Path* NOT TRUE"));
+  }


This list of good and bad expressions is pretty much random. Suggestions are welcome.

Why is this one invalid ?

testExpression("NOT NOT (HLT_Path1 AND L1_Seed_?? OR HLT_Path_*)"));

I mean, it is silly to use NOT NOT, but is it an error?

Ideally, the parser should understand it, but it doesn't.. NOT NOT [expr] isn't working, while NOT (NOT [expr]) will work (last valid expression in the unit test). In the first case, it again gets into a state where it parses one of the NOTs (I assume the 2nd one) as an HLT-path name, then fails.

Understood.

OK, I think NOT NOT ... is pretty low on the wish list, we can definitely live without.

Okay, I would agree. On the other hand, more checking led me to realise that the parser now reads NOTHLT1 as equivalent to NOT HLT1 (or NOT(HLT1), or NOT (HLT1)), and that seems wrong.. so I'll have to take a deeper look.

Maybe HLT1 ANDHLT2 etc share the same problem ?

Indeed, they do.. the best I could come up with right now is the following:

- unary = ((qi::lit("NOT") >> operand)[qi::_val = new_<OperatorNot>(qi::_1)] | operand[qi::_val = qi::_1]); + unary = ((qi::lit("NOT ") >> operand)[qi::_val = new_<OperatorNot>(qi::_1)] | operand[qi::_val = qi::_1]); expression = - unary[qi::_val = qi::_1] >> *((qi::lit("AND") >> unary)[qi::_val = new_<OperatorAnd>(qi::_val, qi::_1)] | + unary[qi::_val = qi::_1] >> *((qi::lit("AND ") >> unary)[qi::_val = new_<OperatorAnd>(qi::_val, qi::_1)] | - (qi::lit("OR") >> unary)[qi::_val = new_<OperatorOr>(qi::_val, qi::_1)]); + (qi::lit("OR ") >> unary)[qi::_val = new_<OperatorOr>(qi::_val, qi::_1)]);

That seems to work (more checks tomorrow), but it would make the following invalid b/c of no spaces

(ABC)AND((THIS)OR(THAT))

missirol · 2022-08-19T10:23:51Z

type bugfix

cmsbuild · 2022-08-19T10:29:48Z

+code-checks

Logs: https://cmssdt.cern.ch/SDT/code-checks/cms-sw-PR-39108/31646

This PR adds an extra 20KB to repository
There are other open Pull requests which might conflict with changes you have proposed:
- File HLTrigger/HLTfilters/test/triggerResultsFilter_by_PathStatus.py modified in PR(s): allow TriggerResultsFilter to throw for invalid HLTPathStatus patterns #39044
- File HLTrigger/HLTfilters/test/triggerResultsFilter_by_TriggerResults.py modified in PR(s): allow TriggerResultsFilter to throw for invalid HLTPathStatus patterns #39044

cmsbuild · 2022-08-19T10:30:13Z

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

missirol · 2022-08-19T12:44:03Z

please test

cmsbuild · 2022-08-19T17:41:17Z

+1

Summary: https://cmssdt.cern.ch/SDT/jenkins-artifacts/pull-request-integration/PR-ff9ea4/26929/summary.html
COMMIT: 23fdbf8
CMSSW: CMSSW_12_5_X_2022-08-18-2300/el8_amd64_gcc10
User test area: For local testing, you can use /cvmfs/cms-ci.cern.ch/week0/cms-sw/cmssw/39108/26929/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: 4 differences found in the comparisons
DQMHistoTests: Total files compared: 51
DQMHistoTests: Total histograms compared: 3693040
DQMHistoTests: Total failures: 8
DQMHistoTests: Total nulls: 0
DQMHistoTests: Total successes: 3693010
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

- Improve the representation of trigger expression - Improved the dumped representation of trigger expressions: - AND, OR, and NOT operators add parentheses around their subexpression; - uninitialised Path and L1uGT readers use an explicit name instead of FALSE. - Print how the expressions are interpreted - Delimit tokens and operands - Require a delimiter after each token and operamd, to avoid en expression like: "NOTSomePath ANDOtherPath" to be parsed as: "NOT SomePath AND OtherPath" - Add more tests - Compare the parsed and expected representations

cmsbuild · 2022-08-20T11:31:29Z

+code-checks

Logs: https://cmssdt.cern.ch/SDT/code-checks/cms-sw-PR-39108/31675

This PR adds an extra 20KB to repository
There are other open Pull requests which might conflict with changes you have proposed:
- File HLTrigger/HLTfilters/test/triggerResultsFilter_by_PathStatus.py modified in PR(s): allow TriggerResultsFilter to throw for invalid HLTPathStatus patterns #39044
- File HLTrigger/HLTfilters/test/triggerResultsFilter_by_TriggerResults.py modified in PR(s): allow TriggerResultsFilter to throw for invalid HLTPathStatus patterns #39044

cmsbuild · 2022-08-22T06:18:36Z

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

cmsbuild · 2022-08-22T08:36:19Z

+code-checks

Logs: https://cmssdt.cern.ch/SDT/code-checks/cms-sw-PR-39108/31695

This PR adds an extra 20KB to repository
There are other open Pull requests which might conflict with changes you have proposed:
- File HLTrigger/HLTfilters/test/triggerResultsFilter_by_PathStatus.py modified in PR(s): allow TriggerResultsFilter to throw for invalid HLTPathStatus patterns #39044
- File HLTrigger/HLTfilters/test/triggerResultsFilter_by_TriggerResults.py modified in PR(s): allow TriggerResultsFilter to throw for invalid HLTPathStatus patterns #39044

cmsbuild · 2022-08-22T08:36:41Z

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

missirol · 2022-08-22T16:51:29Z

please test

As I grow skeptical of finding a better implementation myself, I start the tests.

cmsbuild · 2022-08-22T20:49:16Z

+1

Summary: https://cmssdt.cern.ch/SDT/jenkins-artifacts/pull-request-integration/PR-ff9ea4/26984/summary.html
COMMIT: 608afea
CMSSW: CMSSW_12_5_X_2022-08-22-1100/el8_amd64_gcc10
User test area: For local testing, you can use /cvmfs/cms-ci.cern.ch/week1/cms-sw/cmssw/39108/26984/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: 4 differences found in the comparisons
DQMHistoTests: Total files compared: 51
DQMHistoTests: Total histograms compared: 3693040
DQMHistoTests: Total failures: 14
DQMHistoTests: Total nulls: 0
DQMHistoTests: Total successes: 3693004
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-23T08:33:00Z

+hlt

fixes the NOT operator in the parser used by TriggerResultsFilter
includes further updates to improve the behaviour of the parser (requiring a delimiter for special keywords like AND/OR/NOT, allowing for expressions like NOT NOT)
the parser now does not allow AND/OR/NOT as names of HLT paths; the implementation of that requirement is maybe suboptimal, but it passes all the tests and considered use cases
added unit test for the parser and cmd-line utility to check its behaviour; unit tests of TriggerResultsFilter improved accordingly

cmsbuild · 2022-08-23T08:33:29Z

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-23T09:45:06Z

+1

missirol added 3 commits August 19, 2022 12:00

fix for NOT operator in TriggerExpressionParser

33f39fe

added unit test for triggerExpression::parse

fb453dc

improved unit tests for TriggerResultsFilter

23fdbf8

cmsbuild added this to the CMSSW_12_5_X milestone Aug 19, 2022

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

missirol changed the title ~~fix for NOT operator in triggerExpression::parse~~ fix for NOT operator in triggerExpression::Parser Aug 19, 2022

missirol commented Aug 19, 2022

View reviewed changes

cmsbuild added the bug-fix label Aug 19, 2022

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

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

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

fwyzard and others added 2 commits August 20, 2022 12:03

recursive NOT in triggerExpression::Parser to make "NOT NOT" valid

c438d5f

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

cmsbuild added the code-checks-approved label Aug 22, 2022

dont allow AND/OR/NOT as HLT-Path names in triggerExpression::Parser

608afea

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

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

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

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

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

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

cmsbuild merged commit 4e3f53e into cms-sw:master Aug 23, 2022

This was referenced Aug 23, 2022

fix for NOT operator in triggerExpression::Parser [12_4_X] #39151

Merged

allow TriggerResultsFilter to throw for invalid HLTPathStatus patterns #39044

Merged

This was referenced Aug 24, 2022

Update input of phase2_digi_reco_pixelntuple unit test #39123

Merged

[BuildRules] testing LTO rules cms-sw/cmsdist#8038

Merged

missirol mentioned this pull request Oct 12, 2022

improve the parser and tests missirol/cmssw#2

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

fix for `NOT` operator in `triggerExpression::Parser` #39108

fix for `NOT` operator in `triggerExpression::Parser` #39108

missirol commented Aug 19, 2022

missirol Aug 19, 2022

fwyzard Aug 19, 2022

missirol Aug 19, 2022

missirol Aug 19, 2022

fwyzard Aug 19, 2022

missirol Aug 19, 2022

fwyzard Aug 19, 2022

missirol Aug 19, 2022 •

edited

fwyzard Aug 19, 2022

missirol Aug 19, 2022

missirol commented Aug 19, 2022

cmsbuild commented Aug 19, 2022

cmsbuild commented Aug 19, 2022

missirol commented Aug 19, 2022

cmsbuild commented Aug 19, 2022

cmsbuild commented Aug 20, 2022

cmsbuild commented Aug 22, 2022

cmsbuild commented Aug 22, 2022

cmsbuild commented Aug 22, 2022

missirol commented Aug 22, 2022

cmsbuild commented Aug 22, 2022

missirol commented Aug 23, 2022

cmsbuild commented Aug 23, 2022

perrotta commented Aug 23, 2022

fix for NOT operator in triggerExpression::Parser #39108

fix for NOT operator in triggerExpression::Parser #39108

Conversation

missirol commented Aug 19, 2022

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

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

Choose a reason for hiding this comment

missirol Aug 19, 2022 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

missirol commented Aug 19, 2022

cmsbuild commented Aug 19, 2022

cmsbuild commented Aug 19, 2022

missirol commented Aug 19, 2022

cmsbuild commented Aug 19, 2022

Comparison Summary

cmsbuild commented Aug 20, 2022

cmsbuild commented Aug 22, 2022

cmsbuild commented Aug 22, 2022

cmsbuild commented Aug 22, 2022

missirol commented Aug 22, 2022

cmsbuild commented Aug 22, 2022

Comparison Summary

missirol commented Aug 23, 2022

cmsbuild commented Aug 23, 2022

perrotta commented Aug 23, 2022

fix for `NOT` operator in `triggerExpression::Parser` #39108

fix for `NOT` operator in `triggerExpression::Parser` #39108

missirol Aug 19, 2022 •

edited