[AlCa] Move away from edm::EDAnalyzer LegacyModule in AlCaRecoTriggerBitsRcdRead #36129

malbouis · 2021-11-15T13:42:07Z

PR description:

This is an update of AlCaRecoTriggerBitsRcdRead.cc in order to move from legacy module edm::EDAnalyzer to edm::one::EDAnalyzer.

PR validation:

It compiles.

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

Before submitting your pull requests, make sure you followed this checklist:

verify that the PR is really intended for the chosen branch
verify that changes follow CMS Naming, Coding, And Style Rules
verify that the PR passes the basic test procedure suggested in the CMSSW PR instructions

cmsbuild · 2021-11-15T13:52:27Z

-code-checks

Logs: https://cmssdt.cern.ch/SDT/code-checks/cms-sw-PR-36129/26641

This PR adds an extra 12KB 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-36129/26641/code-format.patch
e.g. curl -k https://cmssdt.cern.ch/SDT/code-checks/cms-sw-PR-36129/26641/code-format.patch | patch -p1
You can also run scram build code-format to apply code format directly

cmsbuild · 2021-11-15T14:08:26Z

+code-checks

Logs: https://cmssdt.cern.ch/SDT/code-checks/cms-sw-PR-36129/26642

This PR adds an extra 12KB to repository

cmsbuild · 2021-11-15T14:08:52Z

A new Pull Request was created by @malbouis for master.

It involves the following packages:

CondTools/HLT (hlt, db)

@malbouis, @cmsbuild, @missirol, @Martin-Grunewald, @ggovi, @francescobrivio, @tvami can you please review it and eventually sign? Thanks.
@silviodonato, @mmusich this is something you requested to watch as well.
@perrotta, @dpiparo, @qliphy you are the release manager for this.

cms-bot commands are listed here

malbouis · 2021-11-15T14:09:38Z

please test

cmsbuild · 2021-11-15T17:11:54Z

+1

Summary: https://cmssdt.cern.ch/SDT/jenkins-artifacts/pull-request-integration/PR-f47090/20541/summary.html
COMMIT: c9780d8
CMSSW: CMSSW_12_2_X_2021-11-15-1100/slc7_amd64_gcc900
User test area: For local testing, you can use /cvmfs/cms-ci.cern.ch/week1/cms-sw/cmssw/36129/20541/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: 42
DQMHistoTests: Total histograms compared: 3320802
DQMHistoTests: Total failures: 0
DQMHistoTests: Total nulls: 0
DQMHistoTests: Total successes: 3320780
DQMHistoTests: Total skipped: 22
DQMHistoTests: Total Missing objects: 0
DQMHistoSizes: Histogram memory added: 0.0 KiB( 41 files compared)
Checked 177 log files, 37 edm output root files, 42 DQM output files
TriggerResults: no differences found

tvami · 2021-11-15T18:03:14Z

Hi @malbouis should the AlCaRecoTriggerBitsRcdUpdate.cc migration be done in this PR as well:
https://cmssdt.cern.ch/SDT/jenkins-artifacts/pull-request-integration/PR-f47090/20541/llvm-analysis/
?

malbouis · 2021-11-15T18:15:32Z

https://cmssdt.cern.ch/SDT/jenkins-artifacts/pull-request-integration/PR-f47090/20541/llvm-analysis/

Good point! I'll update it.

cmsbuild · 2021-11-15T18:24:34Z

-code-checks

Logs: https://cmssdt.cern.ch/SDT/code-checks/cms-sw-PR-36129/26650

This PR adds an extra 16KB 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-36129/26650/code-format.patch
e.g. curl -k https://cmssdt.cern.ch/SDT/code-checks/cms-sw-PR-36129/26650/code-format.patch | patch -p1
You can also run scram build code-format to apply code format directly

cmsbuild · 2021-11-15T20:47:47Z

+code-checks

Logs: https://cmssdt.cern.ch/SDT/code-checks/cms-sw-PR-36129/26654

This PR adds an extra 16KB to repository

