Modernize SiStripMonitorClient #39518

tvami · 2022-09-28T02:57:26Z

PR description:

Since I touched the package and gave a lot of warnings about the Legacy modules,

I moved everything to a ::one:: module
merged the header files to the cc files
moves the es consume into the initializer list

PR validation:

Code compiles w/o warning.

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

tvami · 2022-09-28T02:57:47Z

type bugfix,trk

cmsbuild · 2022-09-28T03:04:51Z

+code-checks

Logs: https://cmssdt.cern.ch/SDT/code-checks/cms-sw-PR-39518/32277

This PR adds an extra 32KB to repository

cmsbuild · 2022-09-28T03:05:21Z

A new Pull Request was created by @tvami (Tamas Vami) for master.

It involves the following packages:

DQM/SiStripMonitorClient (dqm)

@emanueleusai, @ahmad3213, @cmsbuild, @jfernan2, @syuvivida, @pmandrik, @micsucmed, @rvenditti can you please review it and eventually sign? Thanks.
@arossi83, @hdelanno, @sroychow, @fioriNTU, @jandrea, @idebruyn, @mmusich, @threus, @venturia 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

tvami · 2022-09-28T03:28:01Z

@cmsbuild , please test

DQM/SiStripMonitorClient/plugins/SiStripAnalyser.cc

DQM/SiStripMonitorClient/plugins/SiStripCertificationInfo.cc

DQM/SiStripMonitorClient/plugins/SiStripDaqInfo.cc

DQM/SiStripMonitorClient/plugins/SiStripDcsInfo.cc

cmsbuild · 2022-09-28T09:05:30Z

+1

Summary: https://cmssdt.cern.ch/SDT/jenkins-artifacts/pull-request-integration/PR-b5d35e/27807/summary.html
COMMIT: 419be43
CMSSW: CMSSW_12_6_X_2022-09-27-2300/el8_amd64_gcc10
User test area: For local testing, you can use /cvmfs/cms-ci.cern.ch/week0/cms-sw/cmssw/39518/27807/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: 3624338
DQMHistoTests: Total failures: 3
DQMHistoTests: Total nulls: 0
DQMHistoTests: Total successes: 3624313
DQMHistoTests: Total skipped: 22
DQMHistoTests: Total Missing objects: 0
DQMHistoSizes: Histogram memory added: -39.132000000000005 KiB( 50 files compared)
DQMHistoSizes: changed ( 1000.0,... ): -1.131 KiB SiStrip/EventInfo
DQMHistoSizes: changed ( 140.53 ): -0.352 KiB SiStrip/MechanicalView
DQMHistoSizes: changed ( 4.22 ): -4.852 KiB SiStrip/MechanicalView
Checked 212 log files, 49 edm output root files, 51 DQM output files
TriggerResults: no differences found

tvami · 2022-09-28T12:24:55Z

OK, I can make them DQMEDHarvester, thanks Marco for the feedback!

mmusich · 2022-09-28T16:14:31Z

assign trk-dpg

DQMHistoSizes: Histogram memory added: -39.132000000000005 KiB( 50 files compared)
DQMHistoSizes: changed ( 1000.0,... ): -1.131 KiB SiStrip/EventInfo
DQMHistoSizes: changed ( 140.53 ): -0.352 KiB SiStrip/MechanicalView
DQMHistoSizes: changed ( 4.22 ): -4.852 KiB SiStrip/MechanicalView

By the way the comparison in the DQM bin-by-bin does not look good (see link)

-1 fwiw.

cmsbuild · 2022-09-28T16:14:53Z

New categories assigned: trk-dpg

@connorpa,@sroychow you have been requested to review this Pull request/Issue and eventually sign? Thanks

tvami · 2022-10-03T00:10:50Z

I decoupled the two changes in this PR, as the migration to DQMEDHarvester is non-trivial. I get the following error when I simply move to it


>> Compiling edm plugin /data/vami/projects/DQM/CMSSW_12_6_X_2022-09-27-1100/src/DQM/SiStripMonitorClient/plugins/SiStripCertificationInfo.cc
/data/vami/projects/DQM/CMSSW_12_6_X_2022-09-27-1100/src/DQM/SiStripMonitorClient/plugins/SiStripCertificationInfo.cc:37:8: error: virtual function 'virtual void SiStripCertificationInfo::endLuminosityBlock(const edm::LuminosityBlock&, const edm::EventSetup&)' overriding final function
   37 |   void endLuminosityBlock(edm::LuminosityBlock const& lumiSeg, edm::EventSetup const& iSetup);
      |        ^~~~~~~~~~~~~~~~~~
