Allow Portable{Collection,Object}<T, TDev> to be used also independently of ALPAKA_ACCELERATOR_NAMESPACE #43310

makortel · 2023-11-16T22:47:24Z

PR description:

This PR makes the PortableCollection<T, TDev> and PortableObject<T, TDev> alias templates usable also independently of ALPAKA_ACCELERATOR_NAMESPACE. This ability can be useful e.g. for EventSetup data product classes that themselves are templated over the device type (TDev), and want to use the PortableCollection for SoA storage (e.g. having PortableCollection(s) as data members).

Furthermore, this PR simplifies the definitions of the PortableCollection<T, TDev> and PortableObject<T, TDev> alias templates. The trait-based approach is kept, but the traits are fully defined and specialized in DataFormats/Portable/interface/Portable{Collection,Object}.h. In addition, the specializations of the CopyTo{Host,Device} templates were moved to the headers in interface/ as well.

Old description from the RFC-stage of this PR:

The first commit of this PR makes the PortableCollection<T, TDev> alias template usable also independently of ALPAKA_ACCELERATOR_NAMESPACE. This ability can be useful e.g. for EventSetup data product classes that themselves are templated over the device type (TDev), and want to use the PortableCollection for SoA storage (e.g. having PortableCollection(s) as data members). I have an example crafted, but that is waiting for #43298 to get merged, and therefore I went with a standalone PR just for this ability.

While doing that I felt the way the PortableCollection<T, TDev> is defined was a bit cumbersome.

The second commit continues to use the trait-based approach, but the trait is fully defined and specialized in DataFormats/Portable/interface/PortableCollection.h

The third commit uses std::conditional_t in the alias template instead of the trait class template

Some further cleanup can be done, depending on the decided approach. (e.g. in case of second or third commit I'd move the specializations of CopyToHost and CopyToDevice class templates to DataFormats/Portable/interface/PortableCollection.h).

Resolves cms-sw/framework-team#720

PR validation:

Added unit test compiles.

makortel · 2023-11-16T22:48:08Z

@fwyzard Any thoughts?

cmsbuild · 2023-11-16T22:54:34Z

+code-checks

Logs: https://cmssdt.cern.ch/SDT/code-checks/cms-sw-PR-43310/37755

This PR adds an extra 20KB to repository
There are other open Pull requests which might conflict with changes you have proposed:
- File DataFormats/Portable/interface/PortableCollection.h modified in PR(s): Add support to multi layout portable collections (any number) #40285
- File DataFormats/Portable/interface/alpaka/PortableCollection.h modified in PR(s): Add support to multi layout portable collections (any number) #40285, Porting Pixel Tracks to Alpaka [Not to Merge] #41117, Pixel Alpaka Migration: Portable Product [II] #43295

cmsbuild · 2023-11-16T22:54:57Z

A new Pull Request was created by @makortel (Matti Kortelainen) for master.

It involves the following packages:

DataFormats/Portable (heterogeneous)

@cmsbuild, @fwyzard, @makortel can you please review it and eventually sign? Thanks.
@rovere, @missirol this is something you requested to watch as well.
@rappoccio, @antoniovilela, @sextonkennedy you are the release manager for this.

cms-bot commands are listed here

makortel · 2023-11-16T23:22:02Z

@cmsbuild, please test

cmsbuild · 2023-11-17T06:10:05Z

+1

Summary: https://cmssdt.cern.ch/SDT/jenkins-artifacts/pull-request-integration/PR-1d2156/35903/summary.html
COMMIT: e9e99a8
CMSSW: CMSSW_14_0_X_2023-11-16-2300/el8_amd64_gcc12
User test area: For local testing, you can use /cvmfs/cms-ci.cern.ch/week1/cms-sw/cmssw/43310/35903/install.sh to create a dev area with all the needed externals and cmssw changes.

Comparison Summary

Summary:

You potentially removed 302 lines from the logs
Reco comparison results: 136 differences found in the comparisons
DQMHistoTests: Total files compared: 50
DQMHistoTests: Total histograms compared: 3363070
DQMHistoTests: Total failures: 2398
DQMHistoTests: Total nulls: 0
DQMHistoTests: Total successes: 3360650
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-11-17T14:25:13Z

Comparison differences are related to #39803

Log differences are related to #43293

makortel · 2023-12-07T19:17:16Z

@fwyzard Any comments?

fwyzard · 2023-12-11T00:52:53Z

Sorry for the delay, the LUMI hackathon and the CMS Week got me distracted :-/

The first commit of this PR makes the PortableCollection<T, TDev> alias template usable also indepndently of ALPAKA_ACCELERATOR_NAMESPACE.

OK.

A couple of questions:

is there any reason to change class to struct ?
can defining traits::PortableCollectionTrait<T, alpaka::DevCpu> more than once (in DataFormats/Portable/interface/alpaka/PortableCollection.h when compiling for the CPU backend, and now in DataFormats/Portable/interface/PortableHostCollection.h) cause any problems ? Or is it OK to have the same specialisation twice, if they resolve to the same types ?

The second commit continues to use the trait-based approach, but the trait is fully defined and specialized in DataFormats/Portable/interface/PortableCollection.h

OK, this cleans it all up anyway :-)

