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

Fix for wf 11634.15 #40539

Merged
merged 4 commits into from Jan 18, 2023
Merged

Fix for wf 11634.15 #40539

merged 4 commits into from Jan 18, 2023

Conversation

francescobrivio
Copy link
Contributor

@francescobrivio francescobrivio commented Jan 16, 2023

PR description:

This is yet another solution to try and fix wf 11634.15 and clean the IBs:

As Marino mentioned in his PR (#40531) I also could not find a "pretty way" to handle toModify, so I had to rely on copy and pop. Any suggestion to improve this is welcome!

EDIT:
In the end this PR includes the changes from both #40485 and #40531:
the btagCSVV2 discriminator is removed from the standard jet b-tagging nano sequences (since it's not supported anymore for Run 3) and it is added back for the run2 nano sequences.

FYI @cms-sw/btv-pog-l2 @AnnikaStein @missirol @cms-sw/alca-l2 @perrotta

PR validation:

Successfully tested with:
runTheMatrix.py -l 312.0,11634.15,11634.0,12434.0 -j8--ibeos

Backport :

Not a backport - no backport needed.

@francescobrivio
Copy link
Contributor Author

@cmsbuild , please test workflow 11634.15

@cmsbuild
Copy link
Contributor

+code-checks

Logs: https://cmssdt.cern.ch/SDT/code-checks/cms-sw-PR-40539/33767

@cmsbuild
Copy link
Contributor

A new Pull Request was created by @francescobrivio for master.

It involves the following packages:

  • PhysicsTools/NanoAOD (xpog)
  • RecoBTag/SoftLepton (reconstruction)

@mandrenguyen, @clacaputo, @swertz, @vlimant can you please review it and eventually sign? Thanks.
@AlexDeMoor, @emilbols, @JyothsnaKomaragiri, @AnnikaStein, @missirol, @gpetruc, @andrzejnovak, @demuller 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

@tvami
Copy link
Contributor

tvami commented Jan 16, 2023

@francescobrivio for the xpog signature we should run nano

@tvami
Copy link
Contributor

tvami commented Jan 16, 2023

@cmsbuild , please abort

@tvami
Copy link
Contributor

tvami commented Jan 16, 2023

enable nano

@tvami
Copy link
Contributor

tvami commented Jan 16, 2023

@cmsbuild , please test workflow 11634.15

@cmsbuild
Copy link
Contributor

+1

Summary: https://cmssdt.cern.ch/SDT/jenkins-artifacts/pull-request-integration/PR-3025f0/30019/summary.html
COMMIT: 5203caf
CMSSW: CMSSW_13_0_X_2023-01-16-1100/el8_amd64_gcc11
Additional Tests: NANO
User test area: For local testing, you can use /cvmfs/cms-ci.cern.ch/week0/cms-sw/cmssw/40539/30019/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
  • ROOTFileChecks: Some differences in event products or their sizes found
  • Reco comparison results: 11 differences found in the comparisons
  • DQMHistoTests: Total files compared: 49
  • DQMHistoTests: Total histograms compared: 3555538
  • DQMHistoTests: Total failures: 65
  • DQMHistoTests: Total nulls: 0
  • DQMHistoTests: Total successes: 3555451
  • DQMHistoTests: Total skipped: 22
  • DQMHistoTests: Total Missing objects: 0
  • DQMHistoSizes: Histogram memory added: 0.0 KiB( 48 files compared)
  • Checked 214 log files, 167 edm output root files, 49 DQM output files
  • TriggerResults: no differences found

NANO Comparison Summary

Summary:

  • No significant changes to the logs found
  • ROOTFileChecks: Some differences in event products or their sizes found
  • Reco comparison results: 12 differences found in the comparisons
  • DQMHistoTests: Total files compared: 11
  • DQMHistoTests: Total histograms compared: 10839
  • DQMHistoTests: Total failures: 26
  • DQMHistoTests: Total nulls: 0
  • DQMHistoTests: Total successes: 10813
  • DQMHistoTests: Total skipped: 0
  • DQMHistoTests: Total Missing objects: 0
  • DQMHistoSizes: Histogram memory added: 0.0 KiB( 10 files compared)
  • Checked 23 log files, 10 edm output root files, 11 DQM output files

Nano size comparison Summary:

Sample kb/ev ref kb/ev diff kb/ev ev/s/thd ref ev/s/thd diff rate mem/thd ref mem/thd
2500.31 2.240 2.240 0.000 ( +0.0% ) 9.55 9.55 -0.0% 1.467 1.464
2500.311 2.330 2.330 0.000 ( +0.0% ) 9.34 9.14 +2.2% 1.823 1.829
2500.312 2.284 2.284 0.000 ( +0.0% ) 9.52 9.25 +3.0% 1.832 1.821
2500.33 1.101 1.101 0.000 ( +0.0% ) 22.18 21.91 +1.3% 1.641 1.649
2500.331 1.396 1.396 0.000 ( +0.0% ) 16.00 15.95 +0.3% 1.793 1.794
2500.332 1.328 1.328 0.000 ( +0.0% ) 18.05 17.88 +0.9% 1.849 1.850
2500.401 2.145 2.161 -0.016 ( -0.7% ) 10.50 10.35 +1.5% 1.141 1.153
2500.501 1.716 1.726 -0.011 ( -0.6% ) 16.58 16.40 +1.1% 1.064 1.072
2500.511 1.125 1.134 -0.008 ( -0.7% ) 31.00 30.17 +2.8% 1.321 1.317
2500.601 2.056 2.073 -0.017 ( -0.8% ) 12.47 12.48 -0.1% 1.130 1.128

Copy link
Contributor

@swertz swertz left a comment

Choose a reason for hiding this comment

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

Thanks for the fix, could you please also make sure the list of variables are changed consistently in

PhysicsTools/NanoAOD/python/custom_jme_cff.py Outdated Show resolved Hide resolved
PhysicsTools/NanoAOD/python/jetsAK4_CHS_cff.py Outdated Show resolved Hide resolved
@francescobrivio
Copy link
Contributor Author

Thanks @missirol and @swertz for the review!
So IIUC we should have:

  • Annika's changes for run3/run2 modifiers
  • Marino's fix
  • nanoAODDQM cleanup.

@AnnikaStein let me know if you prefer to include everything in your PR or if I should implement everything here!

@AnnikaStein
Copy link
Contributor

@francescobrivio
Thanks a lot for following up on this issue, given that the different fixes are now known to resolve the problematic workflow I'd say you can go ahead with including this all here and we close my PR #40485

@perrotta
Copy link
Contributor

type bug-fix

@perrotta
Copy link
Contributor

urgent

@francescobrivio
Copy link
Contributor Author

test parameters:

  • workflow = 11634.15,2500.31
  • enable = nano

@francescobrivio
Copy link
Contributor Author

@cmsbuild please test

@cmsbuild
Copy link
Contributor

+1

Summary: https://cmssdt.cern.ch/SDT/jenkins-artifacts/pull-request-integration/PR-3025f0/30045/summary.html
COMMIT: 3a70499
CMSSW: CMSSW_13_0_X_2023-01-17-1100/el8_amd64_gcc11
Additional Tests: NANO
User test area: For local testing, you can use /cvmfs/cms-ci.cern.ch/week0/cms-sw/cmssw/40539/30045/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
  • ROOTFileChecks: Some differences in event products or their sizes found
  • Reco comparison results: 63 differences found in the comparisons
  • DQMHistoTests: Total files compared: 50
  • DQMHistoTests: Total histograms compared: 3556670
  • DQMHistoTests: Total failures: 0
  • DQMHistoTests: Total nulls: 0
  • DQMHistoTests: Total successes: 3556648
  • DQMHistoTests: Total skipped: 22
  • DQMHistoTests: Total Missing objects: 0
  • DQMHistoSizes: Histogram memory added: -7.2780000000000005 KiB( 49 files compared)
  • DQMHistoSizes: changed ( 11634.0,... ): -0.864 KiB Physics/NanoAODDQM
  • DQMHistoSizes: changed ( 13234.0,... ): -0.615 KiB Physics/NanoAODDQM
  • Checked 216 log files, 168 edm output root files, 50 DQM output files
  • TriggerResults: no differences found

NANO Comparison Summary

Summary:

  • No significant changes to the logs found
  • ROOTFileChecks: Some differences in event products or their sizes found
  • Reco comparison results: 12 differences found in the comparisons
  • DQMHistoTests: Total files compared: 11
  • DQMHistoTests: Total histograms compared: 10813
  • DQMHistoTests: Total failures: 0
  • DQMHistoTests: Total nulls: 0
  • DQMHistoTests: Total successes: 10813
  • DQMHistoTests: Total skipped: 0
  • DQMHistoTests: Total Missing objects: 0
  • DQMHistoSizes: Histogram memory added: -3.207 KiB( 10 files compared)
  • DQMHistoSizes: changed ( 2500.401,... ): -0.864 KiB Physics/NanoAODDQM
  • DQMHistoSizes: changed ( 2500.511 ): -0.615 KiB Physics/NanoAODDQM
  • Checked 23 log files, 10 edm output root files, 11 DQM output files

Nano size comparison Summary:

Sample kb/ev ref kb/ev diff kb/ev ev/s/thd ref ev/s/thd diff rate mem/thd ref mem/thd
2500.31 2.229 2.229 0.000 ( +0.0% ) 9.52 9.47 +0.5% 1.452 1.468
2500.311 2.319 2.319 0.000 ( +0.0% ) 9.19 8.96 +2.5% 1.823 1.839
2500.312 2.273 2.273 0.000 ( +0.0% ) 9.37 9.21 +1.7% 1.812 1.826
2500.33 1.095 1.095 0.000 ( +0.0% ) 21.79 21.41 +1.8% 1.637 1.643
2500.331 1.390 1.390 0.000 ( +0.0% ) 16.08 15.91 +1.0% 1.793 1.793
2500.332 1.321 1.321 0.000 ( +0.0% ) 17.71 17.89 -1.1% 1.843 1.855
2500.401 2.135 2.152 -0.017 ( -0.8% ) 10.49 10.44 +0.5% 1.137 1.161
2500.501 1.708 1.716 -0.008 ( -0.5% ) 16.62 16.42 +1.2% 1.049 1.080
2500.511 1.120 1.129 -0.008 ( -0.7% ) 30.90 30.99 -0.3% 1.304 1.339
2500.601 2.048 2.063 -0.015 ( -0.7% ) 12.54 12.64 -0.8% 1.120 1.136

@tvami
Copy link
Contributor

tvami commented Jan 18, 2023

@francescobrivio maybe it's time to remove "[RFC]" from the PR title?

@francescobrivio francescobrivio changed the title [RFC] Fix for wf 11634.15 Fix for wf 11634.15 Jan 18, 2023
@perrotta
Copy link
Contributor

perrotta commented Jan 18, 2023

@clacaputo @mandrenguyen @swertz @vlimant could you please at least confirm that you are reviewing this PR?

@perrotta
Copy link
Contributor

btagCSVV2 looks like having disappeared in several nanoAOD outputs: I imagine that this is expected, but please @francescobrivio (et al.) confirm

@swertz
Copy link
Contributor

swertz commented Jan 18, 2023

+xpog

CSVv2 removed from Run3, which is ok (not supported anymore). No other changes in central Nano.

@clacaputo @mandrenguyen @swertz @vlimant could you please at least confirm that you are reviewing this PR?

I reviewed it yesterday...

@perrotta
Copy link
Contributor

+1

  • Reco related changes are in RecoBTag/SoftLepton/python/SoftLeptonByMVAComputers_cff.py, and they look rather innocuous
  • Main updates and fixes in this PR refer to nanoAOD, and this was just approved by xpog (thank you @swertz )
  • Let merge this bypassing the reco signature, so that it can enter the 1100 IB, and we will be likely able to build pre3 in the afternoon: please @cms-sw/reconstruction-l2 comment/complain here if you see anything wrong with it, we can always revert later on

@perrotta
Copy link
Contributor

merge

@jfernan2
Copy link
Contributor

jfernan2 commented Sep 18, 2023

+1
(for the records)

@cmsbuild
Copy link
Contributor

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 be automatically merged.

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

8 participants