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

[PPS] Thread-safe ES condition retrieval #32606

Merged
merged 9 commits into from Jan 7, 2021

Conversation

forthommel
Copy link
Contributor

@forthommel forthommel commented Jan 5, 2021

PR description:

This PR replaces all edm::EventSetup::get<TRcd>().get(..., edm::ESHandle<T>) calls to the thread-safe equivalent T edm::EventSetup::getData(edm::ESGetToken<T, TRcd>) member in the PPS environment (both in local reconstruction and validation codes).
It also drops the CTPPSPixelGainCalibrationDBService helper object made trivial by this new retrieval method.

PR validation:

Code compiles, runs, matrix tests all passing. Detailed validation (run 2 reco + direct simulation) performed by @jan-kaspar and successfully passing.

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

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

@cmsbuild
Copy link
Contributor

cmsbuild commented Jan 5, 2021

+code-checks

Logs: https://cmssdt.cern.ch/SDT/code-checks/cms-sw-PR-32606/20651

  • This PR adds an extra 52KB to repository

@cmsbuild
Copy link
Contributor

cmsbuild commented Jan 5, 2021

A new Pull Request was created by @forthommel (Laurent Forthomme) for master.

It involves the following packages:

RecoPPS/Local
SimPPS/PPSPixelDigiProducer
Validation/CTPPS

@perrotta, @andrius-k, @kmaeshima, @civanch, @ErnestaP, @mdhildreth, @cmsbuild, @jfernan2, @fioriNTU, @slava77, @jpata can you please review it and eventually sign? Thanks.
@jan-kaspar 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
Copy link
Contributor

perrotta commented Jan 6, 2021

please test

@cmsbuild
Copy link
Contributor

cmsbuild commented Jan 6, 2021

+1

Summary: https://cmssdt.cern.ch/SDT/jenkins-artifacts/pull-request-integration/PR-c2cddd/11985/summary.html
COMMIT: dfa2947
CMSSW: CMSSW_11_3_X_2021-01-05-2300/slc7_amd64_gcc900

Comparison Summary

Summary:

  • No significant changes to the logs found
  • Reco comparison results: 4 differences found in the comparisons
  • DQMHistoTests: Total files compared: 37
  • DQMHistoTests: Total histograms compared: 2716967
  • DQMHistoTests: Total failures: 6
  • DQMHistoTests: Total nulls: 1
  • DQMHistoTests: Total successes: 2716938
  • DQMHistoTests: Total skipped: 22
  • DQMHistoTests: Total Missing objects: 0
  • DQMHistoSizes: Histogram memory added: -0.004 KiB( 36 files compared)
  • DQMHistoSizes: changed ( 312.0 ): -0.004 KiB MessageLogger/Warnings
  • Checked 156 log files, 37 edm output root files, 37 DQM output files

@forthommel
Copy link
Contributor Author

It seems the only differences are found to be in the LogErrorHarvester and exceed the scope of this PR.

@jfernan2
Copy link
Contributor

jfernan2 commented Jan 6, 2021

+1

@slava77
Copy link
Contributor

slava77 commented Jan 7, 2021

+1

for #32606 dfa2947

  • code changes are in line with the PR description, essentially technical
  • jenkins tests pass and comparisons with the baseline show no relevant differences

@civanch
Copy link
Contributor

civanch commented Jan 7, 2021

+1

@cmsbuild
Copy link
Contributor

cmsbuild commented Jan 7, 2021

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

@silviodonato
Copy link
Contributor

+1

@cmsbuild cmsbuild merged commit 83dcb93 into cms-sw:master Jan 7, 2021
@forthommel forthommel deleted the pps-validation_estoken-11_3_X branch January 7, 2021 20:54
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

7 participants