fixing obsolete EDAnalyzer in DQM/BeamMonitor #37071

emanueleusai · 2022-02-25T20:52:07Z

PR description:

replacing EDAnalyzer with the appropriate edm::one version for various plugins in the DQM/BeamMonitor package.
No changes to ME expected.

PR validation:

DQM/BeamMonitor now compiles on CMSSW_12_3_CMSDEPRECATED_X_2022-02-21-2300 without warnings

if this PR is a backport please specify the original PR and why you need to backport that PR:

Not a backport

emanueleusai · 2022-02-25T20:56:22Z

assign alca

cmsbuild · 2022-02-25T20:59:35Z

-code-checks

Logs: https://cmssdt.cern.ch/SDT/code-checks/cms-sw-PR-37071/28535

This PR adds an extra 20KB to repository

Code check has found code style and quality issues which could be resolved by applying following patch(s)

code-format:
https://cmssdt.cern.ch/SDT/code-checks/cms-sw-PR-37071/28535/code-format.patch
e.g. curl -k https://cmssdt.cern.ch/SDT/code-checks/cms-sw-PR-37071/28535/code-format.patch | patch -p1
You can also run scram build code-format to apply code format directly

emanueleusai · 2022-02-25T21:06:27Z

addresses part of #36404

mmusich · 2022-02-25T21:11:07Z

There's an earlier attempt from @francescobrivio at #35501. We've been wondering if a DQMEDAnalyzer type should not be used instead: #35501 (comment)
Perhaps you can clarify.

cmsbuild · 2022-02-25T21:11:11Z

+code-checks

Logs: https://cmssdt.cern.ch/SDT/code-checks/cms-sw-PR-37071/28536

This PR adds an extra 20KB to repository

cmsbuild · 2022-02-25T21:11:27Z

New categories assigned: alca

@yuanchao,@francescobrivio,@malbouis,@tvami you have been requested to review this Pull request/Issue and eventually sign? Thanks

cmsbuild · 2022-02-25T21:11:29Z

A new Pull Request was created by @emanueleusai (Emanuele Usai) for master.

It involves the following packages:

DQM/BeamMonitor (dqm)

@malbouis, @yuanchao, @pmandrik, @emanueleusai, @ahmad3213, @tvami, @cmsbuild, @jfernan2, @francescobrivio, @pbo0, @rvenditti can you please review it and eventually sign? Thanks.
@perrotta, @dpiparo, @qliphy you are the release manager for this.

cms-bot commands are listed here

emanueleusai · 2022-02-25T21:50:44Z

@mmusich Interesting. Somehow I forgot about @francescobrivio 's previous attempt. Anyway the deadline for the transition is now imminent so I think it's time to discuss this again.

I implemented it in a slightly different way wrt Francesco, so let's see the result of the tests.

Concerning the use of DQMOneEDAnalyzer, I largely agree with the discussion in the previous PR. Perhaps one can create a new DQMOne"LumiAndRun"EDAnalyzer that templates both edm:: one ::WatchLuminosityBlocks and edm:: one ::WatchRuns. But then one needs to rewrite AlcaBeamMonitorClient & others to access DQMStore through the DQMOne*EDAnalyzer interface rather than accessing it directly. I think just changing the class type isn't going to work.

emanueleusai · 2022-02-25T21:51:08Z

please test

tvami · 2022-02-25T21:53:18Z

DQM/BeamMonitor/plugins/BeamConditionsMonitor.h

  // BeginRun
-  void beginRun(const edm::Run& r, const edm::EventSetup& c) override;
+  // void beginRun(const edm::Run& r, const edm::EventSetup& c) override;


Should these be removed for good? (also the endRun)

yup I forgot to remove them after testing

emanueleusai · 2022-02-25T21:55:19Z

please abort test

cmsbuild · 2022-02-25T22:05:36Z

+code-checks

Logs: https://cmssdt.cern.ch/SDT/code-checks/cms-sw-PR-37071/28541

This PR adds an extra 20KB to repository

cmsbuild · 2022-02-25T22:06:01Z

Pull request #37071 was updated. @malbouis, @yuanchao, @pmandrik, @emanueleusai, @ahmad3213, @tvami, @cmsbuild, @jfernan2, @francescobrivio, @pbo0, @rvenditti can you please check and sign again.

emanueleusai · 2022-02-25T22:07:39Z

