Add ECAL portable data formats, collections and conditions for alpaka #42930

thomreis · 2023-10-02T15:35:48Z

PR description:

This PR is the first in a planned series of three to migrate the ECAL local reconstruction for Run 3 and Phase 2 to alpaka.
It adds the conditions formats and portable collections as well as the data formats and portable collections.

Conditions:

Multifit conditions
Electronics mapping

SoA data formats:

Digis
Digis Phase 2
Uncalibrated RecHits

All developments of @valsdav , @Jakub-Gajownik , and @thomreis have been squashed into one commit for clarity.

Since this PR just adds collections for future use in the coming commits no changes are expected from this PR.

PRs 2 and 3 will be made to add the Phase 2 local reconstruction code and multifit algorithm migrations, respectively.

PR validation:

Passes GPU WFs 12434.512 and 24834.612

thomreis · 2023-10-02T15:36:21Z

type ecal

thomreis · 2023-10-02T15:36:32Z

enable gpu

cmsbuild · 2023-10-02T15:47:57Z

+code-checks

Logs: https://cmssdt.cern.ch/SDT/code-checks/cms-sw-PR-42930/37074

This PR adds an extra 60KB to repository
There are other open Pull requests which might conflict with changes you have proposed:
- File DataFormats/EcalDigi/src/classes_def.xml modified in PR(s): Phase2 Trigger Primitive generation for Ecal Barrel #42633

cmsbuild · 2023-10-02T15:48:27Z

A new Pull Request was created by @thomreis (Thomas Reis) for master.

It involves the following packages:

CondFormats/DataRecord (db, alca)
CondFormats/EcalObjects (db, alca)
DataFormats/EcalDigi (simulation)
DataFormats/EcalRecHit (reconstruction)

@mandrenguyen, @civanch, @jfernan2, @cmsbuild, @mdhildreth, @francescobrivio, @saumyaphor4252, @perrotta, @consuegs can you please review it and eventually sign? Thanks.
@tocheng, @missirol, @mmusich, @seemasharmafnal, @rovere, @rchatter, @wang0jin, @thomreis, @argiro this is something you requested to watch as well.
@antoniovilela, @sextonkennedy, @rappoccio you are the release manager for this.

cms-bot commands are listed here

thomreis · 2023-10-02T15:50:28Z

please test

mmusich · 2023-10-02T16:02:32Z

assign heterogeneous

looks missing (might be wrong though)

cmsbuild · 2023-10-02T16:02:57Z

New categories assigned: heterogeneous

@fwyzard,@makortel you have been requested to review this Pull request/Issue and eventually sign? Thanks

makortel · 2023-10-02T18:26:42Z

CondFormats/EcalObjects/BuildFile.xml

 <use name="HeterogeneousCore/CUDACore"/>
 <use name="HeterogeneousCore/CUDAUtilities"/>
 <use name="boost"/>
 <use name="boost_serialization"/>
 <use name="rootmath"/>
 <use name="clhep"/>
 <use name="cuda"/>
+<use name="HeterogeneousCore/AlpakaCore"/>


This dependence should not be needed in data format package.

makortel · 2023-10-02T18:27:23Z

CondFormats/EcalObjects/BuildFile.xml

 <use name="HeterogeneousCore/CUDACore"/>
 <use name="HeterogeneousCore/CUDAUtilities"/>
 <use name="boost"/>
 <use name="boost_serialization"/>
 <use name="rootmath"/>
 <use name="clhep"/>
 <use name="cuda"/>
+<use name="HeterogeneousCore/AlpakaCore"/>
+<use name="HeterogeneousCore/AlpakaInterface"/>
+<flags ALPAKA_BACKENDS="1"/>


This should be

Suggested change

<flags ALPAKA_BACKENDS="1"/>

<flags ALPAKA_BACKENDS="cuda rocm"/>

or (in a recent IB)

Suggested change

<flags ALPAKA_BACKENDS="1"/>

<flags ALPAKA_BACKENDS="!serial"/>

Rebased to yesterday's IB and implemented the second option.

makortel · 2023-10-02T18:28:26Z

DataFormats/EcalDigi/BuildFile.xml

 <use name="FWCore/MessageLogger"/>
 <use name="FWCore/Utilities"/>
+<use name="HeterogeneousCore/AlpakaInterface"/>
+<flags ALPAKA_BACKENDS="cuda rocm"/>


In a recent IB could be also

Suggested change

<flags ALPAKA_BACKENDS="cuda rocm"/>

<flags ALPAKA_BACKENDS="!serial"/>

