Run3-sim129 Use token rather than label for accessing collections in FastSimulation/ForwardDetectors #39666

bsunanda · 2022-10-07T06:17:24Z

PR description:

Use tokens rather than labels for accessing collections in FastSimulation/ForwardDetectors

PR validation:

Use the runTheMatrix test workflows

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:

Nothing special

…on/ForwardDetectors

cmsbuild · 2022-10-07T06:23:23Z

-code-checks

Logs: https://cmssdt.cern.ch/SDT/code-checks/cms-sw-PR-39666/32471

This PR adds an extra 16KB to repository

Code check has found code style and quality issues which could be resolved by applying following patch(s)

code-format:
https://cmssdt.cern.ch/SDT/code-checks/cms-sw-PR-39666/32471/code-format.patch
e.g. curl -k https://cmssdt.cern.ch/SDT/code-checks/cms-sw-PR-39666/32471/code-format.patch | patch -p1
You can also run scram build code-format to apply code format directly

cmsbuild · 2022-10-07T06:31:27Z

+code-checks

Logs: https://cmssdt.cern.ch/SDT/code-checks/cms-sw-PR-39666/32472

This PR adds an extra 16KB to repository

cmsbuild · 2022-10-07T06:31:48Z

A new Pull Request was created by @bsunanda (Sunanda Banerjee) for master.

It involves the following packages:

FastSimulation/ForwardDetectors (fastsim)

@cmsbuild, @ssekmen, @civanch, @mdhildreth, @sbein can you please review it and eventually sign? Thanks.
@matt-komm 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

bsunanda · 2022-10-07T09:56:47Z

@cmsbuild Please test

cmsbuild · 2022-10-07T14:35:25Z

+1

Summary: https://cmssdt.cern.ch/SDT/jenkins-artifacts/pull-request-integration/PR-58dfc0/28101/summary.html
COMMIT: 7917dff
CMSSW: CMSSW_12_6_X_2022-10-06-2300/el8_amd64_gcc10
User test area: For local testing, you can use /cvmfs/cms-ci.cern.ch/week1/cms-sw/cmssw/39666/28101/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-58dfc0/41834.0_TTbar_14TeV+2026D94+TTbar_14TeV_TuneCP5_GenSimHLBeamSpot14+DigiTrigger+RecoGlobal+HARVESTGlobal

Summary:

No significant changes to the logs found
Reco comparison results: 10 differences found in the comparisons
DQMHistoTests: Total files compared: 49
DQMHistoTests: Total histograms compared: 3391103
DQMHistoTests: Total failures: 12
DQMHistoTests: Total nulls: 0
DQMHistoTests: Total successes: 3391069
DQMHistoTests: Total skipped: 22
DQMHistoTests: Total Missing objects: 0
DQMHistoSizes: Histogram memory added: 0.0 KiB( 48 files compared)
Checked 204 log files, 49 edm output root files, 49 DQM output files
TriggerResults: no differences found

civanch · 2022-10-09T09:09:57Z

+1

all modifications are clear improvements/code clean-up

cmsbuild · 2022-10-09T09:10:15Z

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

perrotta · 2022-10-09T17:45:17Z

