partial technical cleanup of `DQMServices/StreamerIO/plugins` #40420

missirol · 2023-01-03T15:39:15Z

PR description:

This PR applies a partial technical cleanup of the classes in DQMServices/StreamerIO/plugins.

The initial goal was to remove unused class members (e.g.

cmssw/DQMServices/StreamerIO/plugins/DQMStreamerReader.cc

Lines 27 to 28 in 9eb6ddb

    
           runNumber_ = pset.getUntrackedParameter<unsigned int>("runNumber"); 
        
           runInputDir_ = pset.getUntrackedParameter<std::string>("runInputDir");

); then, further technical changes were also included, mainly to clean up #include <header> directives, introduce some const correctness, simplify/modernise small portions of code, and limit (ab)use of using namespace calls.

Merely technical. No changes expected.

PR validation:

None.

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:

N/A

cmsbuild · 2023-01-03T15:51:35Z

+code-checks

Logs: https://cmssdt.cern.ch/SDT/code-checks/cms-sw-PR-40420/33548

This PR adds an extra 36KB to repository

cmsbuild · 2023-01-03T15:52:02Z

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

It involves the following packages:

DQMServices/StreamerIO (dqm)

@emanueleusai, @ahmad3213, @cmsbuild, @syuvivida, @pmandrik, @micsucmed, @rvenditti can you please review it and eventually sign? Thanks.
@barvic 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-01-03T15:53:23Z

please test

cmsbuild · 2023-01-03T19:50:24Z

+1

Summary: https://cmssdt.cern.ch/SDT/jenkins-artifacts/pull-request-integration/PR-de3e18/29787/summary.html
COMMIT: d3c247c
CMSSW: CMSSW_13_0_X_2023-01-03-1100/el8_amd64_gcc11
User test area: For local testing, you can use /cvmfs/cms-ci.cern.ch/week0/cms-sw/cmssw/40420/29787/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: 8 differences found in the comparisons
DQMHistoTests: Total files compared: 49
DQMHistoTests: Total histograms compared: 3555748
DQMHistoTests: Total failures: 157
DQMHistoTests: Total nulls: 0
DQMHistoTests: Total successes: 3555569
DQMHistoTests: Total skipped: 22
DQMHistoTests: Total Missing objects: 0
DQMHistoSizes: Histogram memory added: 0.0 KiB( 48 files compared)
Checked 211 log files, 162 edm output root files, 49 DQM output files
TriggerResults: no differences found

missirol · 2023-01-05T13:48:10Z

DQMServices/StreamerIO/plugins/DQMStreamerReader.cc

      header->hltTriggerNames(tnames);

-      pset.addParameter<Strings>("SelectEvents", hltSel_);


I realised I introduced a mistake here in d3c247c. Supposedly, the original code needed this because TriggerSelector expects a tracked vector<string> (same goes for the relevant ctor of EventSelector), i.e.

cmssw/DQMServices/StreamerIO/plugins/TriggerSelector.cc

Line 44 in 1a989c3

paths = config.getParameter<Strings>("SelectEvents");

cmssw/FWCore/Framework/src/EventSelector.cc

Line 75 in 1a989c3

: pathspecs_(config.empty() ? Strings() : initPathSpecs(config.getParameter<Strings>("SelectEvents"))),

On the other hand, the PSet of DQMStreamerReader provides an untracked vector<string> (guaranted by its fillDescriptions, afaiu).

This will be fixed in the next push with further cleanup (rather than by reverting this incorrect change).

missirol · 2023-01-05T14:59:17Z

DQMServices/StreamerIO/plugins/TriggerSelector.cc

-    eventSelector_.reset(new edm::EventSelector(pathspecs, names));
-  }
-
-  TriggerSelector::TriggerSelector(edm::ParameterSet const& config, Strings const& triggernames, bool old_)


This ctor becomes unnecessary, and is thus removed. It was only used in

cmssw/DQMServices/StreamerIO/plugins/DQMStreamerReader.cc

Line 91 in 1a989c3