cmsbuild · 2021-11-15T20:48:09Z

Pull request #36129 was updated. @malbouis, @cmsbuild, @missirol, @Martin-Grunewald, @ggovi, @francescobrivio, @tvami can you please check and sign again.

malbouis · 2021-11-15T20:50:02Z

please test

cmsbuild · 2021-11-15T23:49:05Z

+1

Summary: https://cmssdt.cern.ch/SDT/jenkins-artifacts/pull-request-integration/PR-f47090/20553/summary.html
COMMIT: f5b8641
CMSSW: CMSSW_12_2_X_2021-11-15-1100/slc7_amd64_gcc900
User test area: For local testing, you can use /cvmfs/cms-ci.cern.ch/week1/cms-sw/cmssw/36129/20553/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: 42
DQMHistoTests: Total histograms compared: 3320802
DQMHistoTests: Total failures: 0
DQMHistoTests: Total nulls: 0
DQMHistoTests: Total successes: 3320780
DQMHistoTests: Total skipped: 22
DQMHistoTests: Total Missing objects: 0
DQMHistoSizes: Histogram memory added: 0.0 KiB( 41 files compared)
Checked 177 log files, 37 edm output root files, 42 DQM output files
TriggerResults: no differences found

tvami · 2021-11-16T00:39:11Z

+db

Martin-Grunewald · 2021-11-16T05:39:52Z

+1

cmsbuild · 2021-11-16T05:40:15Z

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 (and backports should be raised in the release meeting by the corresponding L2)

perrotta · 2021-11-16T06:56:45Z

+1

mmusich · 2021-11-19T10:37:57Z

CondTools/HLT/src/AlCaRecoTriggerBitsRcdUpdate.cc

@@ -28,12 +28,14 @@
 // Rcd for reading old one:
 #include "CondFormats/DataRecord/interface/AlCaRecoTriggerBitsRcd.h"

