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

PUID UL18 backport #32872

Merged

Conversation

singh-ramanpreet
Copy link
Contributor

PR description:

PR validation:

Re-miniAOD info

- run2_miniAOD_UL.toModify(pileupJetId, algos = _chsalgos_106X_UL17)
+ run2_miniAOD_UL.toModify(pileupJetId, algos = _chsalgos_106X_UL18)
+ (run2_miniAOD_UL & run2_jme_2017).toModify(pileupJetId, algos = _chsalgos_106X_UL17)

Default for UL campaign will be UL18 and, for UL17 & UL16* it will be using run2_jme_2017 & run2_jme_2016* respectively.

*(in future)

Code changes

I have kept code changes to minimal since it's a backport, but I can do same level of cleaning as it was done in main PR.

FYI: @alefisico @camclean @marinakolosova

@cmsbuild
Copy link
Contributor

cmsbuild commented Feb 10, 2021

A new Pull Request was created by @singh-ramanpreet (Ramanpreet Singh) for CMSSW_10_6_X.

It involves the following packages:

RecoJets/JetProducers

@perrotta, @jpata, @cmsbuild, @slava77 can you please review it and eventually sign? Thanks.
@rappoccio, @jdolen, @ahinzmann, @jdamgov, @yslai, @nhanvtran, @gkasieczka, @clelange, @schoef, @mariadalfonso, @seemasharmafnal 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

@slava77
Copy link
Contributor

slava77 commented Feb 10, 2021

Default for UL campaign will be UL18 and, for UL17 & UL16* it will be using run2_jme_2017 & run2_jme_2016* respectively.

*(in future)

what is the meaning of the "future" here?
IIUC, the current re-miniAOD UL campaign for 2018 (as well as 2017) is already in progress. We do not allow changes in the production release used by the campaign.

There was some discussion about another re-miniAOD, but that was planned to be done in 11_X or later release.

It seems like the current proposal for the backport should be using run2_miniAOD_devel modifier to satisfy the no-change policy and also allow using the new PU ID in private tests.

@slava77
Copy link
Contributor

slava77 commented Feb 10, 2021

test parameters:

@singh-ramanpreet
Copy link
Contributor Author

singh-ramanpreet commented Feb 10, 2021

what is the meaning of the "future" here?

I thought all the UL campaign will be using 10_6_X. That's why I am assuming UL16 training when available it will also be backported here.

IIUC, the current re-miniAOD UL campaign for 2018 (as well as 2017) is already in progress. We do not allow changes in the production release used by the campaign.

There was some discussion about another re-miniAOD, but that was planned to be done in 11_X or later release.

It seems like the current proposal for the backport should be using run2_miniAOD_devel modifier to satisfy the no-change policy and also allow using the new PU ID in private tests.

Okay, then is there a need for backport? @alefisico @camclean

@alefisico
Copy link
Contributor

what is the meaning of the "future" here?

I thought all the UL campaign will be using 10_6_X. That's why I am assuming UL16 training when available it will also be backported here.

IIUC, the current re-miniAOD UL campaign for 2018 (as well as 2017) is already in progress. We do not allow changes in the production release used by the campaign.
There was some discussion about another re-miniAOD, but that was planned to be done in 11_X or later release.
It seems like the current proposal for the backport should be using run2_miniAOD_devel modifier to satisfy the no-change policy and also allow using the new PU ID in private tests.

Okay, then is there a need for backport? @alefisico @camclean

We will need this backport for any future miniAOD campaign. I think here is more important for the next nanoAOD campaign. (That PR will be done asap)

@mariadalfonso
Copy link
Contributor

mariadalfonso commented Feb 11, 2021

for 11_2 we need a backport

for 10_6 :
change run2_miniAOD_UL into run2_miniAOD_devel for the mini part.
this backport is still needed since it's a partial step, with the changes needed to nano.
would be best to have the nanoPR soo and then we take care of backport all at once in 10_6

@slava77
Copy link
Contributor

slava77 commented Feb 11, 2021

* need latest version of https://github.com/cms-data/RecoJets-JetProducers (`V05-13-00`).

it should be available in the next 10_6_X IB (IIUC, starting from CMSSW_10_6_X_2021-02-11-2300)

@perrotta
Copy link
Contributor

backport of #32418

@singh-ramanpreet
Copy link
Contributor Author

still little idea,
so for this backport (10_6_X)
is it,

- run2_miniAOD_UL.toModify(pileupJetId, algos = _chsalgos_106X_UL17)
+ run2_miniAOD_devel.toModify(pileupJetId, algos = _chsalgos_106X_UL18)
+ (run2_miniAOD_devel & run2_jme_2017).toModify(pileupJetId, algos = _chsalgos_106X_UL17)

or addition,

  run2_miniAOD_UL.toModify(pileupJetId, algos = _chsalgos_106X_UL17)
