PPS: association cut fix #35941

jan-kaspar · 2021-11-01T14:03:29Z

PR description:

This is a bugfix PR to address the issues raised in:

The proposal is based mainly on these suggestions:

PR validation:

The plots below compare results before (blue) and after this PR (red dashed):

dirsim_cmp.pdf: for PPS direct simulation (+ reconstruction), Run2 and Run3
reco_cmp.pdf: for PPS reconstruction, Run2 data

No differences observed as expected.

cmsbuild · 2021-11-01T14:12:17Z

+code-checks

Logs: https://cmssdt.cern.ch/SDT/code-checks/cms-sw-PR-35941/26342

This PR adds an extra 20KB to repository

cmsbuild · 2021-11-01T14:12:41Z

A new Pull Request was created by @jan-kaspar for master.

It involves the following packages:

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

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

malbouis · 2021-11-01T14:20:31Z

please test

malbouis · 2021-11-01T14:21:13Z

type bug-fix

cmsbuild · 2021-11-01T17:39:37Z

-1

Failed Tests: UnitTests
Summary: https://cmssdt.cern.ch/SDT/jenkins-artifacts/pull-request-integration/PR-fe6071/20156/summary.html
COMMIT: 3849f99
CMSSW: CMSSW_12_2_X_2021-11-01-1100/slc7_amd64_gcc900
User test area: For local testing, you can use /cvmfs/cms-ci.cern.ch/week1/cms-sw/cmssw/35941/20156/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 TestDQMServicesDemo had ERRORS

Comparison Summary

Summary:

No significant changes to the logs found
Reco comparison results: 8 differences found in the comparisons
DQMHistoTests: Total files compared: 42
DQMHistoTests: Total histograms compared: 2901890
DQMHistoTests: Total failures: 7
DQMHistoTests: Total nulls: 0
DQMHistoTests: Total successes: 2901861
DQMHistoTests: Total skipped: 22
DQMHistoTests: Total Missing objects: 0
DQMHistoSizes: Histogram memory added: 0.0 KiB( 41 files compared)
Checked 177 log files, 37 edm output root files, 42 DQM output files
TriggerResults: no differences found

malbouis · 2021-11-01T17:48:30Z

please test with #35921

cmsbuild · 2021-11-01T20:58:40Z

+1

Summary: https://cmssdt.cern.ch/SDT/jenkins-artifacts/pull-request-integration/PR-fe6071/20162/summary.html
COMMIT: 3849f99
CMSSW: CMSSW_12_2_X_2021-11-01-1100/slc7_amd64_gcc900
User test area: For local testing, you can use /cvmfs/cms-ci.cern.ch/week1/cms-sw/cmssw/35941/20162/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: 8 differences found in the comparisons
DQMHistoTests: Total files compared: 42
DQMHistoTests: Total histograms compared: 2901890
DQMHistoTests: Total failures: 6
DQMHistoTests: Total nulls: 1
DQMHistoTests: Total successes: 2901861
DQMHistoTests: Total skipped: 22
DQMHistoTests: Total Missing objects: 0
DQMHistoSizes: Histogram memory added: -0.004 KiB( 41 files compared)
DQMHistoSizes: changed ( 312.0 ): -0.004 KiB MessageLogger/Warnings
Checked 177 log files, 37 edm output root files, 42 DQM output files
TriggerResults: no differences found

makortel · 2021-11-02T14:28:23Z

CondFormats/PPSObjects/interface/PPSAssociationCuts.h

+      p.second.buildFunctions();
+  }
+
+  const CutsPerArm &getAssociationCuts(const int sector) const { return association_cuts_.find(sector)->second; }


I just want to point out that with this approach a incorrect payload or sector value lead to undefined behavior (crashes at best, silent garbage output at worst).

Yes, I am aware. I wonder what the best/recommended solution would be... Possibly we could update the initialize method to validate the content. But what to do if wrong? Throw an exception? Is this allowed/recommended?

doing a check and throw (ie abort) at inizialize time is ok.
What is NOT ok is to check validity at each single access

Thanks @VinInn! Then I will push one more commit in an hour or so.

cmsbuild · 2021-11-02T17:10:35Z

+code-checks

Logs: https://cmssdt.cern.ch/SDT/code-checks/cms-sw-PR-35941/26372

This PR adds an extra 16KB to repository

cmsbuild · 2021-11-02T17:11:02Z

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

tvami · 2021-11-02T17:12:08Z

@cmsbuild , please test

cmsbuild · 2021-11-02T20:36:58Z

+1

Summary: https://cmssdt.cern.ch/SDT/jenkins-artifacts/pull-request-integration/PR-fe6071/20196/summary.html
COMMIT: 4eb4170
CMSSW: CMSSW_12_2_X_2021-11-02-1100/slc7_amd64_gcc900
Additional Tests: THREADING
User test area: For local testing, you can use /cvmfs/cms-ci.cern.ch/week1/cms-sw/cmssw/35941/20196/install.sh to create a dev area with all the needed externals and cmssw changes.

Comparison Summary

@slava77 comparisons for the following workflows were not done due to missing matrix map:

/data/cmsbld/jenkins/workspace/compare-root-files-short-matrix/data/PR-fe6071/136.796_RunMET2017C+RunMET2017C+HLTDR2_2017+RECODR2_2017reHLT_skimMET_Prompt+HARVEST2017
/data/cmsbld/jenkins/workspace/compare-root-files-short-matrix/data/PR-fe6071/136.816_RunHLTPhy2017E+RunHLTPhy2017E+HLTDR2_2017+RECODR2_2017reHLT_Prompt+HARVEST2017
/data/cmsbld/jenkins/workspace/compare-root-files-short-matrix/data/PR-fe6071/136.8642_RunJetHT2018BHEfail+RunJetHT2018BHEfail+HLTDR2_2018+RECODR2_2018reHLT_skimJetHT_Prompt_HEfail+HARVEST2018_HEfail
/data/cmsbld/jenkins/workspace/compare-root-files-short-matrix/data/PR-fe6071/136.8861_RunEGamma2018Dml1+RunEGamma2018Dml1+HLTDR2_2018ml+RECODR2_2018reHLT_skimEGamma_Prompt_L1TEgDQM+HARVEST2018_L1TEgDQM_Prompt
/data/cmsbld/jenkins/workspace/compare-root-files-short-matrix/data/PR-fe6071/23234.103_TTbar_14TeV+2026D49Aging3000+TTbar_14TeV_TuneCP5_GenSimHLBeamSpot14+DigiTrigger+RecoGlobal+HARVESTGlobal

Summary:

No significant changes to the logs found
Reco comparison results: 8 differences found in the comparisons
DQMHistoTests: Total files compared: 42
DQMHistoTests: Total histograms compared: 2901890
DQMHistoTests: Total failures: 7
DQMHistoTests: Total nulls: 0
DQMHistoTests: Total successes: 2901861
DQMHistoTests: Total skipped: 22
DQMHistoTests: Total Missing objects: 0
DQMHistoSizes: Histogram memory added: 0.0 KiB( 41 files compared)
Checked 177 log files, 37 edm output root files, 42 DQM output files
TriggerResults: no differences found

tvami · 2021-11-02T20:49:39Z

+1

multi-threaded tests pass now
follow-up to the review happened according to the feedback

cmsbuild · 2021-11-02T20:50:03Z

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. @perrotta, @dpiparo, @qliphy (and backports should be raised in the release meeting by the corresponding L2)

qliphy · 2021-11-03T00:56:53Z

+1

jan-kaspar added 2 commits November 1, 2021 14:27

replace std::vector::at with operator[]

9512cc1

remove mutable class members, use REGISTER_PLUGIN_INIT instead

8362ee6

cmsbuild added this to the CMSSW_12_2_X milestone Nov 1, 2021

cmsbuild added alca-pending code-checks-pending db-pending orp-pending pending-signatures tests-pending labels Nov 1, 2021

This was referenced Nov 1, 2021

CTPPS related issue in several IB workflows #35928

Closed

Range check issue in CTPPS breaking IB #35936

Closed

code format

3849f99

cmsbuild added code-checks-approved and removed code-checks-pending labels Nov 1, 2021

cmsbuild added tests-started and removed tests-pending labels Nov 1, 2021

cmsbuild added the bug-fix label Nov 1, 2021

cmsbuild added tests-rejected and removed tests-started labels Nov 1, 2021

cmsbuild added requires-external tests-started and removed tests-rejected labels Nov 1, 2021

cmsbuild removed the tests-started label Nov 1, 2021

makortel reviewed Nov 2, 2021

View reviewed changes

jan-kaspar added 2 commits November 2, 2021 17:57

validate structure during initialisation

3bd6d0b

code format

4eb4170

cmsbuild added code-checks-pending tests-pending and removed tests-started code-checks-approved labels Nov 2, 2021

cmsbuild added code-checks-approved and removed code-checks-pending labels Nov 2, 2021

cmsbuild added tests-started and removed tests-pending labels Nov 2, 2021

cmsbuild added tests-approved and removed tests-started labels Nov 2, 2021

cmsbuild added alca-approved db-approved fully-signed and removed alca-pending db-pending pending-signatures labels Nov 2, 2021

cmsbuild added orp-approved and removed orp-pending labels Nov 3, 2021

cmsbuild merged commit f681b5e into cms-sw:master Nov 3, 2021

qliphy mentioned this pull request Nov 4, 2021

IB Failure with workflow 23234.103 #35979

Closed

PPS: association cut fix #35941

PPS: association cut fix #35941

Uh oh!

Conversation

jan-kaspar commented Nov 1, 2021

PR description:

PR validation:

Uh oh!

cmsbuild commented Nov 1, 2021

Uh oh!

cmsbuild commented Nov 1, 2021

Uh oh!

malbouis commented Nov 1, 2021

Uh oh!

malbouis commented Nov 1, 2021

Uh oh!

cmsbuild commented Nov 1, 2021

Unit Tests

Comparison Summary

Uh oh!

malbouis commented Nov 1, 2021

Uh oh!

cmsbuild commented Nov 1, 2021

Comparison Summary

Uh oh!

makortel Nov 2, 2021

Choose a reason for hiding this comment

Uh oh!

jan-kaspar Nov 2, 2021

Choose a reason for hiding this comment

Uh oh!

VinInn Nov 2, 2021

Choose a reason for hiding this comment

Uh oh!

jan-kaspar Nov 2, 2021

Choose a reason for hiding this comment

Uh oh!

cmsbuild commented Nov 2, 2021

Uh oh!

cmsbuild commented Nov 2, 2021

Uh oh!

tvami commented Nov 2, 2021

Uh oh!

cmsbuild commented Nov 2, 2021

Comparison Summary

Uh oh!

tvami commented Nov 2, 2021

Uh oh!

cmsbuild commented Nov 2, 2021

Uh oh!

qliphy commented Nov 3, 2021

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

8 participants