In file included from /data/vami/projects/DQM/CMSSW_12_6_X_2022-09-27-1100/src/DQM/SiStripMonitorClient/plugins/SiStripCertificationInfo.cc:9:
/cvmfs/cms-ib.cern.ch/week0/slc7_amd64_gcc10/cms/cmssw-patch/CMSSW_12_6_X_2022-09-27-1100/src/DQMServices/Core/interface/DQMEDHarvester.h:132:8: note: overridden function is 'virtual void DQMEDHarvester::endLuminosityBlock(const edm::LuminosityBlock&, const edm::EventSetup&)'
  132 |   void endLuminosityBlock(edm::LuminosityBlock const &, edm::EventSetup const &) final{};
      |        ^~~~~~~~~~~~~~~~~~
In file included from /cvmfs/cms-ib.cern.ch/nweek-02752/slc7_amd64_gcc10/external/gcc/10.3.0-84898dea653199466402e67d73657f10/include/c++/10.3.0/memory:83,
                 from /cvmfs/cms-ib.cern.ch/week0/slc7_amd64_gcc10/cms/cmssw-patch/CMSSW_12_6_X_2022-09-27-1100/src/FWCore/Framework/interface/FunctorESHandleExceptionFactory.h:24,
                 from /cvmfs/cms-ib.cern.ch/week0/slc7_amd64_gcc10/cms/cmssw-patch/CMSSW_12_6_X_2022-09-27-1100/src/FWCore/Framework/interface/EventSetupRecord.h:42,
                 from /cvmfs/cms-ib.cern.ch/week0/slc7_amd64_gcc10/cms/cmssw-patch/CMSSW_12_6_X_2022-09-27-1100/src/FWCore/Framework/interface/EventSetupRecordImplementation.h:26,
                 from /cvmfs/cms-ib.cern.ch/week0/slc7_amd64_gcc10/cms/cmssw-patch/CMSSW_12_6_X_2022-09-27-1100/src/CalibTracker/Records/interface/SiStripDependentRecords.h:4,
                 from /cvmfs/cms-ib.cern.ch/week0/slc7_amd64_gcc10/cms/cmssw-patch/CMSSW_12_6_X_2022-09-27-1100/src/CalibTracker/Records/interface/SiStripDetCablingRcd.h:1,
                 from /data/vami/projects/DQM/CMSSW_12_6_X_2022-09-27-1100/src/DQM/SiStripMonitorClient/plugins/SiStripCertificationInfo.cc:2:
/cvmfs/cms-ib.cern.ch/nweek-02752/slc7_amd64_gcc10/external/gcc/10.3.0-84898dea653199466402e67d73657f10/include/c++/10.3.0/bits/unique_ptr.h: In instantiation of 'typename std::_MakeUniq<_Tp>::__single_object std::make_unique(_Args&& ...) [with _Tp = SiStripCertificationInfo; _Args = {const edm::ParameterSet&}; typename std::_MakeUniq<_Tp>::__single_object = std::unique_ptr<SiStripCertificationInfo, std::default_delete<SiStripCertificationInfo> >]':
/cvmfs/cms-ib.cern.ch/week0/slc7_amd64_gcc10/cms/cmssw-patch/CMSSW_12_6_X_2022-09-27-1100/src/FWCore/Framework/interface/maker/MakeModuleHelper.h:39:40:   required from 'static std::unique_ptr<_Tp> edm::MakeModuleHelper<Base>::makeModule(const edm::ParameterSet&) [with T = SiStripCertificationInfo; Base = edm::one::EDProducerBase]'
/cvmfs/cms-ib.cern.ch/week0/slc7_amd64_gcc10/cms/cmssw-patch/CMSSW_12_6_X_2022-09-27-1100/src/FWCore/Framework/interface/maker/WorkerMaker.h:82:107:   required from 'std::shared_ptr<edm::maker::ModuleHolder> edm::WorkerMaker<T>::makeModule(const edm::ParameterSet&) const [with T = SiStripCertificationInfo]'
/cvmfs/cms-ib.cern.ch/week0/slc7_amd64_gcc10/cms/cmssw-patch/CMSSW_12_6_X_2022-09-27-1100/src/FWCore/Framework/interface/maker/WorkerMaker.h:76:40:   required from here
/cvmfs/cms-ib.cern.ch/nweek-02752/slc7_amd64_gcc10/external/gcc/10.3.0-84898dea653199466402e67d73657f10/include/c++/10.3.0/bits/unique_ptr.h:962:30: error: invalid new-expression of abstract class type 'SiStripCertificationInfo'
  962 |     { return unique_ptr<_Tp>(new _Tp(std::forward<_Args>(__args)...)); }
      |                              ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
