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

Top recoil hook cleaned, backport to CMSSW_13_2_X #42332

Merged
merged 9 commits into from
Oct 27, 2023

Conversation

vslokenb
Copy link
Contributor

@vslokenb vslokenb commented Jul 21, 2023

PR description:

Backport of recoilToTop. Pushed from 13_2_0_pre2.

PR validation:

Same as linked original PR below. Verified using scram build code-check and scram build code-format. Seem to be a couple of minor indentation changes in lines unrelated to the actual TopRecoilHook in Pythia8Hadronizer.cc as a result of the code-format tool. All differences between original PR are to account for CMSSW_13_2_X formatting needs.

If this PR is a backport please specify the original PR and why you need to backport that PR. If this PR will be backported please specify to which release cycle the backport is meant for:

Original PR found here. Backport requested by maintainers.

@cmsbuild
Copy link
Contributor

cmsbuild commented Jul 21, 2023

A new Pull Request was created by @vslokenb for CMSSW_13_2_X.

It involves the following packages:

  • GeneratorInterface/Pythia8Interface (generators)

@SiewYan, @mkirsano, @Saptaparna, @cmsbuild, @alberto-sanchez, @menglu21, @GurpreetSinghChahal can you please review it and eventually sign? Thanks.
@alberto-sanchez, @mkirsano this is something you requested to watch as well.
@perrotta, @dpiparo, @rappoccio you are the release manager for this.

cms-bot commands are listed here

Fix suggested by PR 42313. Removes useless check.
@cmsbuild
Copy link
Contributor

Pull request #42332 was updated. @SiewYan, @mkirsano, @Saptaparna, @cmsbuild, @alberto-sanchez, @menglu21, @GurpreetSinghChahal can you please check and sign again.

@perrotta
Copy link
Contributor

backport of #42180

@perrotta perrotta changed the title Backport to 13_2_X Top recoil hook cleaned, backport to CMSSW_13_2_X Jul 29, 2023
@menglu21
Copy link
Contributor

menglu21 commented Aug 1, 2023

please test

@cmsbuild
Copy link
Contributor

cmsbuild commented Aug 1, 2023

+1

Summary: https://cmssdt.cern.ch/SDT/jenkins-artifacts/pull-request-integration/PR-dfc0e9/34017/summary.html
COMMIT: f4e5c6c
CMSSW: CMSSW_13_2_X_2023-08-01-1100/el8_amd64_gcc11
User test area: For local testing, you can use /cvmfs/cms-ci.cern.ch/week0/cms-sw/cmssw/42332/34017/install.sh to create a dev area with all the needed externals and cmssw changes.

Comparison Summary

Summary:

  • You potentially added 4 lines to the logs
  • Reco comparison results: 13 differences found in the comparisons
  • DQMHistoTests: Total files compared: 48
  • DQMHistoTests: Total histograms compared: 3195634
  • DQMHistoTests: Total failures: 9
  • DQMHistoTests: Total nulls: 0
  • DQMHistoTests: Total successes: 3195603
  • DQMHistoTests: Total skipped: 22
  • DQMHistoTests: Total Missing objects: 0
  • DQMHistoSizes: Histogram memory added: 0.0 KiB( 47 files compared)
  • Checked 207 log files, 159 edm output root files, 48 DQM output files
  • TriggerResults: no differences found

Copy link
Contributor

@perrotta perrotta left a comment

Choose a reason for hiding this comment

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

This being based on the rather old CMSSW_3_2_0_pre2, it was a bit complicate to compare with the same PR merged in master.
At the end I managed to do it.: therefore, even if a rebase on the more recent 13_2_X IB could help, it is not mandatory, provided you apply the two small adjustments listed here needed to synchronize with the master version of the same PR.
After that (and the @cms-sw/generators-l2 signature, of course) this PR can be merged

vslokenb and others added 2 commits August 4, 2023 13:40
Co-authored-by: Andrea Perrotta <perrotta@cern.ch>
Co-authored-by: Andrea Perrotta <perrotta@cern.ch>
@cmsbuild
Copy link
Contributor

cmsbuild commented Aug 4, 2023

Pull request #42332 was updated. @SiewYan, @mkirsano, @Saptaparna, @cmsbuild, @alberto-sanchez, @menglu21, @GurpreetSinghChahal can you please check and sign again.

@menglu21
Copy link
Contributor

please test

