Moving esConsumes statement from the analyze method into the constructor #37650

igv4321 · 2022-04-21T19:53:47Z

PR description:

Moving esConsumes statement from the "analyze" method into the constructor.

PR validation:

Checked that the database read/write sequence for HFPhase1PMTParams objects works as described in CondTools/Hcal/test/readme_HFPhase1PMTParams.txt

This code is not used by any workflows (calibration tables preparation only), so default matrix tests were not run.

cmsbuild · 2022-04-21T20:00:31Z

+code-checks

Logs: https://cmssdt.cern.ch/SDT/code-checks/cms-sw-PR-37650/29448

This PR adds an extra 12KB to repository

cmsbuild · 2022-04-21T20:00:55Z

A new Pull Request was created by @igv4321 (Igor Volobouev) for master.

It involves the following packages:

CondTools/Hcal (db)

@cmsbuild, @ggovi, @tvami, @malbouis, @francescobrivio can you please review it and eventually sign? Thanks.
@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

tvami · 2022-04-21T20:22:29Z

CondTools/Hcal/interface/BoostIODBReader.h

@@ -51,16 +51,16 @@ class BoostIODBReader : public edm::one::EDAnalyzer<> {
  void analyze(const edm::Event&, const edm::EventSetup&) override;

  std::string outputFile_;
+  edm::ESGetToken<DataType, RecordType> tok_;


Suggested change

edm::ESGetToken<DataType, RecordType> tok_;

const edm::ESGetToken<DataType, RecordType> tok_;

OK, changed as suggested.

tvami · 2022-04-21T20:26:05Z

Hi @igv4321 thanks for doing this change!
I believe cmsRun HFPhase1PMTParamsDBWriter_cfg.py and the corresponding readers could easily be made into a unit test, and I think we should do it. Have you done a unit test for CMSSW before? Should I point you to some example? Thanks.

igv4321 · 2022-04-21T21:10:27Z

@tvami I don't remember writing unit tests... Yes, please point out some example.

cmsbuild · 2022-04-21T21:14:23Z

+code-checks

Logs: https://cmssdt.cern.ch/SDT/code-checks/cms-sw-PR-37650/29449

This PR adds an extra 12KB to repository

cmsbuild · 2022-04-21T21:14:47Z

Pull request #37650 was updated. @cmsbuild, @ggovi, @tvami, @malbouis, @francescobrivio can you please check and sign again.

tvami · 2022-04-21T21:16:00Z

@igv4321
here is an example, the BuildFile
https://github.com/cms-sw/cmssw/blob/master/CondTools/SiPixel/test/BuildFile.xml

launches the test createTestDBObjects.sh
which then does the cmsRun

https://github.com/cms-sw/cmssw/blob/master/CondTools/SiPixel/test/createTestDBObjects.sh#L7-L8

And then a next line could read the sqlite file being created by the HFPhase1PMTParamsDBWriter_cfg

igv4321 · 2022-04-21T22:23:35Z

@tvami Ok, thanks. Unit test added.

cmsbuild · 2022-04-21T22:28:24Z

+code-checks

Logs: https://cmssdt.cern.ch/SDT/code-checks/cms-sw-PR-37650/29450

This PR adds an extra 16KB to repository

cmsbuild · 2022-04-21T22:28:46Z

Pull request #37650 was updated. @cmsbuild, @ggovi, @tvami, @malbouis, @francescobrivio can you please check and sign again.

tvami · 2022-04-21T22:43:50Z

@cmsbuild , please test

cmsbuild · 2022-04-22T03:22:38Z

+1

Summary: https://cmssdt.cern.ch/SDT/jenkins-artifacts/pull-request-integration/PR-d93e44/24117/summary.html
COMMIT: 7ac6e39
CMSSW: CMSSW_12_4_X_2022-04-21-1100/slc7_amd64_gcc10
User test area: For local testing, you can use /cvmfs/cms-ci.cern.ch/week1/cms-sw/cmssw/37650/24117/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-d93e44/39434.75_TTbar_14TeV+2026D88_HLT75e33+TTbar_14TeV_TuneCP5_GenSimHLBeamSpot14+DigiTrigger+RecoGlobal+HLT75e33+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: 3695434
DQMHistoTests: Total failures: 7
DQMHistoTests: Total nulls: 1
DQMHistoTests: Total successes: 3695404
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 205 log files, 45 edm output root files, 49 DQM output files
TriggerResults: no differences found

tvami · 2022-04-22T14:12:10Z

+db

new unit test passes: https://cmssdt.cern.ch/SDT/jenkins-artifacts/pull-request-integration/PR-d93e44/24117/unitTests/src/CondTools/Hcal/test/HFPhase1PMTParams_unittest/testing.log

cmsbuild · 2022-04-22T14:12:29Z

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 · 2022-04-22T20:26:06Z

+1

qliphy · 2022-04-23T12:19:33Z

@igv4321 there is a unit test failure in IB after this PR gets merged. Please have a check and fix it:

https://cmssdt.cern.ch/SDT/cgi-bin/logreader/slc7_amd64_gcc10/CMSSW_12_4_X_2022-04-22-2300/unitTestLogs/CondTools/Hcal#/19-19
ata/cmsbld/jenkins/workspace/ib-run-qa/CMSSW_12_4_X_2022-04-22-2300/src/CondTools/Hcal/test/testHFPhase1PMTParams.sh: line 6: /data/cmsbld/jenkins/workspace/ib-run-qa/CMSSW_12_4_X_2022-04-22-2300/test/slc7_amd64_gcc10/write_HFPhase1PMTParams: No such file or directory
Failure running write_HFPhase1PMTParams: status 127
status = 32512

---> test HFPhase1PMTParams_unittest had ERRORS
TestTime:1
^^^^ End Test HFPhase1PMTParams_unittest ^^^^

igv4321 · 2022-04-23T13:02:00Z

I can't reproduce it, but looking at the log file, this problem could be due to an out-of-order execution of tests in the production environment. @tvami Is there some way to ensure that the <bin file="write_HFPhase1PMTParams.cc"> branch of CondTools/Hcal/test/BuildFile.xml (which actually builds the "write_HFPhase1PMTParams" executable) is completed before the <bin file="HFPhase1PMTParams_unittest.cc"> branch is run?

mmusich · 2022-04-23T13:10:20Z

Is there some way to ensure that the branch of CondTools/Hcal/test/BuildFile.xml (which actually builds the "write_HFPhase1PMTParams" executable) is completed before the branch is run?

The flag PRE_TEST should do the trick:

e.g.:

cmssw/Alignment/OfflineValidation/test/BuildFile.xml

Line 8 in d302344

igv4321 · 2022-04-23T13:36:50Z

OK, thanks. The fix is provided in the PR #37666

tvami · 2022-04-23T15:00:11Z

Thanks @igv4321 for the fix.
@qliphy why didnt the unit test in here https://cmssdt.cern.ch/SDT/jenkins-artifacts/pull-request-integration/PR-d93e44/24117/unitTests/src/CondTools/Hcal/test/HFPhase1PMTParams_unittest/testing.log catch this? Should I test the bugfix PR differently? (for example with the full CMSSW setup?)

smuzaffar · 2022-04-23T16:14:10Z

@tvami , for PR tests and local tests ${CMSSW_BASE}/test/${SCRAM_ARCH}/write_HFPhase1PMTParams exists but for IB tests it does not. For IB tests, we just create a cmssw dev area and all the tests executables are in $CMSSW_RELEASE_BASE. As for unit tests the test/arch directories are in PATH so just run write_HFPhase1PMTParams

Moving esConsumes statement from the analyze method into the constructor

bc509e4

cmsbuild added this to the CMSSW_12_4_X milestone Apr 21, 2022

cmsbuild added code-checks-pending db-pending orp-pending pending-signatures tests-pending labels Apr 21, 2022

cmsbuild added code-checks-approved and removed code-checks-pending labels Apr 21, 2022

tvami reviewed Apr 21, 2022

View reviewed changes

constifying members

d7272da

cmsbuild added code-checks-pending and removed code-checks-approved labels Apr 21, 2022

cmsbuild added code-checks-approved and removed code-checks-pending labels Apr 21, 2022

adding a unit test for HFPhase1PMTParams database read/write

7ac6e39

cmsbuild added code-checks-pending and removed code-checks-approved labels Apr 21, 2022

cmsbuild added code-checks-approved and removed code-checks-pending labels Apr 21, 2022

cmsbuild added tests-started and removed tests-pending labels Apr 21, 2022

cmsbuild added tests-approved and removed tests-started labels Apr 22, 2022

cmsbuild added db-approved fully-signed and removed db-pending pending-signatures labels Apr 22, 2022

cmsbuild added orp-approved and removed orp-pending labels Apr 22, 2022

cmsbuild merged commit 966f784 into cms-sw:master Apr 22, 2022

igv4321 mentioned this pull request Apr 23, 2022

Fixing the order of unit tests in CondTools/Hcal #37666

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Moving esConsumes statement from the analyze method into the constructor #37650

Moving esConsumes statement from the analyze method into the constructor #37650

igv4321 commented Apr 21, 2022

cmsbuild commented Apr 21, 2022

cmsbuild commented Apr 21, 2022

tvami Apr 21, 2022

igv4321 Apr 21, 2022

tvami commented Apr 21, 2022

igv4321 commented Apr 21, 2022

cmsbuild commented Apr 21, 2022

cmsbuild commented Apr 21, 2022

tvami commented Apr 21, 2022 •

edited

Loading

igv4321 commented Apr 21, 2022

cmsbuild commented Apr 21, 2022

cmsbuild commented Apr 21, 2022

tvami commented Apr 21, 2022

cmsbuild commented Apr 22, 2022

tvami commented Apr 22, 2022

cmsbuild commented Apr 22, 2022

perrotta commented Apr 22, 2022

qliphy commented Apr 23, 2022

igv4321 commented Apr 23, 2022 •

edited

Loading

mmusich commented Apr 23, 2022

igv4321 commented Apr 23, 2022

tvami commented Apr 23, 2022

smuzaffar commented Apr 23, 2022

	edm::ESGetToken<DataType, RecordType> tok_;
	const edm::ESGetToken<DataType, RecordType> tok_;

Moving esConsumes statement from the analyze method into the constructor #37650

Moving esConsumes statement from the analyze method into the constructor #37650

Conversation

igv4321 commented Apr 21, 2022

PR description:

PR validation:

cmsbuild commented Apr 21, 2022

cmsbuild commented Apr 21, 2022

tvami Apr 21, 2022

Choose a reason for hiding this comment

igv4321 Apr 21, 2022

Choose a reason for hiding this comment

tvami commented Apr 21, 2022

igv4321 commented Apr 21, 2022

cmsbuild commented Apr 21, 2022

cmsbuild commented Apr 21, 2022

tvami commented Apr 21, 2022 • edited Loading

igv4321 commented Apr 21, 2022

cmsbuild commented Apr 21, 2022

cmsbuild commented Apr 21, 2022

tvami commented Apr 21, 2022

cmsbuild commented Apr 22, 2022

Comparison Summary

tvami commented Apr 22, 2022

cmsbuild commented Apr 22, 2022

perrotta commented Apr 22, 2022

qliphy commented Apr 23, 2022

igv4321 commented Apr 23, 2022 • edited Loading

mmusich commented Apr 23, 2022

igv4321 commented Apr 23, 2022

tvami commented Apr 23, 2022

smuzaffar commented Apr 23, 2022

tvami commented Apr 21, 2022 •

edited

Loading

igv4321 commented Apr 23, 2022 •

edited

Loading