Add README for Alpaka modules #40690

makortel · 2023-02-03T17:49:12Z

PR description:

This PR adds a README.md documenting the Alpaka module framework integration, and also some recommended practices.

PR validation:

None

cmsbuild · 2023-02-03T17:55:43Z

+code-checks

Logs: https://cmssdt.cern.ch/SDT/code-checks/cms-sw-PR-40690/34047

This PR adds an extra 16KB to repository

cmsbuild · 2023-02-03T17:56:08Z

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

It involves the following packages:

HeterogeneousCore/AlpakaCore (heterogeneous)

@cmsbuild, @makortel, @fwyzard can you please review it and eventually sign? Thanks.
@missirol, @rovere 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

makortel · 2023-02-03T18:00:28Z

HeterogeneousCore/AlpakaCore/README.md

+* Any code that `#include`s a header from the framework or from the `HeterogeneousCore/AlpakaCore`(?) must be separated from the Alpaka device code, and have the usual `.cc` suffix.
+  * A few framework headers are allowed to be used in `.dev.cc` files, e.g. `FWCore/Utilities/interface/Exception.h` or `FWCore/MessageLogger/interface/MessageLogger.h` (?)


In the leading order I'd think all code in HeterogeneousCore/AlpakaCore is about the interaction with the framework, and therefore would not be allowed to be used in .dev.cc files.

In the next-to-leading order, maybe some individual headers could be useful in .dev.cc, like an ability to throw cms::Exception and use MessageLogger, and with limited set of headers the maintenance burden of keeping them usable also by device compilers would be tolerable?

FWCore/Utilities/interface/CMSUnrollLoop.h is OK as it explicitly supports CUDA and ROCm (we could move or copy these macros into Alpaka itself, but even then it would not be a problem if somebody uses this file);

other macro-only headers are also fine (FWCore/Utilities/interface/stringize.h, etc.);

FWCore/Utilities/interface/Exception.h should be OK, as it does not depend on other packages, and as long as the exception are used only in host code, not in device code;

FWCore/MessageLogger might be OK, too; though my preference would be to keep the messages in the .cc files and have only device code and simple wrappers in .dev.cc files.

