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.

Sign up for GitHub

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

Jump to bottom

PPS Alignment - bug fix (backport) #34845

Merged
merged 5 commits into from
Aug 16, 2021
Merged

PPS Alignment - bug fix (backport) #34845

merged 5 commits into from
Aug 16, 2021

Conversation

MatiXOfficial
Copy link
Contributor

This PR is a backport. Original PR: #34844. Backport needed to 12_0.

PR description:

Fixed a bug from #31728. Now, the PPSAlignmentConfig plugin is registered, and some necessary declarations were added.

PR validation:

Run the test from CalibPPS/AlignmentGlobal/tests and received expected results.

@cmsbuild cmsbuild added this to the CMSSW_12_0_X milestone Aug 11, 2021
@cmsbuild
Copy link
Contributor

cmsbuild commented Aug 11, 2021 •
edited
Loading

A new Pull Request was created by @MatiXOfficial (Mateusz Kocot) for CMSSW_12_0_X.

It involves the following packages:

  • CondCore/CTPPSPlugins (db)
  • CondFormats/PPSObjects (alca)

@malbouis, @yuanchao, @cmsbuild, @tlampen, @ggovi, @pohsun, @francescobrivio, @tvami can you please review it and eventually sign? Thanks.
@tocheng, @fabferro, @jan-kaspar, @mmusich, @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

@cmsbuild
Copy link
Contributor

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

@francescobrivio
Copy link
Contributor

type bugfix

@francescobrivio
Copy link
Contributor

@MatiXOfficial see same comments as #34844 (review)

@cmsbuild
Copy link
Contributor

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

@francescobrivio
Copy link
Contributor

@cmsbuild please test

@francescobrivio
Copy link
Contributor

@cmsbuild please abort

@ggovi
Copy link
Contributor

ggovi commented Aug 12, 2021

@MatiXOfficial you are still missing a unit test for the new CondFormat ( PPSAlignmentConfig )
https://github.com/cms-sw/cmssw/tree/master/CondFormats/PPSObjects/test

@ggovi
Copy link
Contributor

ggovi commented Aug 12, 2021

Also, we recommend to execute some basic private test using a local db (sqlite ) before the PR, to validate the payload write/read. The missing COND_SERIALIZABLE declaration would have been spotted before the earlier PR

@francescobrivio francescobrivio mentioned this pull request Aug 12, 2021
Merged
@MatiXOfficial
Copy link
Contributor Author

@ggovi Thanks! I will add the missing files shortly. Just for your information, in the first PR, we weren't thinking much about the DB. Now, I am integrating the alignment code with the DB and the PCL. I have already implemented the changes you mentioned, but only locally, since we wait with PR until we can run the matrix test (which requires PPSAlignmentConfig to be registered as a plugin, etc.).

@cmsbuild
Copy link
Contributor

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

@MatiXOfficial
Copy link
Contributor Author

I added the serialization test and also two simple modules that write an sqlite file and retrieve the data. I was able to retrieve the same object that had been written.

@francescobrivio
Copy link
Contributor

@cmsbuild please test

@cmsbuild
Copy link
Contributor

-1

Failed Tests: RelVals-INPUT
Summary: https://cmssdt.cern.ch/SDT/jenkins-artifacts/pull-request-integration/PR-ca87de/17740/summary.html
COMMIT: f2d90ce
CMSSW: CMSSW_12_0_X_2021-08-11-2300/slc7_amd64_gcc900
User test area: For local testing, you can use /cvmfs/cms-ci.cern.ch/week1/cms-sw/cmssw/34845/17740/install.sh to create a dev area with all the needed externals and cmssw changes.

RelVals-INPUT

  • 11605.011605.0_SingleGammaPt35+2021+SingleGammaPt35_pythia8_GenSimINPUT+Digi+Reco+HARVEST+ALCA/step2_SingleGammaPt35+2021+SingleGammaPt35_pythia8_GenSimINPUT+Digi+Reco+HARVEST+ALCA.log
  • 11634.011634.0_TTbar_14TeV+2021+TTbar_14TeV_TuneCP5_GenSimINPUT+Digi+Reco+HARVEST+ALCA/step2_TTbar_14TeV+2021+TTbar_14TeV_TuneCP5_GenSimINPUT+Digi+Reco+HARVEST+ALCA.log
  • 11601.011601.0_SingleElectronPt10+2021+SingleElectronPt10_pythia8_GenSimINPUT+Digi+Reco+HARVEST+ALCA/step2_SingleElectronPt10+2021+SingleElectronPt10_pythia8_GenSimINPUT+Digi+Reco+HARVEST+ALCA.log