-class AlCaRecoTriggerBitsRcdUpdate : public edm::EDAnalyzer {
+class AlCaRecoTriggerBitsRcdUpdate : public edm::one::EDAnalyzer<edm::one::WatchRuns> {


I don't understand why this needs to watch runs. Both the beginRun and endRun methods are empty, so there's no need for them

I agree, there seems to be no real need for watch runs. It can be updated it in a future PR, if it is OK with you.

mmusich · 2021-12-03T22:32:39Z

CondTools/HLT/src/AlCaRecoTriggerBitsRcdRead.cc

@@ -132,7 +132,7 @@ void AlCaRecoTriggerBitsRcdRead::beginRun(const edm::Run &run, const edm::EventS
 }

 ///////////////////////////////////////////////////////////////////////
-void AlCaRecoTriggerBitsRcdRead::endJob() {
+void AlCaRecoTriggerBitsRcdRead::endRun(edm::Run const &, edm::EventSetup const &) {


@malbouis there's another mistake here. This should have not been changed to endJob as now this is called at every endRun transition instead that only once for the last examined IOV.
See a fix at #36362.

Sorry about that and thanks for submitting a fix!

`CondTools/HLT` fix mistake done in #36129

cmsbuild added this to the CMSSW_12_2_X milestone Nov 15, 2021

cmsbuild added code-checks-pending db-pending hlt-pending orp-pending pending-signatures tests-pending labels Nov 15, 2021

cmsbuild added code-checks-rejected code-checks-pending and removed code-checks-pending code-checks-rejected labels Nov 15, 2021

cmsbuild added code-checks-approved and removed code-checks-pending labels Nov 15, 2021

cmsbuild added tests-started and removed tests-pending labels Nov 15, 2021

cmsbuild added tests-approved and removed tests-started labels Nov 15, 2021

cmsbuild added code-checks-pending tests-pending and removed tests-approved code-checks-approved labels Nov 15, 2021

cmsbuild removed the code-checks-pending label Nov 15, 2021

cmsbuild added code-checks-approved and removed code-checks-pending labels Nov 15, 2021

cmsbuild added tests-started and removed tests-pending labels Nov 15, 2021

cmsbuild added tests-approved and removed tests-started labels Nov 15, 2021

cmsbuild added db-approved and removed db-pending labels Nov 16, 2021

cmsbuild added fully-signed hlt-approved and removed hlt-pending pending-signatures labels Nov 16, 2021

cmsbuild added orp-approved and removed orp-pending labels Nov 16, 2021

cmsbuild merged commit ccc14bd into cms-sw:master Nov 16, 2021

cmsbuild mentioned this pull request Nov 17, 2021

MuonAlignment ASAN bug fix #36147

Merged

mmusich reviewed Nov 19, 2021

View reviewed changes

mmusich mentioned this pull request Nov 19, 2021

Modernize CondTools/HLT and add unit tests #36187

Merged

mmusich mentioned this pull request Dec 3, 2021

CondTools/HLT fix mistake done in #36129 #36362

Merged

mmusich reviewed Dec 3, 2021

View reviewed changes

cmsbuild added a commit that referenced this pull request Dec 4, 2021

Merge pull request #36362 from mmusich/fixCondToolsHLT_WrongEndJob

239f053

`CondTools/HLT` fix mistake done in #36129

gabrielmscampos mentioned this pull request Dec 15, 2021

ValidateHLTMenu compatibility update to run under CMSSW 12_0_3_patch2 cms-AlCaDB/AlCaTools#52

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[AlCa] Move away from edm::EDAnalyzer LegacyModule in AlCaRecoTriggerBitsRcdRead #36129

[AlCa] Move away from edm::EDAnalyzer LegacyModule in AlCaRecoTriggerBitsRcdRead #36129

malbouis commented Nov 15, 2021 •

edited

Loading

cmsbuild commented Nov 15, 2021

cmsbuild commented Nov 15, 2021

cmsbuild commented Nov 15, 2021

malbouis commented Nov 15, 2021

cmsbuild commented Nov 15, 2021

tvami commented Nov 15, 2021

malbouis commented Nov 15, 2021

cmsbuild commented Nov 15, 2021

cmsbuild commented Nov 15, 2021

cmsbuild commented Nov 15, 2021

malbouis commented Nov 15, 2021

cmsbuild commented Nov 15, 2021

tvami commented Nov 16, 2021

Martin-Grunewald commented Nov 16, 2021

cmsbuild commented Nov 16, 2021

perrotta commented Nov 16, 2021

mmusich Nov 19, 2021

malbouis Nov 19, 2021

mmusich Nov 19, 2021

mmusich Dec 3, 2021

malbouis Dec 4, 2021

[AlCa] Move away from edm::EDAnalyzer LegacyModule in AlCaRecoTriggerBitsRcdRead #36129

[AlCa] Move away from edm::EDAnalyzer LegacyModule in AlCaRecoTriggerBitsRcdRead #36129

Conversation

malbouis commented Nov 15, 2021 • 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:

cmsbuild commented Nov 15, 2021

cmsbuild commented Nov 15, 2021

cmsbuild commented Nov 15, 2021

malbouis commented Nov 15, 2021

cmsbuild commented Nov 15, 2021

Comparison Summary

tvami commented Nov 15, 2021

malbouis commented Nov 15, 2021

cmsbuild commented Nov 15, 2021

cmsbuild commented Nov 15, 2021

cmsbuild commented Nov 15, 2021

malbouis commented Nov 15, 2021

cmsbuild commented Nov 15, 2021

Comparison Summary

tvami commented Nov 16, 2021

Martin-Grunewald commented Nov 16, 2021

cmsbuild commented Nov 16, 2021

perrotta commented Nov 16, 2021

mmusich Nov 19, 2021

Choose a reason for hiding this comment

malbouis Nov 19, 2021

Choose a reason for hiding this comment

mmusich Nov 19, 2021

Choose a reason for hiding this comment

mmusich Dec 3, 2021

Choose a reason for hiding this comment

malbouis Dec 4, 2021

Choose a reason for hiding this comment

malbouis commented Nov 15, 2021 •

edited

Loading