esConsumes in CondTools/CTPPS #34377

jan-kaspar · 2021-07-07T11:25:13Z

PR description:

As a follow up of #31061, this PR makes the modules in CondTools/CTPPS use the esConsumes mechanism.

This is a technical PR, no difference in test results is expected.

PR validation:

Comparison before (blue) vs. after the PR (red dashed)

for "direct simulation" dirsim_cmp.pdf
for reconstruction reco_cmp.pdf

show no differences - as expected.

cmsbuild · 2021-07-07T11:32:12Z

-code-checks

Logs: https://cmssdt.cern.ch/SDT/code-checks/cms-sw-PR-34377/23764

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

cmsbuild · 2021-07-07T11:54:23Z

+code-checks

Logs: https://cmssdt.cern.ch/SDT/code-checks/cms-sw-PR-34377/23765

This PR adds an extra 24KB to repository

cmsbuild · 2021-07-07T11:54:47Z

A new Pull Request was created by @jan-kaspar for master.

It involves the following packages:

CondTools/CTPPS

@ggovi, @cmsbuild can you please review it and eventually sign? Thanks.
@fabferro, @mmusich this is something you requested to watch as well.
@silviodonato, @dpiparo, @qliphy you are the release manager for this.

cms-bot commands are listed here

perrotta · 2021-07-13T06:39:37Z

please test

cmsbuild · 2021-07-13T11:07:35Z

+1

Summary: https://cmssdt.cern.ch/SDT/jenkins-artifacts/pull-request-integration/PR-decd46/16732/summary.html
COMMIT: 9d07a3a
CMSSW: CMSSW_12_0_X_2021-07-12-2300/slc7_amd64_gcc900
User test area: For local testing, you can use /cvmfs/cms-ci.cern.ch/week1/cms-sw/cmssw/34377/16732/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: 38
DQMHistoTests: Total histograms compared: 2787742
DQMHistoTests: Total failures: 883
DQMHistoTests: Total nulls: 0
DQMHistoTests: Total successes: 2786859
DQMHistoTests: Total skipped: 0
DQMHistoTests: Total Missing objects: 0
DQMHistoSizes: Histogram memory added: 0.0 KiB( 37 files compared)
Checked 160 log files, 37 edm output root files, 38 DQM output files
TriggerResults: no differences found

ggovi · 2021-07-15T14:16:05Z

+1

cmsbuild · 2021-07-15T14:16:25Z

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. @silviodonato, @dpiparo, @qliphy, @perrotta (and backports should be raised in the release meeting by the corresponding L2)

jan-kaspar · 2021-07-22T10:02:53Z

In 10fbac2, I've just committed the patch by @wpcarvalho , thanks! This should fix all the test configs except from write-ctpps-beam-parameters_cfg.py which refers to an SQLite file not present in the CMSSW source code. I suggest that this problem is addressed in another (and topical) PR by the original authors @malbouis and @clemencia who know the best how to solve this tiny issue.

cmsbuild · 2021-07-22T10:05:09Z

+code-checks

Logs: https://cmssdt.cern.ch/SDT/code-checks/cms-sw-PR-34377/24137

This PR adds an extra 16KB to repository

cmsbuild · 2021-07-22T10:05:34Z

Pull request #34377 was updated. @ggovi, @cmsbuild can you please check and sign again.

qliphy · 2021-07-24T09:05:06Z

please test

cmsbuild · 2021-07-24T12:26:49Z

+1

Summary: https://cmssdt.cern.ch/SDT/jenkins-artifacts/pull-request-integration/PR-decd46/17164/summary.html
COMMIT: 10fbac2
CMSSW: CMSSW_12_0_X_2021-07-23-2300/slc7_amd64_gcc900
User test area: For local testing, you can use /cvmfs/cms-ci.cern.ch/week0/cms-sw/cmssw/34377/17164/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: 39
DQMHistoTests: Total histograms compared: 2998564
DQMHistoTests: Total failures: 1
DQMHistoTests: Total nulls: 0
DQMHistoTests: Total successes: 2998541
DQMHistoTests: Total skipped: 22
DQMHistoTests: Total Missing objects: 0
DQMHistoSizes: Histogram memory added: 0.0 KiB( 38 files compared)
Checked 165 log files, 37 edm output root files, 39 DQM output files
TriggerResults: no differences found

ggovi · 2021-07-27T07:10:24Z

+1

cmsbuild · 2021-07-27T07:10:49Z

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. @silviodonato, @dpiparo, @qliphy, @perrotta (and backports should be raised in the release meeting by the corresponding L2)

perrotta · 2021-07-27T13:06:19Z

@qliphy @jan-kaspar can you confirm that the errors that were mentioned earlier in this thread disappeared after the last commit?
If so, this PR can be merged: isn't it?

qliphy · 2021-07-27T13:26:32Z

@perrotta I tested write-ctpps-rprealalignment_table_test.py and serveral scripts, and they work well now.

perrotta · 2021-07-27T13:29:37Z

+1

Dr15Jones · 2021-07-27T20:13:53Z

CondTools/CTPPS/plugins/CTPPSPixGainCalibsESAnalyzer.cc