Expand to see more relval errors ...

Comparison Summary

Summary:

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

@francescobrivio
Copy link
Contributor

@cmsbuild please test

Lately we see many of these failures, let's try again

@cmsbuild
Copy link
Contributor

+1

Summary: https://cmssdt.cern.ch/SDT/jenkins-artifacts/pull-request-integration/PR-ca87de/17754/summary.html
COMMIT: f2d90ce
CMSSW: CMSSW_12_0_X_2021-08-12-2300/slc7_amd64_gcc900
User test area: For local testing, you can use /cvmfs/cms-ci.cern.ch/week1/cms-sw/cmssw/34845/17754/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: 6 differences found in the comparisons
  • DQMHistoTests: Total files compared: 39
  • DQMHistoTests: Total histograms compared: 2998564
  • DQMHistoTests: Total failures: 6
  • DQMHistoTests: Total nulls: 1
  • DQMHistoTests: Total successes: 2998535
  • DQMHistoTests: Total skipped: 22
  • DQMHistoTests: Total Missing objects: 0
  • DQMHistoSizes: Histogram memory added: -0.004 KiB( 38 files compared)
  • DQMHistoSizes: changed ( 312.0 ): -0.004 KiB MessageLogger/Warnings
  • Checked 165 log files, 37 edm output root files, 39 DQM output files
  • TriggerResults: no differences found

@francescobrivio
Copy link
Contributor

+alca

  • only two minor differences are in particleNet and deepTau in wf 1325.81 (TTbar13nano), but I don't see how they can be related to this PR. Also, they don't appear in the PR to master.

@tvami
Copy link
Contributor

tvami commented Aug 14, 2021

backport of #34844

  • just for the github labels

tvami
tvami reviewed Aug 14, 2021
View reviewed changes
CondFormats/PPSObjects/src/classes_def.xml
<class name="PPSAlignmentConfig" class_version="0"/>

</lcgdict>
Copy link
Contributor

Choose a reason for hiding this comment

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

There is a newline missing here, since the PR is not merged yet, would you @MatiXOfficial like to fix that?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Sure, I can fix it, but is it a good idea? The main PR (#34844) has already been merged into master.

Copy link
Contributor

Choose a reason for hiding this comment

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

@perrotta @qliphy what do you think?

Copy link
Contributor

Choose a reason for hiding this comment

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

@perrotta @qliphy what do you think?

While not being pleasant to see while reading the code on github, I don't see any issue because of this missing newline at the end of the file: it was as such even before this fix, and it was already merged as such in the master. Also with the intention of having as similar as possible code in the master and in the 12_0 backport, I would avoid implementing this possible aesthetical fix here.

@ggovi
Copy link
Contributor

ggovi commented Aug 16, 2021

+1

@cmsbuild
Copy link
Contributor

This pull request is fully signed and it will be integrated in one of the next CMSSW_12_0_X IBs (tests are also fine) and once validation in the development release cycle CMSSW_12_1_X is complete. 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
Copy link
Contributor

+1

  • A bug fix, and a few more related tests were added: liable to be backported in the buf fig only release

@cmsbuild cmsbuild merged commit 5c0525f into cms-sw:CMSSW_12_0_X Aug 16, 2021
@MatiXOfficial MatiXOfficial deleted the pps-alignment-global-fix branch August 17, 2021 17:57
@MatiXOfficial MatiXOfficial mentioned this pull request Oct 12, 2021
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@tvami tvami tvami left review comments

@perrotta perrotta perrotta left review comments

Assignees
No one assigned
Labels
alca-approved backport-ok bug-fix db-approved fully-signed orp-approved tests-approved
Projects
None yet
Milestone
CMSSW_12_0_X
Development

Successfully merging this pull request may close these issues.
6 participants
@MatiXOfficial @cmsbuild @francescobrivio @ggovi @tvami @perrotta