please test

mmusich · 2022-02-25T22:29:37Z

I implemented it in a slightly different way wrt Francesco, so let's see the result of the tests.

Let me rephrase. What's the preferred strategy from the DQM group? Clearly the plain old EDAnalyzer class that we have now is still technically working, but there must be some advantage computing-wise in using the producer-based types. Aren't one modules notouriously bad at threading efficiency?

others to access DQMStore through the DQMOne*EDAnalyzer interface rather than accessing it directly. I think just changing the class type isn't going to work.

Right, I admit I didn't follow much further Francesco's PR, but clearly just changing the method signatures is not gonna cut it, the iBooker and iGetter objects shall be used for interacting with the DQMStore

cmsbuild · 2022-02-26T04:30:14Z

+1

Summary: https://cmssdt.cern.ch/SDT/jenkins-artifacts/pull-request-integration/PR-c66ac4/22686/summary.html
COMMIT: 5962add
CMSSW: CMSSW_12_3_X_2022-02-25-1100/slc7_amd64_gcc10
User test area: For local testing, you can use /cvmfs/cms-ci.cern.ch/week1/cms-sw/cmssw/37071/22686/install.sh to create a dev area with all the needed externals and cmssw changes.

The following merge commits were also included on top of IB + this PR after doing git cms-merge-topic:

You can see more details here:
https://cmssdt.cern.ch/SDT/jenkins-artifacts/pull-request-integration/PR-c66ac4/22686/git-recent-commits.json
https://cmssdt.cern.ch/SDT/jenkins-artifacts/pull-request-integration/PR-c66ac4/22686/git-merge-result

Comparison Summary

Summary:

No significant changes to the logs found
Reco comparison results: 1646 differences found in the comparisons
DQMHistoTests: Total files compared: 49
DQMHistoTests: Total histograms compared: 4001143
DQMHistoTests: Total failures: 1762
DQMHistoTests: Total nulls: 0
DQMHistoTests: Total successes: 3999359
DQMHistoTests: Total skipped: 22
DQMHistoTests: Total Missing objects: 0
DQMHistoSizes: Histogram memory added: 0.0 KiB( 48 files compared)
Checked 204 log files, 45 edm output root files, 49 DQM output files
TriggerResults: found differences in 2 / 48 workflows

mmusich · 2022-02-28T09:52:27Z

there's plenty of failures, presumably coming from the inclusion of the commits of #37034 in this PR.

francescobrivio

I agree with @emanueleusai that a new DQMStore class could be used, but that would require a larger work to update all the plugins, maybe it can be posponed to later given that technically the one:EDAnalyzer class is technically ok for the moment.
As @mmusich said @cms-sw/dqm-l2 maybe you can comment on what is the preferred solution from DQM point of view?

francescobrivio · 2022-02-28T09:36:09Z

DQM/BeamMonitor/plugins/AlcaBeamMonitorClient.cc

