HCAL: small fix of HcalTBSource #38910

abdoulline · 2022-08-01T05:17:09Z

PR description:

Fixes a handling of skipEvents parameter
which functionally is similar to that of PoolSource, but specific for HcalTBSource producer.
Used for processing of HCAL local runs (pedestals, led, laser etc.)

PR validation:

As this code is not used in any standard workflow, just for reading HCAL local runs data,
checked by me and one of my colleagues in private.

NB: this is not a backport

cmsbuild · 2022-08-01T05:22:48Z

+code-checks

Logs: https://cmssdt.cern.ch/SDT/code-checks/cms-sw-PR-38910/31351

This PR adds an extra 12KB to repository

cmsbuild · 2022-08-01T05:23:08Z

A new Pull Request was created by @abdoulline (Salavat Abdullin) for master.

It involves the following packages:

IORawData/HcalTBInputService (alca)

@malbouis, @yuanchao, @cmsbuild, @francescobrivio, @ChrisMisan, @tvami can you please review it and eventually sign? Thanks.
@bsunanda, @mmusich, @tocheng this is something you requested to watch as well.
@perrotta, @dpiparo, @qliphy, @rappoccio you are the release manager for this.

cms-bot commands are listed here

ChrisMisan · 2022-08-01T08:43:01Z

type hcal, bug-fix

ChrisMisan · 2022-08-01T08:43:41Z

please test

tvami · 2022-08-01T12:06:47Z

IORawData/HcalTBInputService/plugins/HcalTBSource.cc

+  m_i = m_skip;
+
+  if (m_skip != 0)
+    edm::LogWarning("HcalTBSource: skipEvents != 0 works only for the first input file ") << std::endl;


Suggested change

edm::LogWarning("HcalTBSource: skipEvents != 0 works only for the first input file ") << std::endl;

edm::LogWarning("HcalTBSource") << "skipEvents != 0 works only for the first input file";

this is the correct structure

you dont need std::endl, the LogWarning takes care of that

Sorry, as a (frequent) user of sdt::cout (during debugging) added it kind of "automatically"... Now has removed.

cmsbuild · 2022-08-01T12:27:36Z

+1

Summary: https://cmssdt.cern.ch/SDT/jenkins-artifacts/pull-request-integration/PR-4a2336/26556/summary.html
COMMIT: d19559c
CMSSW: CMSSW_12_5_X_2022-07-31-2300/el8_amd64_gcc10
User test area: For local testing, you can use /cvmfs/cms-ci.cern.ch/week0/cms-sw/cmssw/38910/26556/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: 10 differences found in the comparisons
DQMHistoTests: Total files compared: 51
DQMHistoTests: Total histograms compared: 3669004
DQMHistoTests: Total failures: 19
DQMHistoTests: Total nulls: 1
DQMHistoTests: Total successes: 3668962
DQMHistoTests: Total skipped: 22
DQMHistoTests: Total Missing objects: 0
DQMHistoSizes: Histogram memory added: 0.004 KiB( 50 files compared)
DQMHistoSizes: changed ( 312.0 ): 0.004 KiB MessageLogger/Warnings
Checked 210 log files, 47 edm output root files, 51 DQM output files
TriggerResults: no differences found

cmsbuild · 2022-08-01T13:32:51Z

+code-checks

Logs: https://cmssdt.cern.ch/SDT/code-checks/cms-sw-PR-38910/31361

This PR adds an extra 12KB to repository

cmsbuild · 2022-08-01T13:33:24Z

Pull request #38910 was updated. @malbouis, @yuanchao, @cmsbuild, @saumyaphor4252, @francescobrivio, @ChrisMisan, @tvami can you please check and sign again.

tvami · 2022-08-01T13:33:24Z

IORawData/HcalTBInputService/plugins/HcalTBSource.cc

+  m_i = m_skip;
+
+  if (m_skip != 0)
+    edm::LogWarning("HcalTBSource: skipEvents != 0 works only for the first input file ");