The third commit uses std::conditional_t in the alias template instead of the trait class template

I like best the style of the second commit, but I don't have any concrete objections to the third one.

It feels like the second may be easier to extend if there are other special cases in the future - but I guess whatever logic that uses, can also be implemented using std::conditional_t.

So, I like two best, but I'm also OK with three.

fwyzard · 2023-12-11T00:53:43Z

By the way, can you do the same also for PortableObject ?

makortel · 2023-12-11T17:34:03Z

is there any reason to change class to struct ?

My personal preference for struct when everything is public (which might be somewhat consistent with Core Guidelines C.2. IIRC the earlier PortableCollectionTrait::CollectionType being private lead to compilation errros.

can defining traits::PortableCollectionTrait<T, alpaka::DevCpu> more than once (in DataFormats/Portable/interface/alpaka/PortableCollection.h when compiling for the CPU backend, and now in DataFormats/Portable/interface/PortableHostCollection.h) cause any problems ? Or is it OK to have the same specialisation twice, if they resolve to the same types ?

I'd think two identical specializations would be technically ok in terms of One Definition Rule. Nevertheless, I think having only one definition would be simplest for future maintenance.

I like best the style of the second commit, but I don't have any concrete objections to the third one.

It feels like the second may be easier to extend if there are other special cases in the future - but I guess whatever logic that uses, can also be implemented using std::conditional_t.

I agree the second commit would be easier to extend. Or, even if such extensions could be done with std::conditional_t, maybe with one additional case the code would still be readable, but I'd imagine with third (and onwards) the pattern matching style with the template specializations to be easier to digest.

On the other hand, with just 2 cases the std::conditional_t feels easier to me.

Can you think of a situation where we'd need (or want to have) another special case? (I'm wondering if there is something foreseeable we might want to account already now)

The pattern for selecting between host and device collections repeats at least in the pixel PRs. I'd like some simple-to-use facility for users to use, and std::conditional_t fits the bill. It seems to me if another special case would be needed in the future, the user code for selecting between the host and device collections would have to be addressed anyway. Perhaps at that point we should consider some further abstraction?

By the way, can you do the same also for PortableObject ?

Yes.

fwyzard · 2023-12-11T17:38:20Z

Can you think of a situation where we'd need (or want to have) another special case? (I'm wondering if there is something foreseeable we might want to account already now)

The two possibilities I had in mind are Unified Memory and oneAPI CPU devices (OpenCL-based).

These might need some special casing of the PortableCollections since they would use memory accessible (or just on) the host -- but I haven't given any concrete thoughts about what that would imply.

makortel · 2023-12-15T20:27:47Z

Can you think of a situation where we'd need (or want to have) another special case? (I'm wondering if there is something foreseeable we might want to account already now)

The two possibilities I had in mind are Unified Memory and oneAPI CPU devices (OpenCL-based).

These might need some special casing of the PortableCollections since they would use memory accessible (or just on) the host -- but I haven't given any concrete thoughts about what that would imply.

After thinking a bit more, I'm starting to lean towards the second commit, i.e. keeping the trait-based approach here, to have a little bit more flexibility for the future. I'd still advocate the std::conditional_t for user code for time being.

…tly of ALPAKA_ACCELERATOR_NAMESPACE

… templates Also move the CopyTo{Host,Device} specialisations to interface/ as they are independent from ALPAKA_ACCELERATOR_NAMESPACE.

makortel · 2023-12-22T18:15:47Z

This update