By the way, EventCache.h and QueueCache.h might be fine in .dev.cc files (even though I don't think we have a use for them in user code); I was thinking to eventually push a reworked version directly into Alpaka.

HeterogeneousCore/AlpakaCore/README.md

makortel · 2023-02-03T18:08:05Z

HeterogeneousCore/AlpakaCore/README.md

+* There must be a host-only flavor of the data format that is either idependent of Alpaka, or depends only on Alpaka's Serial backend
+  * The host-only data format must be defined in `Package/SubPackage/interface/` directory
+  * If the data format is to be serialized (with ROOT), it must be serialized in a way that the on-disk format does not depend on Alpaka, i.e. it can be read without Alpaka
+  * For Event data products the ROOT dictionary should be defined in `Package/SubPackage/src/classes{.h,_def.xml}` (**NOTE**: currently they are in `.../src/alpaka/classes_serial{.h,_def.xml}`


I think it would be better to define the dictionaries for the host-only data format classes in src/classes_def.xml instead of src/alpaka/classes_serial_def.xml, because it would be more consistent with "host platform data formats are usable by non-Alpaka modules". Technically ROOT takes care of loading the proper shared object of the dictionary, so the impact is close to invisible in practice. At least we would be able to avoid building the shared object for the serial backend.

I've been thinking about this as well, and it would also simplify supporting different host back-ends like TBB.
I'm mostly waiting to see if Eric's work with dictionaries can simplify things further, but I'm OK with the move.

To be clear: if you prefer to move them already now, I'm fine with that.

Since the move is not particularly urgent, how about, to avoid further conflicts with #40285, we don't touch the existing classes_serial_def.xml files now, but move them after #40285, and follow the guideline for any other new code that may be PR'd before that?

(although I would be fine with moving them already now as well)

I don't think we will be able to review and merge #40285 this week, as we'll be busy with the tutorial until Friday. If you'd like to move them, do go ahead.

My only request is to base the branch with these changes on top of 3cd90b2, which is itself based on CMSSW_13_0_0_pre4, in order to try and keep a single branch valid for both CMSSW_13_0_X and CMSSW_13_1_X.

Fair enough. Maybe I'll move them then.

makortel · 2023-02-03T18:09:20Z

HeterogeneousCore/AlpakaCore/README.md

+    * The `classes_<backend>_def.xml` should declare the dictionaries for the data product type `T`, `edm::DeviceProduct<T>`, and `edm::Wrapper<edm::DeviceProduct<T>>`. All these dictionaries must be declared as transient with `persistent="false"` attribute.
+    * The list of `<backend>` includes currently: `cuda`


Maybe <platform> would make more sense for data formats than <backend>? (even if technically we have only "backend" concept right now)

<platform> (well, Platform) is already an Alpaka concept, and indeed it seems to be a better match than <backend> here: Alpaka has platforms for "CPU", "CUDA", "HIP" (ROCm), etc., and doesn't differentiate between "CPU with serial backend" and "CPU with TBB parallel backend".

OpenMP 5, OpenACC and SYCL are their own platforms - but, to be fair, they likely require their own memory management.

fwyzard · 2023-02-17T06:13:39Z

HeterogeneousCore/AlpakaCore/README.md

+
+Data formats, for both Event and EventSetup, should be placed following their usual rules. The Alpaka-specific conventions are
+* There must be a host-only flavor of the data format that is either idependent of Alpaka, or depends only on Alpaka's Serial backend
+  * The host-only data format must be defined in `Package/SubPackage/interface/` directory


Suggested change

* The host-only data format must be defined in `Package/SubPackage/interface/` directory

* The host-only data format must be defined in `DataFormats/Package/interface/` directory

?

Technically there are other data format packages than DataFormats (e.g. SimDataFormats). Fully transient data products are also allowed to be defined in packages outside of the *DataFormats (and with looser constraints).

I agree seeing DataFormats explicitly here could be helpful for e.g. newcomers. A possible compromise would be to use DataFormats in these placement guidelines, and after the bullet list add a link to https://twiki.cern.ch/twiki/bin/view/CMSPublic/SWGuideCreatingNewProducts for the exact constraints for data format classes. What do you think?

Technically there are other data format packages than DataFormats (e.g. SimDataFormats).

True, but I don't see us making changes to the simulation DataFormats any time soon...

Fully transient data products are also allowed to be defined in packages outside of the *DataFormats (and with looser constraints)

I admit I'm not familiar with the use cases for transient data formats, other than for testing and CUDADataFormats (and I think we want to get rid of the latter).

A possible compromise would be ...

Sounds good to me.

In the end I used the DataFormats/SubPackage only for the cases that referred to Event data products, and kept the more general Package/SubPackage elsewhere, as only a few EventSetup data format classes are defined in DataFormats.

HeterogeneousCore/AlpakaCore/README.md

missirol

Just typos.

HeterogeneousCore/AlpakaCore/README.md

makortel · 2023-02-20T08:24:02Z

Thanks @fwyzard and @missirol, your suggestions should now be included.

cmsbuild · 2023-02-20T08:28:18Z

+code-checks

Logs: https://cmssdt.cern.ch/SDT/code-checks/cms-sw-PR-40690/34269

This PR adds an extra 16KB to repository

cmsbuild · 2023-02-20T08:28:46Z

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

cmsbuild · 2023-02-22T14:01:39Z

+code-checks

Logs: https://cmssdt.cern.ch/SDT/code-checks/cms-sw-PR-40690/34301

This PR adds an extra 16KB to repository

cmsbuild · 2023-02-22T14:02:01Z

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

makortel · 2023-02-24T15:25:22Z

+heterogeneous

cmsbuild · 2023-02-24T15:25:51Z

This pull request is fully signed and it will be integrated in one of the next master IBs after it passes the integration tests. 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)

makortel · 2023-02-24T15:26:05Z

(since this is just a readme, I thought we could skip tests)

perrotta · 2023-02-24T15:58:25Z

please abort

perrotta · 2023-02-24T15:59:20Z

+1

perrotta · 2023-02-24T16:00:03Z

please abort

perrotta · 2023-02-24T16:00:16Z

merge

cmsbuild added this to the CMSSW_13_0_X milestone Feb 3, 2023

cmsbuild added pending-signatures tests-pending orp-pending code-checks-pending heterogeneous-pending labels Feb 3, 2023

makortel mentioned this pull request Feb 3, 2023

Write a README for the new Alpaka ED/ES module API cms-sw/framework-team#439

Closed

cmsbuild added code-checks-approved and removed code-checks-pending labels Feb 3, 2023

makortel commented Feb 3, 2023

View reviewed changes

makortel mentioned this pull request Feb 6, 2023

Follow-ups to "new Alpaka GPU framework" #40140

Open

20 tasks

smuzaffar modified the milestones: CMSSW_13_0_X, CMSSW_13_1_X Feb 11, 2023

This was referenced Feb 16, 2023

Move host data product dictionaries from classes_serial_def.xml to classers_def.xml cms-sw/framework-team#503

Closed

Move dictionary declarations of Alpaka serial backend data products to the host classes_def.xml #40792

Merged

fwyzard reviewed Feb 17, 2023

View reviewed changes

HeterogeneousCore/AlpakaCore/README.md Outdated Show resolved Hide resolved

fwyzard reviewed Feb 17, 2023

View reviewed changes

HeterogeneousCore/AlpakaCore/README.md Outdated Show resolved Hide resolved

missirol reviewed Feb 19, 2023

View reviewed changes

makortel force-pushed the alpakaREADME branch from b8ca610 to a1067a1 Compare February 20, 2023 08:23

cmsbuild added code-checks-pending and removed code-checks-approved labels Feb 20, 2023

cmsbuild added code-checks-approved and removed code-checks-pending labels Feb 20, 2023

makortel force-pushed the alpakaREADME branch from a1067a1 to d4b3eb8 Compare February 20, 2023 14:17

makortel force-pushed the alpakaREADME branch from d4b3eb8 to e72acff Compare February 22, 2023 13:55

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

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

cmsbuild added fully-signed tests-started heterogeneous-approved and removed pending-signatures tests-pending heterogeneous-pending labels Feb 24, 2023

cmsbuild added tests-pending and removed tests-started labels Feb 24, 2023

cmsbuild added tests-started orp-approved and removed tests-pending orp-pending labels Feb 24, 2023

cmsbuild added tests-pending and removed tests-started labels Feb 24, 2023

cmsbuild merged commit 43bc243 into cms-sw:master Feb 24, 2023

makortel deleted the alpakaREADME branch February 24, 2023 17:00

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add README for Alpaka modules #40690

Add README for Alpaka modules #40690

makortel commented Feb 3, 2023

cmsbuild commented Feb 3, 2023

cmsbuild commented Feb 3, 2023

makortel Feb 3, 2023

fwyzard Feb 7, 2023

fwyzard Feb 7, 2023 •

edited

makortel Feb 3, 2023

fwyzard Feb 7, 2023

fwyzard Feb 7, 2023

makortel Feb 15, 2023

fwyzard Feb 16, 2023

makortel Feb 16, 2023

makortel Feb 3, 2023

fwyzard Feb 7, 2023

fwyzard Feb 17, 2023

makortel Feb 17, 2023

fwyzard Feb 17, 2023

makortel Feb 20, 2023

missirol left a comment

makortel commented Feb 20, 2023

cmsbuild commented Feb 20, 2023

cmsbuild commented Feb 20, 2023

cmsbuild commented Feb 22, 2023

cmsbuild commented Feb 22, 2023

makortel commented Feb 24, 2023

cmsbuild commented Feb 24, 2023

makortel commented Feb 24, 2023

perrotta commented Feb 24, 2023

perrotta commented Feb 24, 2023

perrotta commented Feb 24, 2023

perrotta commented Feb 24, 2023

		* Any code that `#include`s a header from the framework or from the `HeterogeneousCore/AlpakaCore`(?) must be separated from the Alpaka device code, and have the usual `.cc` suffix.
		* A few framework headers are allowed to be used in `.dev.cc` files, e.g. `FWCore/Utilities/interface/Exception.h` or `FWCore/MessageLogger/interface/MessageLogger.h` (?)

		* The `classes_<backend>_def.xml` should declare the dictionaries for the data product type `T`, `edm::DeviceProduct<T>`, and `edm::Wrapper<edm::DeviceProduct<T>>`. All these dictionaries must be declared as transient with `persistent="false"` attribute.
		* The list of `<backend>` includes currently: `cuda`

	* The host-only data format must be defined in `Package/SubPackage/interface/` directory
	* The host-only data format must be defined in `DataFormats/Package/interface/` directory

Add README for Alpaka modules #40690

Add README for Alpaka modules #40690

Conversation

makortel commented Feb 3, 2023

PR description:

PR validation:

cmsbuild commented Feb 3, 2023

cmsbuild commented Feb 3, 2023

Choose a reason for hiding this comment

Choose a reason for hiding this comment

fwyzard Feb 7, 2023 • edited

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

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

missirol left a comment

Choose a reason for hiding this comment

makortel commented Feb 20, 2023

cmsbuild commented Feb 20, 2023

cmsbuild commented Feb 20, 2023

cmsbuild commented Feb 22, 2023

cmsbuild commented Feb 22, 2023

makortel commented Feb 24, 2023

cmsbuild commented Feb 24, 2023

makortel commented Feb 24, 2023

perrotta commented Feb 24, 2023

perrotta commented Feb 24, 2023

perrotta commented Feb 24, 2023

perrotta commented Feb 24, 2023

fwyzard Feb 7, 2023 •

edited