Suggested change

edm::LogWarning("HcalTBSource: skipEvents != 0 works only for the first input file ");

edm::LogWarning("HcalTBSource") << "skipEvents != 0 works only for the first input file";

haha

You only dealt with half of what I suggested earlier :)

@abdoulline also, can you please squash these 3 commits now into 1? 3 commits changing 1 file is not great :) Thanks

abdoulline · 2022-08-01T17:10:25Z

@tvami
Sorry once again for a mess with this small update, now even squash didn't work for me (after committing the 3d update):
git reset --soft d19559c
git commit --amend

I've got with
git push my-cmssw fix_HcalTBSource :

! [rejected] fix_HcalTBSource -> fix_HcalTBSource (non-fast-forward)
error: failed to push some refs to 'git@github.com:abdoulline/cmssw.git'
hint: Updates were rejected because the tip of your current branch is behind its remote counterpart.

Probably better would be to close this PR and (re)submit it properly...

ChrisMisan · 2022-08-01T17:16:36Z

@abdoulline this is because you need to do a force push after squashing.

tvami · 2022-08-01T17:18:44Z

git push git@github.com:abdoulline/cmssw.git +HEAD:fix_HcalTBSource

cmsbuild · 2022-08-01T17:51:01Z

+code-checks

Logs: https://cmssdt.cern.ch/SDT/code-checks/cms-sw-PR-38910/31369

This PR adds an extra 12KB to repository

cmsbuild · 2022-08-01T17:51:27Z

Pull request #38910 was updated. @malbouis, @yuanchao, @cmsbuild, @saumyaphor4252, @francescobrivio, @ChrisMisan, @tvami can you please check and sign again.

tvami · 2022-08-01T17:54:20Z

@cmsbuild , please test

cmsbuild · 2022-08-01T21:56:09Z

+1

Summary: https://cmssdt.cern.ch/SDT/jenkins-artifacts/pull-request-integration/PR-4a2336/26575/summary.html
COMMIT: 1ad781f
CMSSW: CMSSW_12_5_X_2022-08-01-1100/el8_amd64_gcc10
User test area: For local testing, you can use /cvmfs/cms-ci.cern.ch/week0/cms-sw/cmssw/38910/26575/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: 51
DQMHistoTests: Total histograms compared: 3669004
DQMHistoTests: Total failures: 8
DQMHistoTests: Total nulls: 0
DQMHistoTests: Total successes: 3668974
DQMHistoTests: Total skipped: 22
DQMHistoTests: Total Missing objects: 0
DQMHistoSizes: Histogram memory added: 0.0 KiB( 50 files compared)
Checked 210 log files, 47 edm output root files, 51 DQM output files
TriggerResults: no differences found

tvami · 2022-08-01T22:06:50Z

+alca

test pass
PR according to description

As this code is not used in any standard workflow, just for reading HCAL local runs data,
checked by me and one of my colleagues in private.

Maybe you could implement a unit test for next time?

cmsbuild · 2022-08-01T22:07: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, @qliphy, @rappoccio (and backports should be raised in the release meeting by the corresponding L2)

qliphy · 2022-08-02T00:45:54Z

+1

cmsbuild added this to the CMSSW_12_5_X milestone Aug 1, 2022

cmsbuild added alca-pending code-checks-pending orp-pending pending-signatures tests-pending labels Aug 1, 2022

cmsbuild added code-checks-approved and removed code-checks-pending labels Aug 1, 2022

cmsbuild added bug-fix hcal labels Aug 1, 2022

cmsbuild added tests-started and removed tests-pending labels Aug 1, 2022

tvami reviewed Aug 1, 2022

View reviewed changes

cmsbuild added tests-approved code-checks-pending tests-pending and removed tests-started tests-approved code-checks-approved labels Aug 1, 2022

cmsbuild added code-checks-approved and removed code-checks-pending labels Aug 1, 2022

