Modifying the AlcaPCCIntegrator for processing concurrent lumiblocks #37538

radla118 · 2022-04-12T12:16:35Z

PR description:

The former AlcaPCCIntegrator is not capable of processing concurrent luminosity blocks, since it is an edm::one::EDProducer. The new module is implemented as an edm::global::EDProducer to be able to run on mulitple threads. The output is not modified, and the corresponding workflow remains the same.

Temporary fix and description of the issue: #37130

PR validation:

The modified producer is tested locally.

if this PR is a backport please specify the original PR and why you need to backport that PR:

Before submitting your pull requests, make sure you followed this checklist:

verify that the PR is really intended for the chosen branch
verify that changes follow CMS Naming, Coding, And Style Rules
verify that the PR passes the basic test procedure suggested in the CMSSW PR instructions

cmsbuild · 2022-04-12T12:24:25Z

+code-checks

Logs: https://cmssdt.cern.ch/SDT/code-checks/cms-sw-PR-37538/29258

This PR adds an extra 12KB to repository

cmsbuild · 2022-04-12T12:24:49Z

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

It involves the following packages:

Calibration/LumiAlCaRecoProducers (alca)

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

tvami · 2022-04-12T14:49:38Z

Calibration/LumiAlCaRecoProducers/plugins/AlcaPCCIntegrator.cc

  edm::Handle<reco::PixelClusterCountsInEvent> pccHandle;
  iEvent.getByToken(pccToken_, pccHandle);

  const reco::PixelClusterCountsInEvent inputPcc = *(pccHandle.product());


This can be simplified to

Suggested change

edm::Handle<reco::PixelClusterCountsInEvent> pccHandle;

iEvent.getByToken(pccToken_, pccHandle);

const reco::PixelClusterCountsInEvent inputPcc = *(pccHandle.product());

const reco::PixelClusterCountsInEvent &inputPcc = iEvent.get(pccToken);

tvami · 2022-04-12T14:51:06Z

test parameters:

enable_tests = threading
workflows = 1020

tvami · 2022-04-12T14:51:33Z

@cmsbuild , please test

tvami · 2022-04-12T14:54:32Z

Hi @makortel , since this PR is about moving to multi-threading, I'd appreciate if you could have a look at it and give some feedback. Thanks!

mmusich · 2022-04-12T14:56:36Z

Shouldn't the real test be done by reverting in the same PR also #37130?

tvami · 2022-04-12T15:01:59Z

Shouldn't the real test be done by reverting in the same PR also #37130?

Oh yes, good point!

tvami · 2022-04-12T15:02:05Z

@cmsbuild , please abort

tvami · 2022-04-12T15:07:11Z

@radla118 can you please do that in this PR?
Just remove these two lines
https://github.com/tvami/cmssw/blob/a9ee76f11f41caea8c32d2e37c239678addc3c15/Configuration/AlCa/python/autoAlca.py#L134-L135

tvami · 2022-04-12T15:29:44Z

@cmsbuild , please test

cmsbuild · 2022-04-12T15:34:23Z

+code-checks

Logs: https://cmssdt.cern.ch/SDT/code-checks/cms-sw-PR-37538/29261

This PR adds an extra 16KB to repository

cmsbuild · 2022-04-29T11:51:23Z

+1

Summary: https://cmssdt.cern.ch/SDT/jenkins-artifacts/pull-request-integration/PR-82e4f8/24324/summary.html
COMMIT: 78daa84
CMSSW: CMSSW_12_4_X_2022-04-28-2300/slc7_amd64_gcc10
Additional Tests: THREADING
User test area: For local testing, you can use /cvmfs/cms-ci.cern.ch/week0/cms-sw/cmssw/37538/24324/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-82e4f8/1001.2_RunZeroBias2017F+RunZeroBias2017F+TIER0EXPRUN2+ALCAEXPRUN2+ALCAHARVDSIPIXELCAL+ALCAHARVDSIPIXELCALLA
/data/cmsbld/jenkins/workspace/compare-root-files-short-matrix/data/PR-82e4f8/1020.0_AlCaLumiPixels2021+AlCaLumiPixels2021+TIER0EXPLP+ALCAEXPLP+ALCAHARVLP+TIER0PROMPTLP

Summary:

No significant changes to the logs found
Reco comparison results: 7 differences found in the comparisons
DQMHistoTests: Total files compared: 51
DQMHistoTests: Total histograms compared: 3695616
DQMHistoTests: Total failures: 13
DQMHistoTests: Total nulls: 1
DQMHistoTests: Total successes: 3695580
DQMHistoTests: Total skipped: 22
DQMHistoTests: Total Missing objects: 0
DQMHistoSizes: Histogram memory added: -0.004 KiB( 50 files compared)
DQMHistoSizes: changed ( 312.0 ): -0.004 KiB MessageLogger/Warnings
Checked 215 log files, 45 edm output root files, 51 DQM output files
TriggerResults: no differences found

tvami · 2022-04-29T13:47:35Z

Hi @radla118 would you like to deal with this comment https://github.com/cms-sw/cmssw/pull/37538/files#r848720360 in this PR, or rather in a follow-up?

radla118 · 2022-04-29T13:53:08Z

Hi @radla118 would you like to deal with this comment https://github.com/cms-sw/cmssw/pull/37538/files#r848720360 in this PR, or rather in a follow-up?