FastSimulation/ForwardDetectors/plugins/FastSimDataFilter.cc


  void FastSimDataFilter::beginJob() {
    ntotal = 0;
    npassed = 0;
  }
  void FastSimDataFilter::endJob() {
-    cout << " FastSimDataFilter: accepted " << npassed << "  out of total " << ntotal << endl;
+    edm::LogVerbatim("FastSim") << " FastSimDataFilter: accepted " << npassed << "  out of total " << ntotal;


Writing the name of the class, instead of a generic "FastSim" can help in case of using these messages for debug:

Suggested change

edm::LogVerbatim("FastSim") << " FastSimDataFilter: accepted " << npassed << " out of total " << ntotal;

edm::LogVerbatim("FastSimDataFilter") << " FastSimDataFilter: accepted " << npassed << " out of total " << ntotal;

perrotta · 2022-10-09T17:47:07Z

FastSimulation/ForwardDetectors/plugins/ProtonTaggerFilter.cc


  switch (beam1mode) {
    case 0:
-      std::cout << "Option chosen for beam 1: ingnore" << std::endl;
+      edm::LogVerbatim("FastSim") << "Option chosen for beam 1: ingnore";


Suggested change

edm::LogVerbatim("FastSim") << "Option chosen for beam 1: ingnore";

edm::LogVerbatim("FastSimProtonTaggerFilter") << "Option chosen for beam 1: ingnore";

etc...

cmsbuild · 2022-10-12T01:56:23Z

-code-checks

Logs: https://cmssdt.cern.ch/SDT/code-checks/cms-sw-PR-39666/32528

This PR adds an extra 20KB to repository

Code check has found code style and quality issues which could be resolved by applying following patch(s)

code-format:
https://cmssdt.cern.ch/SDT/code-checks/cms-sw-PR-39666/32528/code-format.patch
e.g. curl -k https://cmssdt.cern.ch/SDT/code-checks/cms-sw-PR-39666/32528/code-format.patch | patch -p1
You can also run scram build code-format to apply code format directly

cmsbuild · 2022-10-12T02:04:30Z

+code-checks

Logs: https://cmssdt.cern.ch/SDT/code-checks/cms-sw-PR-39666/32530

This PR adds an extra 20KB to repository

cmsbuild · 2022-10-12T02:04:56Z

Pull request #39666 was updated. @cmsbuild, @ssekmen, @civanch, @mdhildreth, @sbein can you please check and sign again.

bsunanda · 2022-10-12T02:05:54Z

@cmsbuild Please test

perrotta · 2022-10-12T05:55:31Z

@perrotta the log verbatim is not commented out. Please take a look at the code

Ah, ok @bsunanda : now I see that you already fixed it in a different line with commit e58e298
That was pretty enough, indeed: would you have answered telling that the fix was applied on a different line I would not have insisted.

cmsbuild · 2022-10-12T09:47:53Z

+1

Summary: https://cmssdt.cern.ch/SDT/jenkins-artifacts/pull-request-integration/PR-58dfc0/28183/summary.html
COMMIT: 44a383a
CMSSW: CMSSW_12_6_X_2022-10-11-2300/el8_amd64_gcc10
User test area: For local testing, you can use /cvmfs/cms-ci.cern.ch/week0/cms-sw/cmssw/39666/28183/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-58dfc0/20834.0_TTbar_14TeV+2026D88+TTbar_14TeV_TuneCP5_GenSimHLBeamSpot14+DigiTrigger+RecoGlobal+HARVESTGlobal
/data/cmsbld/jenkins/workspace/compare-root-files-short-matrix/data/PR-58dfc0/20834.75_TTbar_14TeV+2026D88_HLT75e33+TTbar_14TeV_TuneCP5_GenSimHLBeamSpot14+DigiTrigger+RecoGlobal+HLT75e33+HARVESTGlobal
/data/cmsbld/jenkins/workspace/compare-root-files-short-matrix/data/PR-58dfc0/20896.0_CloseByPGun_CE_E_Front_120um+2026D88+CE_E_Front_120um_GenSimHLBeamSpotHGCALCloseBy+DigiTrigger+RecoGlobal+HARVESTGlobal
/data/cmsbld/jenkins/workspace/compare-root-files-short-matrix/data/PR-58dfc0/20900.0_CloseByPGun_CE_H_Coarse_Scint+2026D88+CE_H_Coarse_Scint_GenSimHLBeamSpotHGCALCloseBy+DigiTrigger+RecoGlobal+HARVESTGlobal
/data/cmsbld/jenkins/workspace/compare-root-files-short-matrix/data/PR-58dfc0/21034.999_TTbar_14TeV+2026D88PU_PMXS1S2PR+TTbar_14TeV_TuneCP5_GenSimHLBeamSpot14+PREMIX_PremixHLBeamSpot14PU+DigiTriggerPU+RecoGlobalPU+HARVESTGlobalPU
/data/cmsbld/jenkins/workspace/compare-root-files-short-matrix/data/PR-58dfc0/23234.0_TTbar_14TeV+2026D94+TTbar_14TeV_TuneCP5_GenSimHLBeamSpot14+DigiTrigger+RecoGlobal+HARVESTGlobal

Summary:

No significant changes to the logs found
Reco comparison results: 6 differences found in the comparisons
DQMHistoTests: Total files compared: 49
DQMHistoTests: Total histograms compared: 3392309
DQMHistoTests: Total failures: 9
DQMHistoTests: Total nulls: 0
DQMHistoTests: Total successes: 3392278
DQMHistoTests: Total skipped: 22
DQMHistoTests: Total Missing objects: 0
DQMHistoSizes: Histogram memory added: 0.0 KiB( 48 files compared)
Checked 204 log files, 49 edm output root files, 49 DQM output files
TriggerResults: no differences found

civanch · 2022-10-17T15:56:46Z

+1

cmsbuild · 2022-10-17T15:57:06Z

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

perrotta · 2022-10-17T17:07:38Z

+1

Use token rather than label for accessing collections in FastSimulati…

f531ff9

…on/ForwardDetectors

cmsbuild added this to the CMSSW_12_6_X milestone Oct 7, 2022

cmsbuild added code-checks-pending fastsim-pending orp-pending pending-signatures tests-pending labels Oct 7, 2022

cmsbuild added code-checks-rejected and removed code-checks-pending labels Oct 7, 2022

Code check

7917dff

cmsbuild added code-checks-pending and removed code-checks-rejected labels Oct 7, 2022

cmsbuild added code-checks-approved and removed code-checks-pending labels Oct 7, 2022

cmsbuild added tests-started and removed tests-pending labels Oct 7, 2022

cmsbuild added tests-approved and removed tests-started labels Oct 7, 2022

cmsbuild added fastsim-approved fully-signed and removed fastsim-pending pending-signatures labels Oct 9, 2022

perrotta reviewed Oct 9, 2022

View reviewed changes

Add EDM_ML_DEBUG for some of the prints

2dacb26

cmsbuild added code-checks-pending and removed code-checks-approved labels Oct 12, 2022

cmsbuild added code-checks-rejected and removed code-checks-pending labels Oct 12, 2022

Code check

44a383a

cmsbuild added code-checks-pending and removed code-checks-rejected labels Oct 12, 2022

cmsbuild added code-checks-approved and removed code-checks-pending labels Oct 12, 2022

cmsbuild added tests-started and removed tests-pending labels Oct 12, 2022

cmsbuild added tests-approved and removed tests-started labels Oct 12, 2022

cmsbuild added fastsim-approved fully-signed and removed fastsim-pending pending-signatures labels Oct 17, 2022

cmsbuild added orp-approved and removed orp-pending labels Oct 17, 2022

cmsbuild merged commit 88844f7 into cms-sw:master Oct 17, 2022

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Run3-sim129 Use token rather than label for accessing collections in FastSimulation/ForwardDetectors #39666

Run3-sim129 Use token rather than label for accessing collections in FastSimulation/ForwardDetectors #39666

bsunanda commented Oct 7, 2022

cmsbuild commented Oct 7, 2022

cmsbuild commented Oct 7, 2022

cmsbuild commented Oct 7, 2022

bsunanda commented Oct 7, 2022

cmsbuild commented Oct 7, 2022

civanch commented Oct 9, 2022

cmsbuild commented Oct 9, 2022

perrotta Oct 9, 2022

perrotta Oct 9, 2022

cmsbuild commented Oct 12, 2022

cmsbuild commented Oct 12, 2022

cmsbuild commented Oct 12, 2022

bsunanda commented Oct 12, 2022

perrotta commented Oct 12, 2022

cmsbuild commented Oct 12, 2022

civanch commented Oct 17, 2022

cmsbuild commented Oct 17, 2022

perrotta commented Oct 17, 2022

	edm::LogVerbatim("FastSim") << " FastSimDataFilter: accepted " << npassed << " out of total " << ntotal;
	edm::LogVerbatim("FastSimDataFilter") << " FastSimDataFilter: accepted " << npassed << " out of total " << ntotal;

	edm::LogVerbatim("FastSim") << "Option chosen for beam 1: ingnore";
	edm::LogVerbatim("FastSimProtonTaggerFilter") << "Option chosen for beam 1: ingnore";

Run3-sim129 Use token rather than label for accessing collections in FastSimulation/ForwardDetectors #39666

Run3-sim129 Use token rather than label for accessing collections in FastSimulation/ForwardDetectors #39666

Conversation

bsunanda commented Oct 7, 2022

PR description:

PR validation:

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:

cmsbuild commented Oct 7, 2022

cmsbuild commented Oct 7, 2022

cmsbuild commented Oct 7, 2022

bsunanda commented Oct 7, 2022

cmsbuild commented Oct 7, 2022

Comparison Summary

civanch commented Oct 9, 2022

cmsbuild commented Oct 9, 2022

perrotta Oct 9, 2022

Choose a reason for hiding this comment

perrotta Oct 9, 2022

Choose a reason for hiding this comment

cmsbuild commented Oct 12, 2022

cmsbuild commented Oct 12, 2022

cmsbuild commented Oct 12, 2022

bsunanda commented Oct 12, 2022

perrotta commented Oct 12, 2022

cmsbuild commented Oct 12, 2022

Comparison Summary

civanch commented Oct 17, 2022

cmsbuild commented Oct 17, 2022

perrotta commented Oct 17, 2022