tvami reviewed Aug 1, 2022

View reviewed changes

fix skipEvents squashed

1ad781f

abdoulline force-pushed the fix_HcalTBSource branch from 523e71b to 1ad781f Compare August 1, 2022 17:43

cmsbuild added code-checks-pending and removed code-checks-approved labels Aug 1, 2022

cmsbuild added code-checks-approved and removed code-checks-pending labels Aug 1, 2022

cmsbuild added tests-started and removed tests-pending labels Aug 1, 2022

cmsbuild added tests-approved and removed tests-started labels Aug 1, 2022

cmsbuild added alca-approved fully-signed and removed alca-pending pending-signatures labels Aug 1, 2022

cmsbuild added orp-approved and removed orp-pending labels Aug 2, 2022

cmsbuild merged commit 23b9f39 into cms-sw:master Aug 2, 2022

This was referenced Aug 2, 2022

Update run for DQM beamhlt client unitTest #38925

Merged

[ROOT626] Root update v6 26 00 patches cms-sw/cmsdist#8010

Merged

abdoulline deleted the fix_HcalTBSource branch January 7, 2023 06:47

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

HCAL: small fix of HcalTBSource #38910

HCAL: small fix of HcalTBSource #38910

abdoulline commented Aug 1, 2022 •

edited

cmsbuild commented Aug 1, 2022

cmsbuild commented Aug 1, 2022

ChrisMisan commented Aug 1, 2022

ChrisMisan commented Aug 1, 2022

tvami Aug 1, 2022

abdoulline Aug 1, 2022

cmsbuild commented Aug 1, 2022

cmsbuild commented Aug 1, 2022

cmsbuild commented Aug 1, 2022

tvami Aug 1, 2022

tvami Aug 1, 2022

tvami Aug 1, 2022

abdoulline commented Aug 1, 2022 •

edited

ChrisMisan commented Aug 1, 2022

tvami commented Aug 1, 2022

cmsbuild commented Aug 1, 2022

cmsbuild commented Aug 1, 2022

tvami commented Aug 1, 2022

cmsbuild commented Aug 1, 2022

tvami commented Aug 1, 2022

cmsbuild commented Aug 1, 2022

qliphy commented Aug 2, 2022

	edm::LogWarning("HcalTBSource: skipEvents != 0 works only for the first input file ") << std::endl;
	edm::LogWarning("HcalTBSource") << "skipEvents != 0 works only for the first input file";

HCAL: small fix of HcalTBSource #38910

HCAL: small fix of HcalTBSource #38910

Conversation

abdoulline commented Aug 1, 2022 • edited

PR description:

PR validation:

cmsbuild commented Aug 1, 2022

cmsbuild commented Aug 1, 2022

ChrisMisan commented Aug 1, 2022

ChrisMisan commented Aug 1, 2022

tvami Aug 1, 2022

Choose a reason for hiding this comment

abdoulline Aug 1, 2022

Choose a reason for hiding this comment

cmsbuild commented Aug 1, 2022

Comparison Summary

cmsbuild commented Aug 1, 2022

cmsbuild commented Aug 1, 2022

tvami Aug 1, 2022

Choose a reason for hiding this comment

tvami Aug 1, 2022

Choose a reason for hiding this comment

tvami Aug 1, 2022

Choose a reason for hiding this comment

abdoulline commented Aug 1, 2022 • edited

ChrisMisan commented Aug 1, 2022

tvami commented Aug 1, 2022

cmsbuild commented Aug 1, 2022

cmsbuild commented Aug 1, 2022

tvami commented Aug 1, 2022

cmsbuild commented Aug 1, 2022

Comparison Summary

tvami commented Aug 1, 2022

cmsbuild commented Aug 1, 2022

qliphy commented Aug 2, 2022

abdoulline commented Aug 1, 2022 •

edited

abdoulline commented Aug 1, 2022 •

edited