void DQMStreamerReader::openFileImp_(const DQMFileIterator::LumiEntry& entry) {

and this PR changes that to use the ctor TriggerSelector(Strings,..) instead.

Rationale: in the one case where TriggerSelector(ParameterSet,..) was used

cmssw/DQMServices/StreamerIO/plugins/TriggerSelector.cc

Line 21 in 1a989c3

TriggerSelector::TriggerSelector(edm::ParameterSet const& config, Strings const& triggernames, bool old_)

old_ == false (default value of argument) and config.empty() == false (because of

cmssw/DQMServices/StreamerIO/plugins/DQMStreamerReader.cc

Line 114 in 1a989c3

pset.addParameter<Strings>("SelectEvents", hltSel_);

), so what is used is

cmssw/DQMServices/StreamerIO/plugins/TriggerSelector.cc

Lines 43 to 49 in 1a989c3

Strings paths;

paths = config.getParameter<Strings>("SelectEvents");

if (!paths.empty()) {

useOld_ = true;

eventSelector_.reset(new edm::EventSelector(config, triggernames));

return;

}

That is effectively equivalent to the TriggerSelector(Strings,..) ctor. There is slight difference between the two constructors: if paths.empty() == true, the PSet-based ctor uses acceptAll_ == true and does not create an EventSelector; on the other hand, the behavior is the same, because TriggerSelector(Strings,..) (the other ctor) would create an EventSelector with an empty pathspecs

cmssw/DQMServices/StreamerIO/plugins/TriggerSelector.cc

Line 18 in 1a989c3

eventSelector_.reset(new edm::EventSelector(pathspecs, names));

and in that case EventSelector will accept all events anyway (as one would naively expect)

cmssw/FWCore/Framework/src/EventSelector.cc

Lines 49 to 51 in 1a989c3

: pathspecs_(initPathSpecs(pathspecs)),

results_from_current_process_(true),

accept_all_(initAcceptAll()),

cmssw/FWCore/Framework/src/EventSelector.cc

Lines 100 to 103 in 1a989c3

bool EventSelector::initAcceptAll() {

if (pathspecs_.empty()) {

return true;

}

The added value of this cleanup is that (1) TriggerSelector becomes independent of ParameterSet, and (2) two static-analyzer warnings (use of catch(...)) are removed.

missirol · 2023-01-05T15:00:10Z

DQMServices/StreamerIO/plugins/TriggerSelector.cc

-      try {
-        std::string myPath = trim(config.getParameter<std::string>("TriggerSelector"));
-        if (!myPath.empty()) {
-          init(myPath, triggernames);
-          return;
-        }
-      } catch (...) {
-      }


A string configuration-parameter named TriggerSelector is not used anywhere in CMSSW, as far as I can see. Not sure if I need to worry about client code outside CMSSW in this case.

missirol · 2023-01-05T15:02:10Z

FWCore/Framework/src/EventSelector.cc

+      : EventSelector(
+            [&config]() {
+              auto const& pset = config.getUntrackedParameterSet("SelectEvents");
+              return (pset.empty() ? Strings() : pset.getParameter<Strings>("SelectEvents"));
+            }(),
+            pathNames) {}


I tentatively added ecb204a, because I noticed that the PSet-based constructor of EventSelector is somewhat inconsistent with its fillDescription method (the latter defines SelectEvents.SelectEvents, while the ctor expects SelectEvents). This required small changes to 3 FWCore unit tests. With this PR, the PSet-based ctor of EventSelector would not be used anywhere, I think (outside those unit tests), so it could even be removed. To be clear, ecb204a is not necessary for this PR, so I could remove it if preferred.

On a quick look I agree that with this PR the PSet-based constructor of EventSelector becomes unused (besides the unit tests). My preference would be to just remove it (probably cleaner to do in a separate PR). Thanks!

Sounds good. I will remove ecb204a from this PR, to keep it 'DQM only' (although I could use review comments on this PR too, e.g. #40420 (comment)), and I'll open a separate PR for the small changes in FWCore.

Edit: done in #40432.

missirol · 2023-01-05T15:02:58Z

DQMServices/StreamerIO/plugins/DQMMonitoringService.cc

-    } catch (...) {
-      // pass
+    } catch (std::exception const& exc) {
+      LogDebug("DQMMonitoringService") << "Exception thrown in outputUpdate method: " << exc.what();


One more static-analyzer warning (catch(...)) is coming from here, but I'm not sure how to fix this one. The change in 3de6025 is tentative, and I welcome feedback.

cmsbuild · 2023-01-05T15:06:17Z

+code-checks

Logs: https://cmssdt.cern.ch/SDT/code-checks/cms-sw-PR-40420/33565

This PR adds an extra 52KB to repository

cmsbuild · 2023-01-05T15:06:43Z

Pull request #40420 was updated. @smuzaffar, @Dr15Jones, @makortel, @emanueleusai, @ahmad3213, @cmsbuild, @syuvivida, @pmandrik, @micsucmed, @rvenditti can you please check and sign again.

cmsbuild · 2023-01-05T17:43:50Z

+code-checks

Logs: https://cmssdt.cern.ch/SDT/code-checks/cms-sw-PR-40420/33570

This PR adds an extra 36KB to repository

cmsbuild · 2023-01-05T17:44:09Z

Pull request #40420 was updated. @emanueleusai, @ahmad3213, @cmsbuild, @syuvivida, @pmandrik, @micsucmed, @rvenditti can you please check and sign again.

missirol · 2023-01-10T17:53:15Z

@cms-sw/dqm-l2, could you please review this PR? Thanks!

emanueleusai · 2023-01-13T04:58:20Z

please test

cmsbuild · 2023-01-13T07:34:48Z

+1

Summary: https://cmssdt.cern.ch/SDT/jenkins-artifacts/pull-request-integration/PR-de3e18/29956/summary.html
COMMIT: 5f0af33
CMSSW: CMSSW_13_0_X_2023-01-12-2300/el8_amd64_gcc11
User test area: For local testing, you can use /cvmfs/cms-ci.cern.ch/week1/cms-sw/cmssw/40420/29956/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: 9 differences found in the comparisons
DQMHistoTests: Total files compared: 49
DQMHistoTests: Total histograms compared: 3555538
DQMHistoTests: Total failures: 6
DQMHistoTests: Total nulls: 0
DQMHistoTests: Total successes: 3555510
DQMHistoTests: Total skipped: 22
DQMHistoTests: Total Missing objects: 0
DQMHistoSizes: Histogram memory added: 0.0 KiB( 48 files compared)
Checked 211 log files, 162 edm output root files, 49 DQM output files
TriggerResults: no differences found

emanueleusai · 2023-01-16T07:30:58Z

+1

cmsbuild · 2023-01-16T07:31:23Z

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)

rappoccio · 2023-01-16T17:16:38Z

+1

cmsbuild added this to the CMSSW_13_0_X milestone Jan 3, 2023

cmsbuild added code-checks-pending dqm-pending orp-pending pending-signatures tests-pending labels Jan 3, 2023

cmsbuild added code-checks-approved and removed code-checks-pending labels Jan 3, 2023

cmsbuild added tests-started and removed tests-pending labels Jan 3, 2023

cmsbuild added tests-approved and removed tests-started labels Jan 3, 2023

missirol commented Jan 5, 2023

View reviewed changes

missirol force-pushed the devel_dqmStreamerCleanup branch from d3c247c to 3de6025 Compare January 5, 2023 14:54

cmsbuild added code-checks-pending core-pending tests-pending and removed tests-approved code-checks-approved labels Jan 5, 2023

missirol commented Jan 5, 2023

View reviewed changes

cmsbuild added code-checks-approved and removed code-checks-pending labels Jan 5, 2023

partial technical cleanup of DQMServices/StreamerIO/plugins

5f0af33

missirol force-pushed the devel_dqmStreamerCleanup branch from 3de6025 to 5f0af33 Compare January 5, 2023 17:38

cmsbuild added code-checks-pending and removed core-pending code-checks-approved labels Jan 5, 2023

cmsbuild added code-checks-approved and removed code-checks-pending labels Jan 5, 2023

missirol mentioned this pull request Jan 5, 2023

remove PSet-based ctor of EventSelector #40432

Merged

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

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

cmsbuild added dqm-approved fully-signed and removed dqm-pending pending-signatures labels Jan 16, 2023

cmsbuild added orp-approved and removed orp-pending labels Jan 16, 2023

cmsbuild merged commit a0ac635 into cms-sw:master Jan 16, 2023

cmsbuild mentioned this pull request Jan 16, 2023

Adding new GCP trends tool and commenting unnecessary print out in Trend #40336

Merged

missirol deleted the devel_dqmStreamerCleanup branch January 16, 2023 21:34

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

partial technical cleanup of `DQMServices/StreamerIO/plugins` #40420

partial technical cleanup of `DQMServices/StreamerIO/plugins` #40420

missirol commented Jan 3, 2023

cmsbuild commented Jan 3, 2023

cmsbuild commented Jan 3, 2023

missirol commented Jan 3, 2023

cmsbuild commented Jan 3, 2023

missirol Jan 5, 2023

missirol Jan 5, 2023

missirol Jan 5, 2023

missirol Jan 5, 2023

makortel Jan 5, 2023

missirol Jan 5, 2023 •

edited

missirol Jan 5, 2023

cmsbuild commented Jan 5, 2023

cmsbuild commented Jan 5, 2023

cmsbuild commented Jan 5, 2023

cmsbuild commented Jan 5, 2023

missirol commented Jan 10, 2023

emanueleusai commented Jan 13, 2023

cmsbuild commented Jan 13, 2023

emanueleusai commented Jan 16, 2023

cmsbuild commented Jan 16, 2023

rappoccio commented Jan 16, 2023

	runNumber_ = pset.getUntrackedParameter<unsigned int>("runNumber");
	runInputDir_ = pset.getUntrackedParameter<std::string>("runInputDir");

		header->hltTriggerNames(tnames);

		pset.addParameter<Strings>("SelectEvents", hltSel_);

	Strings paths;
	paths = config.getParameter<Strings>("SelectEvents");
	if (!paths.empty()) {
	useOld_ = true;
	eventSelector_.reset(new edm::EventSelector(config, triggernames));
	return;
	}

	: pathspecs_(initPathSpecs(pathspecs)),
	results_from_current_process_(true),
	accept_all_(initAcceptAll()),

	bool EventSelector::initAcceptAll() {
	if (pathspecs_.empty()) {
	return true;
	}

partial technical cleanup of DQMServices/StreamerIO/plugins #40420

partial technical cleanup of DQMServices/StreamerIO/plugins #40420

Conversation

missirol commented Jan 3, 2023

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:

cmsbuild commented Jan 3, 2023

cmsbuild commented Jan 3, 2023

missirol commented Jan 3, 2023

cmsbuild commented Jan 3, 2023

Comparison Summary

missirol Jan 5, 2023

Choose a reason for hiding this comment

missirol Jan 5, 2023

Choose a reason for hiding this comment

missirol Jan 5, 2023

Choose a reason for hiding this comment

missirol Jan 5, 2023

Choose a reason for hiding this comment

makortel Jan 5, 2023

Choose a reason for hiding this comment

missirol Jan 5, 2023 • edited

Choose a reason for hiding this comment

missirol Jan 5, 2023

Choose a reason for hiding this comment

cmsbuild commented Jan 5, 2023

cmsbuild commented Jan 5, 2023

cmsbuild commented Jan 5, 2023

cmsbuild commented Jan 5, 2023

missirol commented Jan 10, 2023

emanueleusai commented Jan 13, 2023

cmsbuild commented Jan 13, 2023

Comparison Summary

emanueleusai commented Jan 16, 2023

cmsbuild commented Jan 16, 2023

rappoccio commented Jan 16, 2023

partial technical cleanup of `DQMServices/StreamerIO/plugins` #40420

partial technical cleanup of `DQMServices/StreamerIO/plugins` #40420

missirol Jan 5, 2023 •

edited