/data/vami/projects/DQM/CMSSW_12_6_X_2022-09-27-1100/src/DQM/SiStripMonitorClient/plugins/SiStripCertificationInfo.cc:28:7: note:   because the following virtual functions are pure within 'SiStripCertificationInfo':
   28 | class SiStripCertificationInfo : public DQMEDHarvester {
      |       ^~~~~~~~~~~~~~~~~~~~~~~~
In file included from /data/vami/projects/DQM/CMSSW_12_6_X_2022-09-27-1100/src/DQM/SiStripMonitorClient/plugins/SiStripCertificationInfo.cc:9:
/cvmfs/cms-ib.cern.ch/week0/slc7_amd64_gcc10/cms/cmssw-patch/CMSSW_12_6_X_2022-09-27-1100/src/DQMServices/Core/interface/DQMEDHarvester.h:166:16: note:     'virtual void DQMEDHarvester::dqmEndJob(dqm::legacy::DQMStore::IBooker&, dqm::legacy::DQMStore::IGetter&)'
  166 |   virtual void dqmEndJob(DQMStore::IBooker &, DQMStore::IGetter &) = 0;
      |                ^~~~~~~~~

DQM/SiStripMonitorClient/scripts/PhaseITreeProducer.py

DQM/SiStripMonitorClient/scripts/TH2PolyOfflineMaps.py

DQM/SiStripMonitorClient/plugins/SiStripAnalyser.cc

DQM/SiStripMonitorClient/plugins/SiStripCertificationInfo.cc

DQM/SiStripMonitorClient/plugins/SiStripDaqInfo.cc

DQM/SiStripMonitorClient/plugins/SiStripDcsInfo.cc

tvami · 2022-10-10T15:12:36Z

@perrotta I fixed the duplicate includes, thanks for noticing that!

tvami · 2022-10-10T15:12:42Z

@cmsbuild , please test

cmsbuild · 2022-10-10T15:16:01Z

+code-checks

Logs: https://cmssdt.cern.ch/SDT/code-checks/cms-sw-PR-39518/32516

This PR adds an extra 24KB to repository

cmsbuild · 2022-10-10T15:16:29Z

Pull request #39518 was updated. @connorpa, @sroychow, @emanueleusai, @ahmad3213, @jfernan2, @syuvivida, @pmandrik, @micsucmed, @rvenditti can you please check and sign again.

cmsbuild · 2022-10-10T19:02:50Z

+1

Summary: https://cmssdt.cern.ch/SDT/jenkins-artifacts/pull-request-integration/PR-b5d35e/28161/summary.html
COMMIT: 391bbb1
CMSSW: CMSSW_12_6_X_2022-10-10-1100/el8_amd64_gcc10
User test area: For local testing, you can use /cvmfs/cms-ci.cern.ch/week0/cms-sw/cmssw/39518/28161/install.sh to create a dev area with all the needed externals and cmssw changes.

Comparison Summary

@slava77 comparisons for the following workflows were not done due to missing matrix map:

/data/cmsbld/jenkins/workspace/compare-root-files-short-matrix/data/PR-b5d35e/41834.0_TTbar_14TeV+2026D94+TTbar_14TeV_TuneCP5_GenSimHLBeamSpot14+DigiTrigger+RecoGlobal+HARVESTGlobal

Summary:

No significant changes to the logs found
Reco comparison results: 4 differences found in the comparisons
DQMHistoTests: Total files compared: 49
DQMHistoTests: Total histograms compared: 3392309
DQMHistoTests: Total failures: 3
DQMHistoTests: Total nulls: 0
DQMHistoTests: Total successes: 3392284
DQMHistoTests: Total skipped: 22
DQMHistoTests: Total Missing objects: 0
DQMHistoSizes: Histogram memory added: 0.0 KiB( 48 files compared)
Checked 204 log files, 49 edm output root files, 49 DQM output files
TriggerResults: no differences found

tvami · 2022-10-11T00:45:00Z

@sroychow @emanueleusai can you please re-sign?

emanueleusai · 2022-10-11T04:52:29Z

+1

perrotta · 2022-10-11T05:01:22Z

@sroychow last updates were just cleanings: I think you can sign once again fror @cms-sw/trk-dpg-l2

sroychow · 2022-10-11T05:45:50Z

+1

cmsbuild · 2022-10-11T05:46:12Z

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)

perrotta · 2022-10-11T06:26:40Z

+1