+ run2_miniAOD_devel.toModify(pileupJetId, algos = _chsalgos_106X_UL18)

or just devel with latest,

- run2_miniAOD_UL.toModify(pileupJetId, algos = _chsalgos_106X_UL17)
+ run2_miniAOD_devel.toModify(pileupJetId, algos = _chsalgos_106X_UL18)

Comment on lines 38 to 39
run2_miniAOD_UL.toModify(pileupJetId, algos = _chsalgos_106X_UL18)
(run2_miniAOD_UL & run2_jme_2017).toModify(pileupJetId, algos = _chsalgos_106X_UL17)
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
run2_miniAOD_UL.toModify(pileupJetId, algos = _chsalgos_106X_UL18)
(run2_miniAOD_UL & run2_jme_2017).toModify(pileupJetId, algos = _chsalgos_106X_UL17)
run2_miniAOD_UL.toModify(pileupJetId, algos = _chsalgos_106X_UL17)
(run2_miniAOD_devel & run2_jme_2018).toModify(pileupJetId, algos = _chsalgos_106X_UL18)

is what I understood is needed

  • the first line reverts back to the current default of the PR
  • the second enables the 2018 PU ID for the 2018 "devel" re-miniAOD

Copy link
Contributor Author

Choose a reason for hiding this comment

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

there is no run2_jme_2018_cff, did you mean run2_miniAOD_devel.toModify(pileupJetId, algos = _chsalgos_106X_UL18) for second line?

Copy link
Contributor

@slava77 slava77 Feb 11, 2021

Choose a reason for hiding this comment

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

my expectation is that my proposed change above when used with --era Run2_2018,run2_miniAOD_devel --procModifiers run2_miniAOD_UL will give an updated UL re-mini with the 2018 PU ID for the 2018 era/setup. (to compare with the production default --era Run2_2018 --procModifiers run2_miniAOD_UL which is locked to use PU ID 2017 by the no-change policy due to the 2018 campaign being already in progress)

Copy link
Contributor

@slava77 slava77 Feb 11, 2021

Choose a reason for hiding this comment

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

there is no run2_jme_2018_cff, did you mean run2_miniAOD_devel.toModify(pileupJetId, algos = _chsalgos_106X_UL18) for second line?

Ehm, it may be better that run2_jme_2018 actually existed.
Otherwise, the last line would have (run2_miniAOD_devel & (~run2_jme_2016) & (~run2_jme_2017)).

@cmsbuild
Copy link
Contributor

Pull request #32872 was updated. @perrotta, @jpata, @cmsbuild, @slava77 can you please check and sign again.

@singh-ramanpreet
Copy link
Contributor Author

fixed, only addition is now
run2_miniAOD_devel.toModify(pileupJetId, algos = _chsalgos_106X_UL18)

please review/test.

@@ -34,6 +35,7 @@
# residualsTxt = cms.FileInPath("RecoJets/JetProducers/data/download.url") # must be an existing file
)
run2_miniAOD_UL.toModify(pileupJetId, algos = _chsalgos_106X_UL17)
run2_miniAOD_devel.toModify(pileupJetId, algos = _chsalgos_106X_UL18)
Copy link
Contributor

Choose a reason for hiding this comment

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

from #32872 (comment)

Ehm, it may be better that run2_jme_2018 actually existed.
Otherwise, the last line would have (run2_miniAOD_devel & (~run2_jme_2016) & (~run2_jme_2017)).

Copy link
Contributor Author

Choose a reason for hiding this comment

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

okay, somehow I missed this comment. I will push another quick fix.

@cmsbuild
Copy link
Contributor

Pull request #32872 was updated. @perrotta, @jpata, @cmsbuild, @slava77 can you please check and sign again.

@singh-ramanpreet
Copy link
Contributor Author

now it should be in line with comments.

please review/test.

@slava77
Copy link
Contributor

slava77 commented Feb 11, 2021

@cmsbuild please test

@cmsbuild
Copy link
Contributor

-1

Failed Tests: UnitTests RelVals RelVals-INPUT
Summary: https://cmssdt.cern.ch/SDT/jenkins-artifacts/pull-request-integration/PR-8c8b6b/12847/summary.html
COMMIT: e8c384a
CMSSW: CMSSW_10_6_X_2021-02-11-1100/slc7_amd64_gcc700
User test area: For local testing, you can use /cvmfs/cms-ci.cern.ch/week1/cms-sw/cmssw/32872/12847/install.sh to create a dev area with all the needed externals and cmssw changes.

Unit Tests

I found errors in the following unit tests:

---> test runtestPhysicsToolsNanoAOD had ERRORS