makortel · 2023-10-02T18:37:24Z

DataFormats/EcalDigi/interface/EcalDigiPhase2SoA.h

+
+  GENERATE_SOA_LAYOUT(EcalDigiPhase2SoALayout,
+                      SOA_COLUMN(uint32_t, id),
+                      SOA_COLUMN(DataArrayPhase2Struct, data),


The sampleSize seems to be 16 (phase1) or 10 (phase2) So this column has 16-or-10 2-byte sub-elements per SoA-element (i.e. 32-or-20 bytes).

How is this data structure accessed by the consuming code? Does the consuming code use a GPU-thread per element of this SoA? If yes, then I'm wondering if it would be more performant to split this column into sampleSize arrays with Eigen vectors? In principle that should yield memory access pattern that is more coalesced than this one.

(as far as I'm concerned this optimization can be done later, it would anyway be better to study the impact on the performance rather than to rely on anything I wrote above on the top of my head)

(this pattern repeats in other SoAs as well, I'm not commenting on them in order to have one place to discuss about it)

It should be the other way around, in fact. 10 elements for Phase 1 and 16 for Phase 2.
At the moment one thread will use all 10 or 16 elements to calculate the weighted sum in the Phase 2 case.
I am not sure how the fitting in the multifit algorithm works but I think if also uses all samples to perform the fit in one thread.

makortel · 2023-10-02T18:39:10Z

DataFormats/EcalDigi/src/classes_def.xml

+     targetClass="ecal::DigiHostCollection"
+     version="[1-]"
+     source="ecal::EcalDigiSoA layout_;"
+     target="buffer_"


This should be

Suggested change

target="buffer_"

target="buffer_,layout_,view_"

makortel · 2023-10-02T18:39:34Z

DataFormats/EcalDigi/src/classes_def.xml

+     targetClass="ecal::DigiPhase2HostCollection"
+     version="[1-]"
+     source="ecal::EcalDigiPhase2SoA layout_;"
+     target="buffer_"


This should be

Suggested change

target="buffer_"

target="buffer_,layout_,view_"

makortel · 2023-10-02T18:41:03Z

DataFormats/EcalRecHit/src/classes_def.xml

+    targetClass="ecal::UncalibratedRecHitHostCollection"
+    version="[1-]"
+    source="ecal::EcalUncalibratedRecHitSoA layout_;"
+    target="buffer_"


This should be

Suggested change

target="buffer_"

target="buffer_,layout_,view_"

cmsbuild · 2023-10-02T19:52:36Z

-1

Failed Tests: HeaderConsistency UnitTests RelVals-GPU
Summary: https://cmssdt.cern.ch/SDT/jenkins-artifacts/pull-request-integration/PR-a3482e/35011/summary.html
COMMIT: a9d393c
CMSSW: CMSSW_13_3_X_2023-10-02-1100/el8_amd64_gcc11
Additional Tests: GPU
User test area: For local testing, you can use /cvmfs/cms-ci.cern.ch/week1/cms-sw/cmssw/42930/35011/install.sh to create a dev area with all the needed externals and cmssw changes.

Unit Tests

I found 1 errors in the following unit tests:

---> test CondToolsLHCInfoNewPopConTest had ERRORS

RelVals-GPU

12434.58712434.587_TTbar_14TeV+2023_Patatrack_AllTripletsGPU_Validation/step2_TTbar_14TeV+2023_Patatrack_AllTripletsGPU_Validation.log
12434.58612434.586_TTbar_14TeV+2023_Patatrack_AllTripletsGPU/step2_TTbar_14TeV+2023_Patatrack_AllTripletsGPU.log

Comparison Summary

Summary:

You potentially added 6 lines to the logs
Reco comparison results: 8 differences found in the comparisons
DQMHistoTests: Total files compared: 50
DQMHistoTests: Total histograms compared: 3358320
DQMHistoTests: Total failures: 6
DQMHistoTests: Total nulls: 0
DQMHistoTests: Total successes: 3358292
DQMHistoTests: Total skipped: 22
DQMHistoTests: Total Missing objects: 0
DQMHistoSizes: Histogram memory added: 0.0 KiB( 49 files compared)
Checked 214 log files, 167 edm output root files, 50 DQM output files
TriggerResults: no differences found

makortel · 2023-10-02T20:00:57Z

RelVals-GPU

12434.58712434.587_TTbar_14TeV+2023_Patatrack_AllTripletsGPU_Validation/step2_TTbar_14TeV+2023_Patatrack_AllTripletsGPU_Validation.log

12434.58612434.586_TTbar_14TeV+2023_Patatrack_AllTripletsGPU/step2_TTbar_14TeV+2023_Patatrack_AllTripletsGPU.log

These failure should be fixed in the next IB that includes #42916

cmsbuild · 2023-10-24T19:52:14Z

+1

Summary: https://cmssdt.cern.ch/SDT/jenkins-artifacts/pull-request-integration/PR-a3482e/35386/summary.html
COMMIT: 438a156
CMSSW: CMSSW_13_3_X_2023-10-24-1100/el8_amd64_gcc12
Additional Tests: GPU
User test area: For local testing, you can use /cvmfs/cms-ci.cern.ch/week0/cms-sw/cmssw/42930/35386/install.sh to create a dev area with all the needed externals and cmssw changes.

Comparison Summary

Summary:

You potentially removed 1 lines from the logs
Reco comparison results: 26 differences found in the comparisons
DQMHistoTests: Total files compared: 50
DQMHistoTests: Total histograms compared: 3357400
DQMHistoTests: Total failures: 1070
DQMHistoTests: Total nulls: 0
DQMHistoTests: Total successes: 3356308
DQMHistoTests: Total skipped: 22
DQMHistoTests: Total Missing objects: 0
DQMHistoSizes: Histogram memory added: 0.0 KiB( 49 files compared)
Checked 214 log files, 167 edm output root files, 50 DQM output files
TriggerResults: no differences found

GPU Comparison Summary

Summary:

No significant changes to the logs found
Reco comparison results: 1 differences found in the comparisons
DQMHistoTests: Total files compared: 3
DQMHistoTests: Total histograms compared: 39740
DQMHistoTests: Total failures: 420
DQMHistoTests: Total nulls: 0
DQMHistoTests: Total successes: 39320
DQMHistoTests: Total skipped: 0
DQMHistoTests: Total Missing objects: 0
DQMHistoSizes: Histogram memory added: 0.0 KiB( 2 files compared)
Checked 8 log files, 10 edm output root files, 3 DQM output files
TriggerResults: no differences found

mmusich · 2023-10-24T20:06:34Z

@perrotta

As you point out, persisted structures with fixed sizes may give problems in schema evolution if we decide at some point to modify that size. I agree that transient ones can be treated with more liberty. Nonetheless, my point is also that if we hard-code the size parameter of one structure which is defined in another class and package, then we can get problems nonetheless when we try to modify it in one place and need to propagate that change elsewhere. If the fixed size is defined centrally in the class that introduces it, instead, it can be picked everywhere else, and it has to get modified only once in case of need.

I was answering to the other Andrea and not to your review.

thomreis · 2023-10-24T20:18:42Z

These are anyway not persisted, right? IIRC fixed-size containers persisted in the database cannot be schema-evolved (as of today) which can be problematic if this has to be changed down the line, but transient structures in memory can be whatever is most convenient.

As you point out, persisted structures with fixed sizes may give problems in schema evolution if we decide at some point to modify that size. I agree that transient ones can be treated with more liberty. Nonetheless, my point is also that if we hard-code the size parameter of one structure which is defined in another class and package, then we can get problems nonetheless when we try to modify it in one place and need to propagate that change elsewhere. If the fixed size is defined centrally in the class that introduces it, instead, it can be picked everywhere else, and it has to get modified only once in case of need.

Is this not the case here? The array sizes are defined in the conditions format SoA definitions and will be used in the other packages to do operations with the conditions. For example in the ESProducer inside the RecoLocalCalo/EcalRecProducers package to initialte a memcpy: https://github.com/thomreis/cmssw/blob/ecal-reco-gpu-alpaka-migration-multifit-133x/RecoLocalCalo/EcalRecProducers/plugins/alpaka/EcalMultifitConditionsHostESProducer.cc#L142
Or is there a better place where the array sizes could be defined?

fwyzard · 2023-10-25T14:31:14Z

+heterogeneous

perrotta · 2023-10-26T09:13:27Z

+1

thomreis · 2023-10-26T09:25:16Z

Hi @cms-sw/simulation-l2 @cms-sw/reconstruction-l2 do you have any more comments on this PR?

civanch · 2023-10-26T15:53:48Z

+1

thomreis · 2023-10-30T19:47:40Z

Hi @cms-sw/reconstruction-l2 do you have any further comments?

mandrenguyen · 2023-10-31T02:57:48Z

+1

cmsbuild · 2023-10-31T02:58:18Z

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

antoniovilela · 2023-11-01T13:35:01Z

+1

cmsbuild added this to the CMSSW_13_3_X milestone Oct 2, 2023

cmsbuild added reconstruction-pending simulation-pending alca-pending db-pending pending-signatures tests-pending orp-pending code-checks-pending labels Oct 2, 2023

cmsbuild added the ecal label Oct 2, 2023

cmsbuild added code-checks-approved and removed code-checks-pending labels Oct 2, 2023

cmsbuild added tests-started and removed tests-pending labels Oct 2, 2023

cmsbuild added the heterogeneous-pending label Oct 2, 2023

makortel reviewed Oct 2, 2023

View reviewed changes

cmsbuild added tests-rejected and removed tests-started labels Oct 2, 2023

thomreis force-pushed the ecal-reco-gpu-alpaka-migration-integration-133x branch from a9d393c to 5756e0f Compare October 3, 2023 09:23

cmsbuild removed tests-rejected code-checks-approved labels Oct 3, 2023

cmsbuild added tests-approved and removed tests-started labels Oct 24, 2023

cmsbuild added heterogeneous-approved and removed heterogeneous-pending labels Oct 25, 2023

cmsbuild added alca-approved db-approved and removed alca-pending db-pending labels Oct 26, 2023

cmsbuild added simulation-approved and removed simulation-pending labels Oct 26, 2023

cmsbuild added reconstruction-approved fully-signed and removed reconstruction-pending pending-signatures labels Oct 31, 2023

cmsbuild added orp-approved and removed orp-pending labels Nov 1, 2023

cmsbuild merged commit d746f22 into cms-sw:master Nov 1, 2023

thomreis mentioned this pull request Nov 13, 2023

ECAL unpacker and ECAL multifit algorithm migration to alpaka #43257

Merged

thomreis deleted the ecal-reco-gpu-alpaka-migration-integration-133x branch February 2, 2024 14:54

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add ECAL portable data formats, collections and conditions for alpaka #42930

Add ECAL portable data formats, collections and conditions for alpaka #42930

thomreis commented Oct 2, 2023

thomreis commented Oct 2, 2023

thomreis commented Oct 2, 2023

cmsbuild commented Oct 2, 2023

cmsbuild commented Oct 2, 2023

thomreis commented Oct 2, 2023

mmusich commented Oct 2, 2023

cmsbuild commented Oct 2, 2023

makortel Oct 2, 2023

makortel Oct 2, 2023

thomreis Oct 3, 2023

makortel Oct 2, 2023

makortel Oct 2, 2023

thomreis Oct 3, 2023

makortel Oct 2, 2023

makortel Oct 2, 2023

makortel Oct 2, 2023

cmsbuild commented Oct 2, 2023

makortel commented Oct 2, 2023

RelVals-GPU

cmsbuild commented Oct 24, 2023

mmusich commented Oct 24, 2023

thomreis commented Oct 24, 2023

fwyzard commented Oct 25, 2023

perrotta commented Oct 26, 2023

thomreis commented Oct 26, 2023

civanch commented Oct 26, 2023

thomreis commented Oct 30, 2023

mandrenguyen commented Oct 31, 2023

cmsbuild commented Oct 31, 2023

antoniovilela commented Nov 1, 2023

	<flags ALPAKA_BACKENDS="1"/>
	<flags ALPAKA_BACKENDS="cuda rocm"/>

	<flags ALPAKA_BACKENDS="1"/>
	<flags ALPAKA_BACKENDS="!serial"/>

Add ECAL portable data formats, collections and conditions for alpaka #42930

Add ECAL portable data formats, collections and conditions for alpaka #42930

Conversation

thomreis commented Oct 2, 2023

PR description:

PR validation:

thomreis commented Oct 2, 2023

thomreis commented Oct 2, 2023

cmsbuild commented Oct 2, 2023

cmsbuild commented Oct 2, 2023

thomreis commented Oct 2, 2023

mmusich commented Oct 2, 2023

cmsbuild commented Oct 2, 2023

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

cmsbuild commented Oct 2, 2023

Unit Tests

RelVals-GPU

Comparison Summary

makortel commented Oct 2, 2023

RelVals-GPU

cmsbuild commented Oct 24, 2023

Comparison Summary

GPU Comparison Summary

mmusich commented Oct 24, 2023

thomreis commented Oct 24, 2023

fwyzard commented Oct 25, 2023

perrotta commented Oct 26, 2023

thomreis commented Oct 26, 2023

civanch commented Oct 26, 2023

thomreis commented Oct 30, 2023

mandrenguyen commented Oct 31, 2023

cmsbuild commented Oct 31, 2023

antoniovilela commented Nov 1, 2023