-      : m_outfilename(p.getUntrackedParameter<std::string>("outputrootfile", "output.root")) {
-    //    std::cout<<"CTPPSPixGainCalibsESAnalyzer"<<std::endl;
+      : m_outfilename(p.getUntrackedParameter<std::string>("outputrootfile", "output.root")),
+        tokenCalibration_(esConsumes<CTPPSPixelGainCalibrations, CTPPSPixelGainCalibrationsRcd>()) {


For future reference, this could have been tokenCalibration_(esConsumes()). The code is 'smart enough' to determine what the type is based on the type of the token.

Dr15Jones · 2021-07-27T20:15:34Z

CondTools/CTPPS/plugins/CTPPSPixGainCalibsESAnalyzer.cc

+  edm::ESHandle<CTPPSPixelGainCalibrations> calhandle = context.getHandle(tokenCalibration_);
  edm::LogPrint("CTPPSPixGainCalibsReader") << "got eshandle";
-  context.get<CTPPSPixelGainCalibrationsRcd>().get(calhandle);
  edm::LogPrint("CTPPSPixGainCalibsReader") << "got context";
  const CTPPSPixelGainCalibrations* pPixelGainCalibrations = calhandle.product();


For future reference, this could simply have been

const CTPPSPixelGainCalibrations* pPixelGainCalibrations = &context.getData(tokenCalibration_)

jan-kaspar · 2021-07-28T08:01:27Z

Thanks @Dr15Jones !

jan-kaspar added 2 commits June 1, 2021 16:44

systematic use of esConsumes

42b3e09

Patch recommended by Wagner.

0d5cfc1

cmsbuild added this to the CMSSW_12_0_X milestone Jul 7, 2021

cmsbuild added code-checks-pending db-pending orp-pending pending-signatures tests-pending labels Jul 7, 2021

cmsbuild added code-checks-rejected and removed code-checks-pending labels Jul 7, 2021

apply formatting

9d07a3a

cmsbuild added code-checks-pending and removed code-checks-rejected labels Jul 7, 2021

cmsbuild added code-checks-approved and removed code-checks-pending labels Jul 7, 2021

cmsbuild added tests-started and removed tests-pending labels Jul 13, 2021

cmsbuild added tests-approved and removed tests-started labels Jul 13, 2021

cmsbuild added db-approved fully-signed and removed db-pending pending-signatures labels Jul 15, 2021

cmsbuild added code-checks-approved and removed code-checks-pending labels Jul 22, 2021

cmsbuild added tests-started and removed tests-pending labels Jul 24, 2021

cmsbuild added tests-approved and removed tests-started labels Jul 24, 2021

cmsbuild added db-approved fully-signed and removed db-pending pending-signatures labels Jul 27, 2021

cmsbuild added orp-approved and removed orp-pending labels Jul 27, 2021

cmsbuild merged commit 45994cb into cms-sw:master Jul 27, 2021

Dr15Jones reviewed Jul 27, 2021

View reviewed changes

This was referenced Jul 27, 2021

Remove IgProfModule as obsolete #34653

Merged

Enable printout in ReusableObjectHolder test #34651

Merged

Fix crash on edit of Jet collection after view destruction #34655

Merged

jan-kaspar deleted the esConsumes_CondTools_CTPPS branch August 31, 2021 08:24

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

esConsumes in CondTools/CTPPS #34377

esConsumes in CondTools/CTPPS #34377

jan-kaspar commented Jul 7, 2021

cmsbuild commented Jul 7, 2021

cmsbuild commented Jul 7, 2021

cmsbuild commented Jul 7, 2021

perrotta commented Jul 13, 2021

cmsbuild commented Jul 13, 2021

ggovi commented Jul 15, 2021

cmsbuild commented Jul 15, 2021

jan-kaspar commented Jul 22, 2021

cmsbuild commented Jul 22, 2021

cmsbuild commented Jul 22, 2021

qliphy commented Jul 24, 2021

cmsbuild commented Jul 24, 2021

ggovi commented Jul 27, 2021

cmsbuild commented Jul 27, 2021

perrotta commented Jul 27, 2021

qliphy commented Jul 27, 2021

perrotta commented Jul 27, 2021

Dr15Jones Jul 27, 2021

Dr15Jones Jul 27, 2021

jan-kaspar commented Jul 28, 2021

esConsumes in CondTools/CTPPS #34377

esConsumes in CondTools/CTPPS #34377

Conversation

jan-kaspar commented Jul 7, 2021

PR description:

PR validation:

cmsbuild commented Jul 7, 2021

cmsbuild commented Jul 7, 2021

cmsbuild commented Jul 7, 2021

perrotta commented Jul 13, 2021

cmsbuild commented Jul 13, 2021

Comparison Summary

ggovi commented Jul 15, 2021

cmsbuild commented Jul 15, 2021

jan-kaspar commented Jul 22, 2021

cmsbuild commented Jul 22, 2021

cmsbuild commented Jul 22, 2021

qliphy commented Jul 24, 2021

cmsbuild commented Jul 24, 2021

Comparison Summary

ggovi commented Jul 27, 2021

cmsbuild commented Jul 27, 2021

perrotta commented Jul 27, 2021

qliphy commented Jul 27, 2021

perrotta commented Jul 27, 2021

Dr15Jones Jul 27, 2021

Choose a reason for hiding this comment

Dr15Jones Jul 27, 2021

Choose a reason for hiding this comment

jan-kaspar commented Jul 28, 2021