Add an MPISenderPortable and MPIReceiverPortable modules to send/receive arbitrary device collections#50503
Open
ghyls wants to merge 2 commits into
Open
Add an MPISenderPortable and MPIReceiverPortable modules to send/receive arbitrary device collections#50503ghyls wants to merge 2 commits into
MPISenderPortable and MPIReceiverPortable modules to send/receive arbitrary device collections#50503ghyls wants to merge 2 commits into
Conversation
Contributor
|
cms-bot internal usage |
Contributor
|
+code-checks Logs: https://cmssdt.cern.ch/SDT/code-checks/cms-sw-PR-50503/48670
|
This was referenced Mar 31, 2026
Contributor
|
Milestone for this pull request has been moved to CMSSW_17_0_X. Please open a backport if it should also go in to CMSSW_16_1_X. |
This was referenced Apr 7, 2026
Contributor
|
A new Pull Request was created by @ghyls for master. It involves the following packages:
@Dr15Jones, @Moanwar, @civanch, @fwyzard, @jfernan2, @kpedro88, @makortel, @mandrenguyen, @mdhildreth, @smuzaffar, @srimanob can you please review it and eventually sign? Thanks. cms-bot commands are listed here |
ghyls
commented
May 7, 2026
| <use name="HeterogeneousCore/TrivialSerialisation"/> | ||
| <flags ALPAKA_BACKENDS="1"/> | ||
| <flags EDM_PLUGIN="1"/> | ||
| </library> No newline at end of file |
Contributor
Author
There was a problem hiding this comment.
A newline will be added
Contributor
Author
There was a problem hiding this comment.
will be renamed to testGenericClonerPortable_cfg.py
fwyzard
reviewed
May 7, 2026
| bool hasCopyToHost() const override { return HasCopyToHost<T, Queue>; } | ||
|
|
||
| bool hasCopyToDevice() const override { | ||
| if constexpr (HasCopyToHost<T, Queue>) { |
Contributor
There was a problem hiding this comment.
Why do you check for HasCopyToHost before checking for HasCopyToDevice ?
Contributor
There was a problem hiding this comment.
To be followed up in a separate PR, see https://github.com/cms-ngt-hlt/ngt32/issues/46 .
fwyzard
reviewed
May 7, 2026
| } | ||
|
|
||
| std::function<std::shared_ptr<Queue>(edm::WrapperBase const&)> getQueue() const override { | ||
| if constexpr (HasCopyToHost<T, Queue>) { |
Contributor
There was a problem hiding this comment.
Why do you check for HasCopyToHost before returning the queue?
A DeviceProduct<T> can provide the queue even if the underling type does not have a copy-to-host specialisation.
Contributor
Author
There was a problem hiding this comment.
Hmm true, thank you.
getQueue() is only used in the context of registering a D to H transformation, which requires a HasCopyToHost<T> to exist. That is why otherwise I made it return immediately.
However, I see how this makes getQueue misleading. Since this check is already done by preTransformDtoH and transformDtoH I have removed it from getQueue.
fwyzard
reviewed
May 7, 2026
| if (deviceSerialiser) { | ||
| entry.typeID = edm::TypeID{deviceSerialiser->productTypeID()}; | ||
| entry.getToken = | ||
| this->consumes(edm::TypeToGet{entry.typeID, edm::PRODUCT_TYPE}, edm::InputTag{label, instance}); |
Contributor
There was a problem hiding this comment.
Suggested change
| this->consumes(edm::TypeToGet{entry.typeID, edm::PRODUCT_TYPE}, edm::InputTag{label, instance}); | |
| this->consumes(edm::TypeToGet{entry.typeID, edm::PRODUCT_TYPE}, src); |
?
fwyzard
reviewed
May 7, 2026
|
|
||
| if (verbose_) { | ||
| edm::LogInfo("GenericClonerPortable") << "will clone device product of type '" << type << "', label '" | ||
| << label << "', instance '" << instance << "'"; |
Contributor
There was a problem hiding this comment.
Suggested change
| << label << "', instance '" << instance << "'"; | |
| << src.label() << "', instance '" << src.instance() << "'"; |
Contributor
There was a problem hiding this comment.
Actually it may be possible to simply print src, can check if this works
edm::LogInfo("GenericClonerPortable") << "will clone device product of type '" << type << "', tag '" << src << '\'';?
Contributor
Author
There was a problem hiding this comment.
Indeed, it works. Thank you
fwyzard
reviewed
May 7, 2026
| if (hostSerialiser) { | ||
| entry.typeID = edm::TypeID{twd.typeInfo()}; | ||
| entry.getToken = | ||
| this->consumes(edm::TypeToGet{entry.typeID, edm::PRODUCT_TYPE}, edm::InputTag{label, instance}); |
Contributor
There was a problem hiding this comment.
Suggested change
| this->consumes(edm::TypeToGet{entry.typeID, edm::PRODUCT_TYPE}, edm::InputTag{label, instance}); | |
| this->consumes(edm::TypeToGet{entry.typeID, edm::PRODUCT_TYPE}, src); |
fwyzard
reviewed
May 7, 2026
Comment on lines
+184
to
+185
| edm::LogInfo("GenericClonerPortable") << "will clone host product of type '" << type << "', label '" << label | ||
| << "', instance '" << instance << "'"; |
Contributor
There was a problem hiding this comment.
Suggested change
| edm::LogInfo("GenericClonerPortable") << "will clone host product of type '" << type << "', label '" << label | |
| << "', instance '" << instance << "'"; | |
| edm::LogInfo("GenericClonerPortable") << "will clone host product of type '" << type << "', label '" << src.label() | |
| << "', instance '" << src.instance() << "'"; |
fwyzard
reviewed
May 7, 2026
| } | ||
|
|
||
| entry.typeID = edm::TypeID{twd.typeInfo()}; | ||
| entry.getToken = this->consumes(edm::TypeToGet{entry.typeID, edm::PRODUCT_TYPE}, edm::InputTag{label, instance}); |
Contributor
There was a problem hiding this comment.
Suggested change
| entry.getToken = this->consumes(edm::TypeToGet{entry.typeID, edm::PRODUCT_TYPE}, edm::InputTag{label, instance}); | |
| entry.getToken = this->consumes(edm::TypeToGet{entry.typeID, edm::PRODUCT_TYPE}, src); |
fwyzard
reviewed
May 7, 2026
Comment on lines
+210
to
+211
| edm::LogInfo("GenericClonerPortable") << "will clone ROOT-serialised product of type '" << type << "', label '" | ||
| << label << "', instance '" << instance << "'"; |
Contributor
There was a problem hiding this comment.
Suggested change
| edm::LogInfo("GenericClonerPortable") << "will clone ROOT-serialised product of type '" << type << "', label '" | |
| << label << "', instance '" << instance << "'"; | |
| edm::LogInfo("GenericClonerPortable") << "will clone ROOT-serialised product of type '" << type << "', label '" | |
| << src.label() << "', instance '" << src.instance() << "'"; |
fwyzard
reviewed
May 7, 2026
| [copyAsync = std::forward<TCopyAsync>(copyAsync), synchronize = this->synchronize()]( | ||
| edm::StreamID streamID, edm::WrapperBase const& wb, edm::WaitingTaskWithArenaHolder holder) -> std::any { | ||
| detail::EDMetadataAcquireSentry sentry(streamID, std::move(holder), synchronize); | ||
| auto productOnHost = copyAsync(sentry.metadata()->queue(), wb); |
Contributor
There was a problem hiding this comment.
isn't this productOnDevice ?
Contributor
Author
There was a problem hiding this comment.
Sorry for not having noticed this...
It is fixed now.
cmsbuild
added
tests-pending
code-checks-pending
and removed
tests-started
code-checks-approved
labels
Contributor
Contributor
|
Pull request #50503 was updated. @Dr15Jones, @Moanwar, @civanch, @cmsbuild, @fwyzard, @jfernan2, @kpedro88, @makortel, @mandrenguyen, @mdhildreth, @smuzaffar, @srimanob can you please check and sign again. |
Contributor
|
allow @ghyls test rights |
makortel
reviewed
May 15, 2026
Contributor
makortel
left a comment
makortel
left a comment
There was a problem hiding this comment.
Few things that caught my eye
| std::string productInstance) { | ||
| TransformerBase::registerTransformAsyncImp( | ||
| *this, iToken, returnType, std::move(productInstance), std::move(iPre), std::move(iF)); | ||
| } |
Contributor
There was a problem hiding this comment.
Please extend this overload to the other module base classes that provide Transformer (that is, global and limited).
The real test suite for the Transformer is in
cmssw/FWCore/Integration/test/BuildFile.xml
Lines 60 to 78 in bd607fa
Could you extend those to cover this overload as well?
Comment on lines
+129
to
+133
| edm::EDPutToken produces(edm::TypeID deviceProductType, | ||
| edm::TypeID hostProductType, | ||
| std::string instanceName, | ||
| TCopyAsync&& copyAsync, | ||
| TTransform&& transform) { |
Contributor
There was a problem hiding this comment.
I suppose this produces() overload can be called only when the copy operation exists for the host-to-device copy (which is different from the typed produces()), and the present API does not support producing a type-erased host-only data product. I'd suggest to add a comment about that.
Comment on lines
+129
to
+130
| edm::EDPutToken produces(edm::TypeID deviceProductType, | ||
| edm::TypeID hostProductType, |
Contributor
There was a problem hiding this comment.
My personal preference would be to have the primarily produced type first, and the implicitly copied type second
Suggested change
| edm::EDPutToken produces(edm::TypeID deviceProductType, | |
| edm::TypeID hostProductType, | |
| edm::EDPutToken produces(edm::TypeID hostProductType, | |
| edm::TypeID deviceProductType, |
| TTransform&& transform) { | ||
| edm::EDPutToken token = this->producesCollector().template produces<Tr>(hostProductType, instanceName); | ||
|
|
||
| if constexpr (not detail::useProductDirectly) { |
Contributor
There was a problem hiding this comment.
In the detail::useProductDirectly == true case, should deviceProductType == hostProductType?
| std::string instanceName, | ||
| TCopyAsync&& copyAsync, | ||
| TTransform&& transform) { | ||
| edm::EDPutToken token = this->producesCollector().template produces<Tr>(hostProductType, instanceName); |
Contributor
There was a problem hiding this comment.
Why go through producesCollector() instead of
Suggested change
| edm::EDPutToken token = this->producesCollector().template produces<Tr>(hostProductType, instanceName); | |
| edm::EDPutToken token = Base::template produces<Tr>(hostProductType, instanceName); |
?
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
4 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:
These PR includes two separate developments, one of which requires the other.
Enable the registration at runtime of DtoH and HtoD product transformations:
Add an
MPISenderPortableandMPIReceiverPortablemodules to send/receive device collectionsMPISenderPortable.ccandMPIReceiverPortable.cc, which can send/receive device runtime-typed device collections for which a device TrivialSerialiser plugin exists, directly to/from device memory.PR validation:
A
GenericClonerPortabletest module is introduced to demonstrate the D to H and H to D transformation registrations at runtime. The module clones a host or a device product and registers the H to D (or D to H) transformation for it. The test is configured viatestGenericClonerDevice.pyA small test is added to
FWCore/Framework/test/stream_producer_catch2.ccto test the non-templatedregisterTransformAsyncoverload added toFramework/interface/stream/implementors.h.The Portable MPI modules (
MPISenderPortable.ccandMPIReceiverPortable.cc) are tested via the two new configurations added toHeterogeneousCore/MPICore/test.Backport
16_0_Xand16_1_Xfor it to be used in the NGT demonstrator during data taking this year.