rebases on top of CMSSW_14_0_0_pre1
keeps the trait-based approach
moves the specializations of CopyTo{Host,Device} to the headers in interface/, as those specializations do not depend on ALPAKA_ACCELERATOR_NAMESPACE

To me this PR is now complete.

cmsbuild · 2023-12-22T18:16:54Z

+code-checks

Logs: https://cmssdt.cern.ch/SDT/code-checks/cms-sw-PR-43310/38291

This PR adds an extra 20KB to repository
There are other open Pull requests which might conflict with changes you have proposed:
- File DataFormats/Portable/interface/PortableCollection.h modified in PR(s): Add support to multi layout portable collections (any number) #40285
- File DataFormats/Portable/interface/alpaka/PortableCollection.h modified in PR(s): Add support to multi layout portable collections (any number) #40285

cmsbuild · 2023-12-22T18:17:13Z

Pull request #43310 was updated. @fwyzard, @makortel, @cmsbuild can you please check and sign again.

makortel · 2023-12-22T18:48:03Z

@cmsbuild, please test

cmsbuild · 2023-12-22T21:36:51Z

+1

Summary: https://cmssdt.cern.ch/SDT/jenkins-artifacts/pull-request-integration/PR-1d2156/36661/summary.html
COMMIT: 1143991
CMSSW: CMSSW_14_0_X_2023-12-22-1100/el8_amd64_gcc12
User test area: For local testing, you can use /cvmfs/cms-ci.cern.ch/week0/cms-sw/cmssw/43310/36661/install.sh to create a dev area with all the needed externals and cmssw changes.

Comparison Summary

Summary:

You potentially added 88 lines to the logs
Reco comparison results: 21 differences found in the comparisons
DQMHistoTests: Total files compared: 48
DQMHistoTests: Total histograms compared: 3247277
DQMHistoTests: Total failures: 1219
DQMHistoTests: Total nulls: 0
DQMHistoTests: Total successes: 3246036
DQMHistoTests: Total skipped: 22
DQMHistoTests: Total Missing objects: 0
DQMHistoSizes: Histogram memory added: 0.0 KiB( 47 files compared)
Checked 200 log files, 161 edm output root files, 48 DQM output files
TriggerResults: no differences found

fwyzard · 2023-12-24T23:44:04Z

+1

cmsbuild · 2023-12-24T23:44:28Z

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

rappoccio · 2024-01-08T20:41:57Z

+1

… it to be used outside of ALPAKA_ACCELERATOR_NAMESPACE I.e. repeat cms-sw#43310 on PortableMultiCollection.

cmsbuild added this to the CMSSW_14_0_X milestone Nov 16, 2023

cmsbuild added pending-signatures tests-pending orp-pending code-checks-pending heterogeneous-pending labels Nov 16, 2023

cmsbuild added code-checks-approved and removed code-checks-pending labels Nov 16, 2023

cmsbuild added tests-started and removed tests-pending labels Nov 16, 2023

cmsbuild added tests-approved and removed tests-started labels Nov 17, 2023

makortel mentioned this pull request Nov 17, 2023

Non reproducibility in wf 136.793 #43293

Open

This was referenced Nov 21, 2023

Porting Pixel Tracks to Alpaka [Not to Merge] #41117

Closed

Pixel Alpaka Migration: Portable Product [II] #43295

Merged

makortel added 2 commits December 22, 2023 19:09

Allow Portable{Collection,Object}<T, TDev> to be used also independen…

c607f8a

…tly of ALPAKA_ACCELERATOR_NAMESPACE

Simplify the definition of Portable{Collection,Object}<T, TDev> alias…

1143991

… templates Also move the CopyTo{Host,Device} specialisations to interface/ as they are independent from ALPAKA_ACCELERATOR_NAMESPACE.

makortel force-pushed the portableCollectionDef branch from e9e99a8 to 1143991 Compare December 22, 2023 18:11

cmsbuild added tests-pending code-checks-pending labels Dec 22, 2023

cmsbuild added code-checks-approved and removed code-checks-pending labels Dec 22, 2023

cmsbuild added tests-started and removed tests-pending labels Dec 22, 2023

cmsbuild added tests-approved and removed tests-started labels Dec 22, 2023

cmsbuild added fully-signed heterogeneous-approved and removed pending-signatures heterogeneous-pending labels Dec 24, 2023

