Pixel Alpaka Migration: Portable Product [II]#43295
Merged
cmsbuild merged 5 commits intocms-sw:masterfrom Nov 24, 2023
Merged
Conversation
AdrianoDee
changed the title
AdrianoDee
changed the title
This was referenced Nov 15, 2023
Contributor
|
+code-checks Logs: https://cmssdt.cern.ch/SDT/code-checks/cms-sw-PR-43295/37711
|
Contributor
|
A new Pull Request was created by @AdrianoDee (Adriano Di Florio) for master. It involves the following packages:
@cmsbuild, @fwyzard, @makortel can you please review it and eventually sign? Thanks. cms-bot commands are listed here |
cmsbuild
added
reconstruction-pending
code-checks-pending
and removed
code-checks-approved
labels
Contributor
|
+code-checks Logs: https://cmssdt.cern.ch/SDT/code-checks/cms-sw-PR-43295/37712
|
Contributor
makortel
reviewed
Nov 15, 2023
DataFormats/BeamSpot/BuildFile.xml
Outdated
| <use name="clhep"/> | ||
| <use name="DataFormats/Portable"/> | ||
| <use name="HeterogeneousCore/AlpakaInterface"/> | ||
| <flags ALPAKA_BACKENDS="cuda rocm"/> |
Contributor
There was a problem hiding this comment.
Suggested change
| <flags ALPAKA_BACKENDS="cuda rocm"/> | |
| <flags ALPAKA_BACKENDS="!serial"/> |
is preferred now
Comment on lines
8
to
9
| <class name="BeamSpotPOD" ClassVersion="10"> | ||
| <version ClassVersion="10" checksum="280341519"/> |
Contributor
There was a problem hiding this comment.
New class versions should start with 3
Suggested change
| <class name="BeamSpotPOD" ClassVersion="10"> | |
| <version ClassVersion="10" checksum="280341519"/> | |
| <class name="BeamSpotPOD" ClassVersion="3"> | |
| <version ClassVersion="3" checksum="280341519"/> |
| #include "HeterogeneousCore/AlpakaInterface/interface/config.h" | ||
| #include "HeterogeneousCore/AlpakaInterface/interface/memory.h" | ||
|
|
||
| // generic SoA-based product in device memory |
Contributor
There was a problem hiding this comment.
I don't see any relation to SoA. The class stores an object of type T in an Alpaka buffer.
|
|
||
| // generic SoA-based product in device memory | ||
| template <typename T, typename TDev, typename = std::enable_if_t<alpaka::isDevice<TDev>>> | ||
| class PortableDeviceProduct { |
Contributor
There was a problem hiding this comment.
What is the added value of this class compared to plain Alpaka buffer?
I can think of
- can be default-constructed
- handles the data format
const-requirements of the framework correctly
Are these correct? Is something missing?
(I'm mostly trying to understand the situation better, e.g. if we could eventually find a way without this level of indirection)
Contributor
There was a problem hiding this comment.
How about PortableDeviceObject as the name? The name "product" is quite specific to how framework handles "data products", whereas this class seems to address a bit different concern. Also edm::DeviceProduct<PortableDeviceProduct<Foo>> looks repetitive with the name "Product".
Contributor
Author
There was a problem hiding this comment.
I can think of
- can be default-constructed
- handles the data format const-requirements of the framework correctly
Are these correct? Is something missing?
I think this were the two main points at the time (@borzari correct me if I'm wrong). And yes, I agree PortableDeviceObject would work better. The same for the host counterpart.
Contributor
Author
There was a problem hiding this comment.
And for the "general one" would go with PortableObject instead of PortableProduct.
| #include "HeterogeneousCore/AlpakaInterface/interface/host.h" | ||
| #include "HeterogeneousCore/AlpakaInterface/interface/memory.h" | ||
|
|
||
| // generic SoA-based product in host memory |
Contributor
There was a problem hiding this comment.
Seems to not have anything to do with SoA either.
| // part of the ROOT read streamer | ||
| static void ROOTReadStreamer(PortableHostProduct* newObj, Product& product) { | ||
| std::cerr << "ROOT object at " << &product << std::endl; | ||
| std::cerr << "id: " << product.id << std::endl; |
Contributor
There was a problem hiding this comment.
Printouts need to be removed
| Product const* operator->() const { return buffer_->data(); } | ||
|
|
||
| // access the buffer | ||
| Buffer buffer() { return *buffer_; } |
Contributor
There was a problem hiding this comment.
Could this be
Suggested change
| Buffer buffer() { return *buffer_; } | |
| Buffer& buffer() { return *buffer_; } |
(although I see the same pattern in PortableDeviceCollection)
Contributor
There was a problem hiding this comment.
No, it really should be a copy.
Otherwise one could modify the internals of the buffer.
|
|
||
| // generic SoA-based product in host memory | ||
| template <typename T> | ||
| class PortableHostProduct { |
Contributor
There was a problem hiding this comment.
How about PortableHostObject?
| ConstBuffer const_buffer() const { return *buffer_; } | ||
|
|
||
| // part of the ROOT read streamer | ||
| static void ROOTReadStreamer(PortableHostProduct* newObj, Product& product) { |
Contributor
There was a problem hiding this comment.
Do I understand correctly this function is not currently called? Could you add a test where it is used?
9 tasks
Contributor
|
+code-checks Logs: https://cmssdt.cern.ch/SDT/code-checks/cms-sw-PR-43295/37732
|
Contributor
|
enable gpu |
Contributor
|
please test |
Contributor
|
+heterogeneous |
cmsbuild
added
tests-started
heterogeneous-approved
and removed
tests-pending
heterogeneous-pending
labels
Contributor
|
+code-checks Logs: https://cmssdt.cern.ch/SDT/code-checks/cms-sw-PR-43295/37821
|
Contributor
|
Pull request #43295 was updated. @jfernan2, @mandrenguyen can you please check and sign again. |
Contributor
|
+1 Summary: https://cmssdt.cern.ch/SDT/jenkins-artifacts/pull-request-integration/PR-a88f5d/35992/summary.html Comparison SummarySummary:
GPU Comparison SummarySummary:
|
Contributor
|
The tests are complaining about a namespace in the header files: |
Contributor
The which is fine (because nothing is added to the global namespace) |
Contributor
|
@cms-sw/reconstruction-l2 @makortel |
Contributor
|
+reconstruction |
cmsbuild
added
reconstruction-approved
fully-signed
and removed
reconstruction-pending
pending-signatures
labels
Contributor
|
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. @antoniovilela, @sextonkennedy, @rappoccio (and backports should be raised in the release meeting by the corresponding L2) |
Contributor
|
+1 |
2 tasks
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
7 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
PR description:
This PR stems from #41117 and it's the 2nd of a series of smaller PRs:
It includes the
PortableProductobject and a couple of tests.Keeping track of the comments in PixelTracksAlpaka#40.