RelVals

  • 136.8523136.8523_RunJetHT2018C_nanoULremini+RunJetHT2018C_nanoULremini+NANOEDM2018_106Xv2+HARVESTNANOAOD2018_106Xv2/step2_RunJetHT2018C_nanoULremini+RunJetHT2018C_nanoULremini+NANOEDM2018_106Xv2+HARVESTNANOAOD2018_106Xv2.log
  • 136.88811136.88811_RunJetHT2018D_reminiaodUL+RunJetHT2018D_reminiaodUL+REMINIAOD_data2018UL+HARVEST2018_REMINIAOD_data2018UL/step2_RunJetHT2018D_reminiaodUL+RunJetHT2018D_reminiaodUL+REMINIAOD_data2018UL+HARVEST2018_REMINIAOD_data2018UL.log
  • 136.8311136.8311_RunJetHT2017F_reminiaod+RunJetHT2017F_reminiaod+REMINIAOD_data2017+HARVEST2017_REMINIAOD_data2017/step2_RunJetHT2017F_reminiaod+RunJetHT2017F_reminiaod+REMINIAOD_data2017+HARVEST2017_REMINIAOD_data2017.log
Expand to see more relval errors ...

RelVals-INPUT

  • 4.64.6_MinimumBias2010A+MinimumBias2010A+RECOSKIMALCA+HARVESTDR1/step2_MinimumBias2010A+MinimumBias2010A+RECOSKIMALCA+HARVESTDR1.log
  • 136.72411136.72411_RunJetHT2016B_reminiaodUL+RunJetHT2016B_reminiaodUL+REMINIAOD_data2016UL_HIPM+HARVESTDR2_REMINIAOD_data2016UL_HIPM/step2_RunJetHT2016B_reminiaodUL+RunJetHT2016B_reminiaodUL+REMINIAOD_data2016UL_HIPM+HARVESTDR2_REMINIAOD_data2016UL_HIPM.log
  • 136.7611136.7611_RunJetHT2016E_reminiaod+RunJetHT2016E_reminiaod+REMINIAOD_data2016_HIPM+HARVESTDR2_REMINIAOD_data2016_HIPM/step2_RunJetHT2016E_reminiaod+RunJetHT2016E_reminiaod+REMINIAOD_data2016_HIPM+HARVESTDR2_REMINIAOD_data2016_HIPM.log
Expand to see more relval errors ...

@singh-ramanpreet
Copy link
Contributor Author

Unit Tests

I found errors in the following unit tests:

---> test runtestPhysicsToolsNanoAOD had ERRORS

run2_miniAOD_devel is not imported. need to fix that.

Co-authored-by: Slava Krutelyov <slava77@gmail.com>
@cmsbuild
Copy link
Contributor

Pull request #32872 was updated. @perrotta, @jpata, @cmsbuild, @slava77 can you please check and sign again.

@slava77
Copy link
Contributor

slava77 commented Feb 11, 2021

@cmsbuild please test

@cmsbuild
Copy link
Contributor

+1

Summary: https://cmssdt.cern.ch/SDT/jenkins-artifacts/pull-request-integration/PR-8c8b6b/12849/summary.html
COMMIT: bc2beb8
CMSSW: CMSSW_10_6_X_2021-02-11-1100/slc7_amd64_gcc700
User test area: For local testing, you can use /cvmfs/cms-ci.cern.ch/week1/cms-sw/cmssw/32872/12849/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: 0 differences found in the comparisons
  • DQMHistoTests: Total files compared: 35
  • DQMHistoTests: Total histograms compared: 3215540
  • DQMHistoTests: Total failures: 1
  • DQMHistoTests: Total nulls: 0
  • DQMHistoTests: Total successes: 3215205
  • DQMHistoTests: Total skipped: 334
  • DQMHistoTests: Total Missing objects: 0
  • DQMHistoSizes: Histogram memory added: 0.0 KiB( 34 files compared)
  • Checked 143 log files, 29 edm output root files, 35 DQM output files

@slava77
Copy link
Contributor

slava77 commented Feb 12, 2021

+1

for #32872 bc2beb8

  • this is a partial backport from Pileup Id training for UL18 #32418 , staying in the constraints of the no-change policy/requirement for the production re-miniAOD setup
    • cleanup of old configs is not backported
    • more appropriate/improved 94X/80X/UL modifications are not applied to keep changes minimal
    • the new UL18 PU ID is made available via run2_miniAOD_devel, which can be added on top of the production miniAOD configuration for analysis or private tests
  • jenkins tests pass and comparisons show no differences

The external files should already be in the IB according to cms-sw/cmsdist@IB/CMSSW_10_6_X_2021-02-07-0000/slc7_amd64_gcc700...IB/CMSSW_10_6_X_2021-02-11-2300/slc7_amd64_gcc700

@cmsbuild
Copy link
Contributor

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

@qliphy
Copy link
Contributor

qliphy commented Feb 14, 2021

+1

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