remove access to bare ROOT objects in HLTFiltersDQMonitor
#40426
Conversation
|
+code-checks Logs: https://cmssdt.cern.ch/SDT/code-checks/cms-sw-PR-40426/33558
|
|
A new Pull Request was created by @missirol (Marino Missiroli) for master. It involves the following packages:
@emanueleusai, @ahmad3213, @cmsbuild, @syuvivida, @pmandrik, @micsucmed, @rvenditti can you please review it and eventually sign? Thanks. cms-bot commands are listed here |
|
type bugfix |
| if (binIndexMap_.find(iPathName) == binIndexMap_.end()) { | ||
| throw cms::Exception("HLTFiltersDQMonitorInvalidBinLabel") | ||
| << "invalid key for bin-index map (name of Path bin in MonitorElement of HLT Menu): \"" << iPathName << "\""; | ||
| } | ||
| auto const ibin = binIndexMap_[iPathName]; |
There was a problem hiding this comment.
The following would be more efficient
| if (binIndexMap_.find(iPathName) == binIndexMap_.end()) { | |
| throw cms::Exception("HLTFiltersDQMonitorInvalidBinLabel") | |
| << "invalid key for bin-index map (name of Path bin in MonitorElement of HLT Menu): \"" << iPathName << "\""; | |
| } | |
| auto const ibin = binIndexMap_[iPathName]; | |
| auto const foundBin = binIndexMap_.find(iPathName); | |
| if (foundBin == binIndexMap_.end()) { | |
| throw cms::Exception("HLTFiltersDQMonitorInvalidBinLabel") | |
| << "invalid key for bin-index map (name of Path bin in MonitorElement of HLT Menu): \"" << iPathName << "\""; | |
| } | |
| auto const ibin = *foundBin; |
by avoiding a second search (even if it has a constant cost on average).
| } | ||
| auto const& dsetPathNames(hltConfigProvider_.datasetContent(idset)); | ||
| MonitorElement* const meDatasetProf = meDatasetMap_[meDatasetName]; |
There was a problem hiding this comment.
Also here storing the returned iterator from meDatasetMap_.find(meDatasetName) in a variable and de-referencing it would be more efficient.
| throw cms::Exception("HLTFiltersDQMonitorInvalidBinLabel") | ||
| << "invalid key for bin-index map (name of Path bin in MonitorElement of Dataset): \"" << ibinKey << "\""; | ||
| } | ||
| auto const ibin = binIndexMap_[ibinKey]; |
There was a problem hiding this comment.
Also here storing the returned iterator from binIndexMap_.find(ibinKey) in a variable and de-referencing it would be more efficient.
| continue; | ||
| } | ||
|
|
||
| MonitorElement* const mePathProf = mePathMap_[mePathName]; |
There was a problem hiding this comment.
Also here storing the returned iterator from mePathMap_.find(mePathName) in a variable and de-referencing it would be more efficient.
| << "invalid key for bin-index map (name of Module bin in MonitorElement of Path): \"" << ibinKey | ||
| << "\""; | ||
| } | ||
| auto const ibin = binIndexMap_[ibinKey]; |
There was a problem hiding this comment.
Also here storing the returned iterator from binIndexMap_.find(ibinKey) in a variable and de-referencing it would be more efficient.
Thanks for the review! Added in ed5f69b. |
|
+code-checks Logs: https://cmssdt.cern.ch/SDT/code-checks/cms-sw-PR-40426/33560
|
|
Pull request #40426 was updated. @emanueleusai, @ahmad3213, @cmsbuild, @syuvivida, @pmandrik, @micsucmed, @rvenditti can you please check and sign again. |
| ## CLI parser | ||
| import argparse | ||
| import sys | ||
|
|
||
| parser = argparse.ArgumentParser( | ||
| prog = 'cmsRun '+sys.argv[0]+' --', | ||
| description = 'Configuration file to test of the HLTFiltersDQMonitor plugin.', | ||
| formatter_class = argparse.ArgumentDefaultsHelpFormatter | ||
| ) | ||
|
|
There was a problem hiding this comment.
Just for completeness: this argparse configuration was ~copied from
cmssw/FWCore/Integration/test/testSwitchProducerTask_cfg.py
Lines 3 to 14 in 287f4db
I was wondering if there are cases where
VarParsing has advantages (or, is recommended) wrt argparse.
The only hiccup I encountered with this argparse was with
cmsRun DQMOffline/Trigger/test/testHLTFiltersDQMonitor_cfg.py -- -hwhich I naively expected to work, but leads to an error [1] (didn't figure out the solution).
[1]
----- Begin Fatal Exception 04-Jan-2023 19:08:48 CET-----------------------
An exception of category 'ConfigFileReadError' occurred while
[0] Processing the python configuration file named DQMOffline/Trigger/test/testHLTFiltersDQMonitor_cfg.py
Exception Message:
unknown python problem occurred.
SystemExit: 0
At:
/cvmfs/cms-ib.cern.ch/sw/x86_64/week0/el8_amd64_gcc11/external/python3/3.9.14-76a14295dd5255228210eb596893b98c/lib/python3.9/argparse.py(2569): exit
/cvmfs/cms-ib.cern.ch/sw/x86_64/week0/el8_amd64_gcc11/external/python3/3.9.14-76a14295dd5255228210eb596893b98c/lib/python3.9/argparse.py(1100): __call__
/cvmfs/cms-ib.cern.ch/sw/x86_64/week0/el8_amd64_gcc11/external/python3/3.9.14-76a14295dd5255228210eb596893b98c/lib/python3.9/argparse.py(1935): take_action
/cvmfs/cms-ib.cern.ch/sw/x86_64/week0/el8_amd64_gcc11/external/python3/3.9.14-76a14295dd5255228210eb596893b98c/lib/python3.9/argparse.py(2007): consume_optional
/cvmfs/cms-ib.cern.ch/sw/x86_64/week0/el8_amd64_gcc11/external/python3/3.9.14-76a14295dd5255228210eb596893b98c/lib/python3.9/argparse.py(2067): _parse_known_args
/cvmfs/cms-ib.cern.ch/sw/x86_64/week0/el8_amd64_gcc11/external/python3/3.9.14-76a14295dd5255228210eb596893b98c/lib/python3.9/argparse.py(1861): parse_known_args
DQMOffline/Trigger/test/testHLTFiltersDQMonitor_cfg.py(37): <module>
----- End Fatal Exception -------------------------------------------------
There was a problem hiding this comment.
Just for completeness: this
argparseconfiguration was ~copied from
For future reference the use of argparse is also described in
https://twiki.cern.ch/twiki/bin/view/CMSPublic/SWGuideAboutPythonConfigFile#argparse
The only hiccup I encountered with this
argparsewas withcmsRun DQMOffline/Trigger/test/testHLTFiltersDQMonitor_cfg.py -- -hwhich I naively expected to work, but leads to an error [1]
Hah, interesting. It seems that sys.exit(0) gets communicated as a thrown exception by PyBind11 in the C++ side, which framework then reports as shown (the printout from -h is there, although in a slightly confusing way). It is not immediately clear to me if we should do something about it, so a specific issue might be worth it.
As for VarParsing vs argparse, core does not have a specific recommendation. First has longer history within CMSSW (and not really developed further), and the latter comes from python's standard library.
|
please test |
|
+1 Summary: https://cmssdt.cern.ch/SDT/jenkins-artifacts/pull-request-integration/PR-45ee1a/29799/summary.html Comparison SummarySummary:
|
|
The only real differences in PR-test outputs are in wfs This is just due to the renaming of one histogram, which is expected because of this change Overall, things look okay (I don't have other updates planned for this PR). @cms-sw/dqm-l2, please prioritise the review of this PR, since it is a bugfix. |
|
+1
|
cmsbuild
added
dqm-approved
fully-signed
and removed
dqm-pending
pending-signatures
labels
|
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) |
|
+1 |
PR description:
This PR updates the plugin
HLTFiltersDQMonitorin order not to access bare ROOT objects via the methods of DQM'sMonitorElement. It potentially solves #40419.A unit test is also added, along with other small technical updates to
HLTFiltersDQMonitor.The PR contains the following changes, in order.
0f5ff99 adds a unit test for the plugin in question. This unit test passes with
threads == streams == 1, and it often reproduces the error in Error in THashTable::Rehash in HLTFiltersDQMonitor #40419 when using 4 or more threads/streams (and before adding f51c546).f51c546 implements the actual fix following suggestions by @makortel in Error in THashTable::Rehash in HLTFiltersDQMonitor #40419 (comment) and Error in THashTable::Rehash in HLTFiltersDQMonitor #40419 (comment); calls to
FindBinare removed at the cost of using a string-to-binIndex map (std::unordered_map<std::string, unsigned int>) which is filled once per run. This commit also updates the unit test to run with 4 threads(==streams), instead of 1.6211e8b includes small technical changes to improve readability/clarity (renaming of some of the parameters of
HLTFiltersDQMonitor, renaming ofcfimodule to start withdqmrather thanhlt, various mostly-stylistic changes inHLTFiltersDQMonitor.cc).Merely technical. No changes expected in the outputs of PR tests, except for the partial renaming of one
MonitorElementinHLTFiltersDQMonitorhere:PR validation:
The new unit test often reproduced the error in #40419 when using 4 or more threads(==streams) without the cpp changes in this PR. Once the latter are included, the same test consistently passed.
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:
TBD. This is a bugfix, and backports might be needed.