cmsbuild added this to the CMSSW_12_6_X milestone Sep 28, 2022

cmsbuild added bug-fix code-checks-pending dqm-pending orp-pending pending-signatures tests-pending trk labels Sep 28, 2022

cmsbuild added code-checks-approved and removed code-checks-pending labels Sep 28, 2022

cmsbuild added tests-started and removed tests-pending labels Sep 28, 2022

mmusich reviewed Sep 28, 2022

View reviewed changes

DQM/SiStripMonitorClient/plugins/SiStripAnalyser.cc Outdated Show resolved Hide resolved

mmusich reviewed Sep 28, 2022

View reviewed changes

DQM/SiStripMonitorClient/plugins/SiStripCertificationInfo.cc Outdated Show resolved Hide resolved

DQM/SiStripMonitorClient/plugins/SiStripDaqInfo.cc Show resolved Hide resolved

DQM/SiStripMonitorClient/plugins/SiStripDcsInfo.cc Outdated Show resolved Hide resolved

cmsbuild added tests-approved and removed tests-started labels Sep 28, 2022

tvami mentioned this pull request Sep 28, 2022

Migrate legacy EDModules to thread-efficient alternatives #36404

Closed

cmsbuild added the trk-dpg-pending label Sep 28, 2022

tvami changed the title ~~Modernize SiStripMonitorClient, bugfix to the destructor of two scripts~~ Modernize SiStripMonitorClient Oct 3, 2022

cmsbuild mentioned this pull request Oct 3, 2022

Bugfix to the destructor of two TkDQM scripts #39573

Merged

mmusich suggested changes Oct 3, 2022

View reviewed changes

cmsbuild added pending-signatures tests-pending trk-dpg-pending labels Oct 10, 2022

cmsbuild added tests-started and removed tests-pending labels Oct 10, 2022

cmsbuild added code-checks-approved and removed code-checks-pending labels Oct 10, 2022

cmsbuild added tests-approved and removed tests-started labels Oct 10, 2022

cmsbuild added dqm-approved and removed dqm-pending labels Oct 11, 2022

cmsbuild added fully-signed trk-dpg-approved and removed pending-signatures trk-dpg-pending labels Oct 11, 2022

cmsbuild added orp-approved and removed orp-pending labels Oct 11, 2022

cmsbuild merged commit 3e3d74f into cms-sw:master Oct 11, 2022

mmusich mentioned this pull request Oct 13, 2022

add edm::one::SharedResources to DQM/SiStripMonitorClient plugins #39723

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Modernize SiStripMonitorClient #39518

Modernize SiStripMonitorClient #39518

tvami commented Sep 28, 2022 •

edited

Loading

tvami commented Sep 28, 2022

cmsbuild commented Sep 28, 2022

cmsbuild commented Sep 28, 2022

tvami commented Sep 28, 2022

cmsbuild commented Sep 28, 2022

tvami commented Sep 28, 2022

mmusich commented Sep 28, 2022

cmsbuild commented Sep 28, 2022

tvami commented Oct 3, 2022

tvami commented Oct 10, 2022

tvami commented Oct 10, 2022

cmsbuild commented Oct 10, 2022

cmsbuild commented Oct 10, 2022

cmsbuild commented Oct 10, 2022

tvami commented Oct 11, 2022

emanueleusai commented Oct 11, 2022

perrotta commented Oct 11, 2022

sroychow commented Oct 11, 2022

cmsbuild commented Oct 11, 2022

perrotta commented Oct 11, 2022

Modernize SiStripMonitorClient #39518

Modernize SiStripMonitorClient #39518

Conversation

tvami commented Sep 28, 2022 • edited Loading

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:

tvami commented Sep 28, 2022

cmsbuild commented Sep 28, 2022

cmsbuild commented Sep 28, 2022

tvami commented Sep 28, 2022

cmsbuild commented Sep 28, 2022

Comparison Summary

tvami commented Sep 28, 2022

mmusich commented Sep 28, 2022

cmsbuild commented Sep 28, 2022

tvami commented Oct 3, 2022

tvami commented Oct 10, 2022

tvami commented Oct 10, 2022

cmsbuild commented Oct 10, 2022

cmsbuild commented Oct 10, 2022

cmsbuild commented Oct 10, 2022

Comparison Summary

tvami commented Oct 11, 2022

emanueleusai commented Oct 11, 2022

perrotta commented Oct 11, 2022

sroychow commented Oct 11, 2022

cmsbuild commented Oct 11, 2022

perrotta commented Oct 11, 2022

tvami commented Sep 28, 2022 •

edited

Loading