Hi @tvami, I would fix the issue in a follow-up PR, since some of the configuration files are also affected

tvami · 2022-04-29T13:57:47Z

Ok, I made a reminder issue about it:
cms-AlCaDB/AlCaTools#61

tvami · 2022-04-29T13:58:36Z

+alca

Jenkins tests pass (including MT version too)
code was tested privately and gives the same results, see Modifying the AlcaPCCIntegrator for processing concurrent lumiblocks #37538 (comment)

makortel · 2022-04-29T14:00:20Z

+core

clacaputo · 2022-05-02T10:03:23Z

+reconstruction

cmsbuild · 2022-05-02T10:03:42Z

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)

perrotta · 2022-05-02T12:29:32Z

Calibration/LumiAlCaRecoProducers/plugins/AlcaPCCIntegrator.cc


+private:
+  edm::EDPutTokenT<reco::PixelClusterCounts> lumiPutToken_;


In the forthcoming PR in which you fix the trackedness of the parameters, please also make these class members const

perrotta · 2022-05-02T12:32:12Z

+1

It is understood that a follow up PR should be prepared, to take into account https://github.com/cms-sw/cmssw/pull/37538/files#r848720360 and Modifying the AlcaPCCIntegrator for processing concurrent lumiblocks #37538 (review)

cmsbuild added this to the CMSSW_12_4_X milestone Apr 12, 2022

cmsbuild added alca-pending code-checks-pending orp-pending pending-signatures tests-pending labels Apr 12, 2022

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

tvami reviewed Apr 12, 2022

View reviewed changes

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

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

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

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

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

cmsbuild added tests-started and removed tests-pending labels Apr 29, 2022

cmsbuild added tests-approved and removed tests-started labels Apr 29, 2022

radla118 marked this pull request as ready for review April 29, 2022 12:10

cmsbuild added alca-approved and removed alca-pending labels Apr 29, 2022

cmsbuild added core-approved and removed core-pending labels Apr 29, 2022

cmsbuild added fully-signed reconstruction-approved and removed reconstruction-pending pending-signatures labels May 2, 2022

perrotta reviewed May 2, 2022

View reviewed changes

cmsbuild added orp-approved and removed orp-pending labels May 2, 2022

cmsbuild merged commit 3cc13ef into cms-sw:master May 2, 2022

This was referenced May 3, 2022

Added IOV synchronization callbacks to ActivityRegistry #37771

Merged

fix use after scope in DMRChecker::bookResidualsHistogram #37796

Merged

This was referenced May 25, 2022

Fix for the parameter set of the AlcaPCCIntegrator module #38077

Merged

[12_3_X] AlcaPCCIntegrator thread safety and parameter set fixes for Run3 #38098

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Modifying the AlcaPCCIntegrator for processing concurrent lumiblocks #37538

Modifying the AlcaPCCIntegrator for processing concurrent lumiblocks #37538

radla118 commented Apr 12, 2022

cmsbuild commented Apr 12, 2022

cmsbuild commented Apr 12, 2022

tvami Apr 12, 2022

tvami commented Apr 12, 2022 •

edited

Loading

tvami commented Apr 12, 2022

tvami commented Apr 12, 2022

mmusich commented Apr 12, 2022

tvami commented Apr 12, 2022

tvami commented Apr 12, 2022

tvami commented Apr 12, 2022

tvami commented Apr 12, 2022

cmsbuild commented Apr 12, 2022

cmsbuild commented Apr 29, 2022

tvami commented Apr 29, 2022

radla118 commented Apr 29, 2022

tvami commented Apr 29, 2022

tvami commented Apr 29, 2022

makortel commented Apr 29, 2022

clacaputo commented May 2, 2022

cmsbuild commented May 2, 2022

perrotta May 2, 2022

perrotta commented May 2, 2022


		private:
		edm::EDPutTokenT<reco::PixelClusterCounts> lumiPutToken_;

Modifying the AlcaPCCIntegrator for processing concurrent lumiblocks #37538

Modifying the AlcaPCCIntegrator for processing concurrent lumiblocks #37538

Conversation

radla118 commented Apr 12, 2022

PR description:

PR validation:

if this PR is a backport please specify the original PR and why you need to backport that PR:

cmsbuild commented Apr 12, 2022

cmsbuild commented Apr 12, 2022

tvami Apr 12, 2022

Choose a reason for hiding this comment

tvami commented Apr 12, 2022 • edited Loading

tvami commented Apr 12, 2022

tvami commented Apr 12, 2022

mmusich commented Apr 12, 2022

tvami commented Apr 12, 2022

tvami commented Apr 12, 2022

tvami commented Apr 12, 2022

tvami commented Apr 12, 2022

cmsbuild commented Apr 12, 2022

cmsbuild commented Apr 29, 2022

Comparison Summary

tvami commented Apr 29, 2022

radla118 commented Apr 29, 2022

tvami commented Apr 29, 2022

tvami commented Apr 29, 2022

makortel commented Apr 29, 2022

clacaputo commented May 2, 2022

cmsbuild commented May 2, 2022

perrotta May 2, 2022

Choose a reason for hiding this comment

perrotta commented May 2, 2022

tvami commented Apr 12, 2022 •

edited

Loading