Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

ECAL PoolDBOutput code migration #36062

Merged
merged 15 commits into from Nov 12, 2021

Conversation

thomreis
Copy link
Contributor

@thomreis thomreis commented Nov 9, 2021

PR description:

This PR migrates ECAL related code to new PoolDBOutput functions writeOneIOV, createOneIOV, and appendOneIOV as described in https://indico.cern.ch/event/1088598/contributions/4580152/attachments/2333275/3976721/DBOutputService%20changes.pdf .
Some code cleaning is done in addition.

PR validation:

The code compiles and test configurations (if existing) were run without an issue.

@cmsbuild
Copy link
Contributor

cmsbuild commented Nov 9, 2021

+code-checks

Logs: https://cmssdt.cern.ch/SDT/code-checks/cms-sw-PR-36062/26526

  • This PR adds an extra 84KB to repository

@cmsbuild
Copy link
Contributor

cmsbuild commented Nov 9, 2021

A new Pull Request was created by @thomreis (Thomas Reis) for master.

It involves the following packages:

  • CalibCalorimetry/EcalTPGTools (l1, alca)
  • CondFormats/EcalObjects (db, alca)
  • CondTools/Ecal (db)
  • Geometry/CaloEventSetup (geometry)
  • Geometry/EcalAlgo (geometry)

@malbouis, @civanch, @Dr15Jones, @makortel, @cvuosalo, @ianna, @mdhildreth, @cmsbuild, @rekovic, @yuanchao, @ggovi, @tvami, @cecilecaillol, @francescobrivio can you please review it and eventually sign? Thanks.
@rchatter, @tocheng, @argiro, @bsunanda, @thomreis, @simonepigazzini, @mmusich, @fabiocos, @seemasharmafnal 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

Comment on lines 151 to 152
const auto handle = evtSetup.getHandle(pedestalsToken_);
const EcalTPGPedestals* obj = handle.product();
Copy link
Contributor

@tvami tvami Nov 9, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
const auto handle = evtSetup.getHandle(pedestalsToken_);
const EcalTPGPedestals* obj = handle.product();
const auto obj = &evtSetup.getData(pedestalsToken_);

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I agree that would be even better.
But it could be const auto& obj = evtSetup.getData(pedestalsToken_); in fact.

@tvami
Copy link
Contributor

tvami commented Nov 9, 2021

Hi @thomreis I think this should go in the other direction, make the obj an instance and pass the reference instead of the pointer. I made a simple suggestion for one of the cases above.

@cmsbuild
Copy link
Contributor

+code-checks

Logs: https://cmssdt.cern.ch/SDT/code-checks/cms-sw-PR-36062/26539

  • This PR adds an extra 216KB to repository

@cmsbuild
Copy link
Contributor

Pull request #36062 was updated. @malbouis, @civanch, @Dr15Jones, @makortel, @cvuosalo, @ianna, @mdhildreth, @cmsbuild, @rekovic, @yuanchao, @ggovi, @tvami, @cecilecaillol, @francescobrivio can you please check and sign again.

@ggovi
Copy link
Contributor

ggovi commented Nov 10, 2021

please test

@cmsbuild
Copy link
Contributor

+1

Summary: https://cmssdt.cern.ch/SDT/jenkins-artifacts/pull-request-integration/PR-9043b3/20428/summary.html
COMMIT: 07c1b12
CMSSW: CMSSW_12_2_X_2021-11-09-2300/slc7_amd64_gcc900
User test area: For local testing, you can use /cvmfs/cms-ci.cern.ch/week0/cms-sw/cmssw/36062/20428/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: 4 differences found in the comparisons
  • DQMHistoTests: Total files compared: 42
  • DQMHistoTests: Total histograms compared: 2901890
  • DQMHistoTests: Total failures: 5
  • DQMHistoTests: Total nulls: 1
  • DQMHistoTests: Total successes: 2901862
  • DQMHistoTests: Total skipped: 22
  • DQMHistoTests: Total Missing objects: 0
  • DQMHistoSizes: Histogram memory added: -0.004 KiB( 41 files compared)
  • DQMHistoSizes: changed ( 312.0 ): -0.004 KiB MessageLogger/Warnings
  • Checked 177 log files, 37 edm output root files, 42 DQM output files
  • TriggerResults: no differences found

@tvami
Copy link
Contributor

tvami commented Nov 10, 2021

+alca

  • tests run fine, code changes as expected

@cvuosalo
Copy link
Contributor

+1

@ggovi
Copy link
Contributor

ggovi commented Nov 12, 2021

+db

@tvami
Copy link
Contributor

tvami commented Nov 12, 2021

Hi @perrotta @qliphy please consider this fully signed... The L1T signature touches on a single file, I'm sure Vladimir would agree...

@qliphy
Copy link
Contributor

qliphy commented Nov 12, 2021

+1

@qliphy
Copy link
Contributor

qliphy commented Nov 12, 2021

merge

@cmsbuild cmsbuild merged commit 6e91d22 into cms-sw:master Nov 12, 2021
@thomreis thomreis deleted the ecal_pooldboutput_migration branch November 15, 2021 09:31
@thomreis
Copy link
Contributor Author

thomreis commented Nov 16, 2021

This addresses parts of cms-AlCaDB/AlCaTools#41 as well.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

6 participants