@perrotta
Copy link
Contributor

hold
(see #42332 (review))

@cmsbuild
Copy link
Contributor

-1

Failed Tests: RelVals-INPUT
Summary: https://cmssdt.cern.ch/SDT/jenkins-artifacts/pull-request-integration/PR-dfc0e9/34956/summary.html
COMMIT: cce6879
CMSSW: CMSSW_13_2_X_2023-09-28-1100/el8_amd64_gcc11
User test area: For local testing, you can use /cvmfs/cms-ci.cern.ch/week0/cms-sw/cmssw/42332/34956/install.sh to create a dev area with all the needed externals and cmssw changes.

Found compilation warnings

RelVals-INPUT

The relvals timed out after 4 hours.

Comparison Summary

Summary:

  • No significant changes to the logs found
  • Reco comparison results: 5 differences found in the comparisons
  • DQMHistoTests: Total files compared: 48
  • DQMHistoTests: Total histograms compared: 3198243
  • DQMHistoTests: Total failures: 4
  • DQMHistoTests: Total nulls: 0
  • DQMHistoTests: Total successes: 3198217
  • DQMHistoTests: Total skipped: 22
  • DQMHistoTests: Total Missing objects: 0
  • DQMHistoSizes: Histogram memory added: 0.0 KiB( 47 files compared)
  • Checked 207 log files, 159 edm output root files, 48 DQM output files
  • TriggerResults: no differences found

@menglu21
Copy link
Contributor

@perrotta is the error related to this PR

@perrotta
Copy link
Contributor

perrotta commented Oct 10, 2023

@perrotta is the error related to this PR

Well, the relvals timed out is not a fault of this PR, but the compilation warnings are.
Please apply the same fix which was applied in master

@menglu21
Copy link
Contributor

ok, it seems I overlooked the fix, sorry for that. @vslokenb please follow @perrotta comments

Co-authored-by: Andrea Perrotta <perrotta@cern.ch>
@cmsbuild
Copy link
Contributor

Pull request #42332 was updated. @GurpreetSinghChahal, @menglu21, @alberto-sanchez, @bbilin, @SiewYan, @cmsbuild, @mkirsano can you please check and sign again.

@menglu21
Copy link
Contributor

@cmsbuild please test

@cmsbuild
Copy link
Contributor

+1

Summary: https://cmssdt.cern.ch/SDT/jenkins-artifacts/pull-request-integration/PR-dfc0e9/35219/summary.html
COMMIT: a4bc408
CMSSW: CMSSW_13_2_X_2023-10-17-1100/el8_amd64_gcc11
User test area: For local testing, you can use /cvmfs/cms-ci.cern.ch/week1/cms-sw/cmssw/42332/35219/install.sh to create a dev area with all the needed externals and cmssw changes.

Comparison Summary

Summary:

  • You potentially added 2 lines to the logs
  • Reco comparison results: 12 differences found in the comparisons
  • DQMHistoTests: Total files compared: 48
  • DQMHistoTests: Total histograms compared: 3198785
  • DQMHistoTests: Total failures: 6
  • DQMHistoTests: Total nulls: 0
  • DQMHistoTests: Total successes: 3198757
  • DQMHistoTests: Total skipped: 22
  • DQMHistoTests: Total Missing objects: 0
  • DQMHistoSizes: Histogram memory added: 0.0 KiB( 47 files compared)
  • Checked 207 log files, 159 edm output root files, 48 DQM output files
  • TriggerResults: no differences found

@menglu21
Copy link
Contributor

+1

@cmsbuild
Copy link
Contributor

This pull request is fully signed and it will be integrated in one of the next CMSSW_13_2_X IBs (tests are also fine) and once validation in the development release cycle CMSSW_13_3_X is complete. This pull request will now be reviewed by the release team before it's merged. @rappoccio, @sextonkennedy, @antoniovilela (and backports should be raised in the release meeting by the corresponding L2)

@rappoccio
Copy link
Contributor

@perrotta are you okay with the PR now? Have your comments been addressed?

@perrotta
Copy link
Contributor

@perrotta are you okay with the PR now? Have your comments been addressed?

Ciao Sal. Yes, now this PR correctly implements the fix that was merged in master with #42313

@antoniovilela
Copy link
Contributor

+1

@cmsbuild cmsbuild merged commit b81e432 into cms-sw:CMSSW_13_2_X Oct 27, 2023
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.

6 participants