cmsbuild added orp-approved and removed orp-pending labels Jan 8, 2024

cmsbuild merged commit 1b67f09 into cms-sw:master Jan 8, 2024
11 checks passed

makortel deleted the portableCollectionDef branch January 8, 2024 21:47

makortel added a commit to makortel/cmssw that referenced this pull request Feb 15, 2024

Simplify definitions of PortableMultiCollection<TDev, ...>, and allow…

dd0102e

… it to be used outside of ALPAKA_ACCELERATOR_NAMESPACE I.e. repeat cms-sw#43310 on PortableMultiCollection.

makortel added a commit to makortel/cmssw that referenced this pull request Feb 15, 2024

Simplify definitions of PortableMultiCollection<TDev, ...>, and allow…

9dc4cd5

… it to be used outside of ALPAKA_ACCELERATOR_NAMESPACE I.e. repeat cms-sw#43310 on PortableMultiCollection.

makortel mentioned this pull request Feb 15, 2024

Simplify definition of PortableMultiCollection<TDev, ...>, and allow it to be used outside of ALPAKA_ACCELERATOR_NAMESPACE #43983

Merged

makortel added a commit to makortel/cmssw that referenced this pull request Feb 19, 2024

Simplify definitions of PortableMultiCollection<TDev, ...>, and allow…

86e706b

… it to be used outside of ALPAKA_ACCELERATOR_NAMESPACE I.e. repeat cms-sw#43310 on PortableMultiCollection.

makortel mentioned this pull request Feb 19, 2024

[14_0_X] Simplify definition of PortableMultiCollection<TDev, ...>, and allow it to be used outside of ALPAKA_ACCELERATOR_NAMESPACE #44014

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Allow Portable{Collection,Object}<T, TDev> to be used also independently of ALPAKA_ACCELERATOR_NAMESPACE #43310

Allow Portable{Collection,Object}<T, TDev> to be used also independently of ALPAKA_ACCELERATOR_NAMESPACE #43310

makortel commented Nov 16, 2023 •

edited

Loading

makortel commented Nov 16, 2023

cmsbuild commented Nov 16, 2023

cmsbuild commented Nov 16, 2023 •

edited

Loading

makortel commented Nov 16, 2023

cmsbuild commented Nov 17, 2023

makortel commented Nov 17, 2023

makortel commented Dec 7, 2023

fwyzard commented Dec 11, 2023 •

edited

Loading

fwyzard commented Dec 11, 2023

makortel commented Dec 11, 2023

fwyzard commented Dec 11, 2023

makortel commented Dec 15, 2023

makortel commented Dec 22, 2023

cmsbuild commented Dec 22, 2023

cmsbuild commented Dec 22, 2023

makortel commented Dec 22, 2023

cmsbuild commented Dec 22, 2023

fwyzard commented Dec 24, 2023

cmsbuild commented Dec 24, 2023

rappoccio commented Jan 8, 2024

Allow Portable{Collection,Object}<T, TDev> to be used also independently of ALPAKA_ACCELERATOR_NAMESPACE #43310

Allow Portable{Collection,Object}<T, TDev> to be used also independently of ALPAKA_ACCELERATOR_NAMESPACE #43310

Conversation

makortel commented Nov 16, 2023 • edited Loading

PR description:

PR validation:

makortel commented Nov 16, 2023

cmsbuild commented Nov 16, 2023

cmsbuild commented Nov 16, 2023 • edited Loading

makortel commented Nov 16, 2023

cmsbuild commented Nov 17, 2023

Comparison Summary

makortel commented Nov 17, 2023

makortel commented Dec 7, 2023

fwyzard commented Dec 11, 2023 • edited Loading

fwyzard commented Dec 11, 2023

makortel commented Dec 11, 2023

fwyzard commented Dec 11, 2023

makortel commented Dec 15, 2023

makortel commented Dec 22, 2023

cmsbuild commented Dec 22, 2023

cmsbuild commented Dec 22, 2023

makortel commented Dec 22, 2023

cmsbuild commented Dec 22, 2023

Comparison Summary

fwyzard commented Dec 24, 2023

cmsbuild commented Dec 24, 2023

rappoccio commented Jan 8, 2024

makortel commented Nov 16, 2023 •

edited

Loading

cmsbuild commented Nov 16, 2023 •

edited

Loading

fwyzard commented Dec 11, 2023 •

edited

Loading