@@ -70,6 +70,8 @@ void AlcaBeamMonitorClient::beginRun(const edm::Run& r, const EventSetup& contex
 void AlcaBeamMonitorClient::analyze(const Event& iEvent, const EventSetup& iSetup) {}

 //----------------------------------------------------------------------------------------------------------------------
+void AlcaBeamMonitorClient::beginLuminosityBlock(const LuminosityBlock& iLumi, const EventSetup& iSetup) {}


Not related to this line, but shouldn't the ~AlcaBeamMonitorClient() {} be changed to ~AlcaBeamMonitorClient() = default; ?

the same comment can be applied to

BeamConditionsMonitor.cc

PixelVTXMonitor.cc

Vx3DHLTAnalyzer.cc

good point, I'll fix that too

emanueleusai · 2022-03-02T21:34:06Z

@mmusich @francescobrivio We discussed this internally with the other @cms-sw/dqm-l2 . Our preferred position would be to first move from EDAnalyzer to one::EDAnalyzer or similar (e.g. DQMEDAnalyzer to DQMOneEDAnalyzer) in order to comply with the deprecation of EDAnalyzer. Then later on, in a separate set of PRs, address the issue of the DQM modules still not using DQM*EDAnalyzer and make sure DQMStore is not accessed directly, but only through the iBooker/iGetter methods.

mmusich · 2022-03-02T21:36:28Z

@cmsbuild, please test

to get a clean comparison

makortel · 2022-03-02T21:37:32Z

From core point of view it would be preferable to address the real issues at the same time the base class type is changed. On a quick peek on DQMStore and DQMEDAnalyzer base classes I was not able to convince myself if it would be safe to use DQMStore directly from two non-DQMEDAnalyzer modules concurrently.

emanueleusai · 2022-03-02T22:59:28Z

@makortel thank you for giving the core point of view on this. I can make an attempt at rewriting the module(s) to use DQM*EDAnalyzer but it'd be safer if BeamMonitor experts (AlCa developers I assume) can look into it if we go this way.

cmsbuild · 2022-03-03T01:34:00Z

+1

Summary: https://cmssdt.cern.ch/SDT/jenkins-artifacts/pull-request-integration/PR-c66ac4/22786/summary.html
COMMIT: 5962add
CMSSW: CMSSW_12_3_X_2022-03-02-1100/slc7_amd64_gcc10
User test area: For local testing, you can use /cvmfs/cms-ci.cern.ch/week0/cms-sw/cmssw/37071/22786/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: 6 differences found in the comparisons
DQMHistoTests: Total files compared: 49
DQMHistoTests: Total histograms compared: 3990927
DQMHistoTests: Total failures: 13
DQMHistoTests: Total nulls: 1
DQMHistoTests: Total successes: 3990891
DQMHistoTests: Total skipped: 22
DQMHistoTests: Total Missing objects: 0
DQMHistoSizes: Histogram memory added: -0.004 KiB( 48 files compared)
DQMHistoSizes: changed ( 312.0 ): -0.004 KiB MessageLogger/Warnings
Checked 204 log files, 45 edm output root files, 49 DQM output files
TriggerResults: no differences found

francescobrivio · 2022-03-03T14:22:01Z

@makortel thank you for giving the core point of view on this. I can make an attempt at rewriting the module(s) to use DQM*EDAnalyzer but it'd be safer if BeamMonitor experts (AlCa developers I assume) can look into it if we go this way.

I am the only developer at the moment and I would need some time to (digest and) test the correct way to update these plugins (maybe following Javi's suggestion from #35501 (comment) since my last attempt with DQM*EDAnalyzer wasn't successful).
IIUC the current implementation from @emanueleusai is technically correct although not the preferred one from core.
So I guess the question is back to @cms-sw/dqm-l2 and @makortel on they urgency of this update?
The options are:

we merge this one moving away from deprecated APIs now and posponing the update to DQM*EDAnalyzer to later
we reject this one keep having the deprecated EDAnalyzer till I have time to fix it all in one go

mmusich · 2022-03-03T14:28:13Z

I am the only developer at the moment

I guess I can help with that.

we merge this one moving away from deprecated APIs now and posponing the update to DQM*EDAnalyzer to later

IMHO this solution is equivalent to sweeping the dust under the rug. The deprecation warning is gone, but then there's no guarantee it won't fail in production.

francescobrivio · 2022-03-03T14:44:02Z

-1

After private conversation with @mmusich we decided to give it a try and do all the changes at the same time as soon as possible. So we can close this.
@makortel could you remind me the timelines for moving away from deprecated EDAnalyzer?

makortel · 2022-03-03T14:57:49Z

After private conversation with @mmusich we decided to give it a try and do all the changes at the same time as soon as possible. So we can close this.

Thanks!

@makortel could you remind me the timelines for moving away from deprecated EDAnalyzer?

The goal to migrate (at minimum) all legacy EDModules being used in production is by 13_0_0 (at which point we'll change the current warning message of such modules to an exception).

fixing obsolete EDAnalyzer in DQM/BeamMonitor

57241db

cmsbuild added this to the CMSSW_12_3_X milestone Feb 25, 2022

cmsbuild added code-checks-pending dqm-pending orp-pending pending-signatures tests-pending labels Feb 25, 2022

cmsbuild added the alca-pending label Feb 25, 2022

cmsbuild added code-checks-rejected and removed code-checks-pending labels Feb 25, 2022

fix code style

5769913

cmsbuild added code-checks-pending and removed code-checks-rejected labels Feb 25, 2022

cmsbuild added code-checks-approved and removed code-checks-pending labels Feb 25, 2022

cmsbuild added tests-started and removed tests-pending labels Feb 25, 2022

tvami reviewed Feb 25, 2022

View reviewed changes

cmsbuild added tests-pending and removed tests-started labels Feb 25, 2022

cmsbuild added code-checks-approved and removed code-checks-pending labels Feb 25, 2022

cmsbuild added tests-started and removed tests-pending labels Feb 25, 2022

cmsbuild added tests-approved and removed tests-started labels Feb 26, 2022

francescobrivio reviewed Feb 28, 2022

View reviewed changes

cmsbuild added tests-started and removed tests-approved labels Mar 2, 2022

cmsbuild added tests-approved and removed tests-started labels Mar 3, 2022

cmsbuild added alca-rejected and removed alca-pending labels Mar 3, 2022

emanueleusai closed this Mar 3, 2022

francescobrivio mentioned this pull request Mar 7, 2022

Modules prohibiting concurrent lumis in RelVal workflows #25090

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

fixing obsolete EDAnalyzer in DQM/BeamMonitor #37071

fixing obsolete EDAnalyzer in DQM/BeamMonitor #37071

emanueleusai commented Feb 25, 2022

emanueleusai commented Feb 25, 2022

cmsbuild commented Feb 25, 2022

emanueleusai commented Feb 25, 2022

mmusich commented Feb 25, 2022

cmsbuild commented Feb 25, 2022

cmsbuild commented Feb 25, 2022

cmsbuild commented Feb 25, 2022

emanueleusai commented Feb 25, 2022 •

edited

Loading

emanueleusai commented Feb 25, 2022

tvami Feb 25, 2022

emanueleusai Feb 25, 2022

emanueleusai commented Feb 25, 2022

cmsbuild commented Feb 25, 2022

cmsbuild commented Feb 25, 2022

emanueleusai commented Feb 25, 2022

mmusich commented Feb 25, 2022

cmsbuild commented Feb 26, 2022

mmusich commented Feb 28, 2022

francescobrivio left a comment

francescobrivio Feb 28, 2022

francescobrivio Feb 28, 2022

emanueleusai Mar 2, 2022

emanueleusai commented Mar 2, 2022

mmusich commented Mar 2, 2022

makortel commented Mar 2, 2022

emanueleusai commented Mar 2, 2022

cmsbuild commented Mar 3, 2022

francescobrivio commented Mar 3, 2022

mmusich commented Mar 3, 2022

francescobrivio commented Mar 3, 2022

makortel commented Mar 3, 2022

fixing obsolete EDAnalyzer in DQM/BeamMonitor #37071

fixing obsolete EDAnalyzer in DQM/BeamMonitor #37071

Conversation

emanueleusai commented Feb 25, 2022

PR description:

PR validation:

if this PR is a backport please specify the original PR and why you need to backport that PR:

emanueleusai commented Feb 25, 2022

cmsbuild commented Feb 25, 2022

emanueleusai commented Feb 25, 2022

mmusich commented Feb 25, 2022

cmsbuild commented Feb 25, 2022

cmsbuild commented Feb 25, 2022

cmsbuild commented Feb 25, 2022

emanueleusai commented Feb 25, 2022 • edited Loading

emanueleusai commented Feb 25, 2022

tvami Feb 25, 2022

Choose a reason for hiding this comment

emanueleusai Feb 25, 2022

Choose a reason for hiding this comment

emanueleusai commented Feb 25, 2022

cmsbuild commented Feb 25, 2022

cmsbuild commented Feb 25, 2022

emanueleusai commented Feb 25, 2022

mmusich commented Feb 25, 2022

cmsbuild commented Feb 26, 2022

Comparison Summary

mmusich commented Feb 28, 2022

francescobrivio left a comment

Choose a reason for hiding this comment

francescobrivio Feb 28, 2022

Choose a reason for hiding this comment

francescobrivio Feb 28, 2022

Choose a reason for hiding this comment

emanueleusai Mar 2, 2022

Choose a reason for hiding this comment

emanueleusai commented Mar 2, 2022

mmusich commented Mar 2, 2022

makortel commented Mar 2, 2022

emanueleusai commented Mar 2, 2022

cmsbuild commented Mar 3, 2022

Comparison Summary

francescobrivio commented Mar 3, 2022

mmusich commented Mar 3, 2022

francescobrivio commented Mar 3, 2022

makortel commented Mar 3, 2022

emanueleusai commented Feb 25, 2022 •

edited

Loading