-
Notifications
You must be signed in to change notification settings - Fork 4.3k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Implement AlpakaBackendProducer and AlpakaBackendFilter [14.0.x] #44388
Implement AlpakaBackendProducer and AlpakaBackendFilter [14.0.x] #44388
Conversation
backport #44387 |
enable gpu |
please test |
A new Pull Request was created by @fwyzard for CMSSW_14_0_X. It involves the following packages:
@cmsbuild, @makortel, @fwyzard can you please review it and eventually sign? Thanks. cms-bot commands are listed here
|
cms-bot internal usage |
+1 Summary: https://cmssdt.cern.ch/SDT/jenkins-artifacts/pull-request-integration/PR-a818d9/38096/summary.html Comparison SummarySummary:
GPU Comparison SummarySummary:
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The test doesn't seem to really test the behavior of AlpakaBackendFilter
.
process.source = cms.Source('EmptySource') | ||
|
||
process.load('Configuration.StandardSequences.Accelerators_cff') | ||
process.load('HeterogeneousCore.AlpakaCore.ProcessAcceleratorAlpaka_cfi') |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Strictly speaking this load()
is redundant because ProcessAcceleratorAlpaka_cfi
is already imported in Accelerators_cff
.
What do you mean ? |
process.SelectedBackend = cms.Path(process.alpakaBackendProducer + process.alpakaBackendFilter) | ||
process.AnyOtherBackend = cms.Path(process.alpakaBackendProducer + ~process.alpakaBackendFilter) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The test doesn't seem to really test the behavior of
AlpakaBackendFilter
.What do you mean ?
I mean the filtering decision being the expected one is not being tested. E.g. something along
process.SelectedBackend = cms.Path(process.alpakaBackendProducer + process.alpakaBackendFilter) | |
process.AnyOtherBackend = cms.Path(process.alpakaBackendProducer + ~process.alpakaBackendFilter) | |
process.mustRun = cms.EDProducer("edmtest::MustRunIntProducer", ivalue=cms.int32(1)) | |
process.mustNotRun = cms.EDProducer("FailingProducer") | |
process.SelectedBackend = cms.Path(process.alpakaBackendProducer + process.alpakaBackendFilter + process.mustRun) | |
process.AnyOtherBackend = cms.Path(process.alpakaBackendProducer + ~process.alpakaBackendFilter + process.mustNotRun) |
would lead to the job to fail if the filtering decision (i.e. the joint logic in AlpakaBackendFilter
constructor and filter()
function) would be unexpected for any reason.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ah, I see.
I guess it's enough to have the FailingProducer
?
Is that a real module one can use, or just an example ?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I guess it's enough to have the
FailingProducer
?
Right, in this case where the "other behavior" is achieved with ~
(whose behavior we should be testing in the framework) the first case should indeed be sufficient.
If the other Path would use another instance of AlpakaBackendFilter
with different backend
configuration, then testing it explicitly could be useful.
Is that a real module one can use, or just an example ?
It is available (from test modules defined in FWCore/Framework/test
).
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
OK, thanks, I'll make the changes.
Implement AlpakaBackendProducer, an empty alpaka-based EDProducer whose only purpose is to save in the event what alpaka backend has been used. Implement AlpakaBackendFilter, an EDFilter that selects events based on the alpaka backend used to run a previous producer. Implement a unit test for both modules.
8d6395c
to
6c9a284
Compare
please test |
+1 |
This name is unique, the previous name clashed with AlpakaTest/plugins/BuildFile.xml Co-authored-by: Matti Kortelainen <[email protected]>
please test |
+1 Summary: https://cmssdt.cern.ch/SDT/jenkins-artifacts/pull-request-integration/PR-a818d9/38208/summary.html Comparison SummarySummary:
GPU Comparison SummarySummary:
|
REMINDER @sextonkennedy, @antoniovilela, @rappoccio: This PR was tested with #44440, please check if they should be merged together |
+heterogeneous |
This pull request is fully signed and it will be integrated in one of the next CMSSW_14_0_X IBs (tests are also fine) and once validation in the development release cycle CMSSW_14_1_X is complete. This pull request will now be reviewed by the release team before it's merged. @rappoccio, @antoniovilela, @sextonkennedy (and backports should be raised in the release meeting by the corresponding L2) |
@cms-sw/orp-l2 I think this is ready to be merged. |
+1 |
PR description:
Implement
AlpakaBackendProducer
, an empty alpaka-basedEDProducer
whose only purpose is to save in the event what alpaka backend has been used.Implement
AlpakaBackendFilter
, anEDFilter
that selects events based on the alpaka backend used to run a previous producer.Implement a unit test for both modules.
The aim is to replace this
SwitchProducer
in the HLT menuwith this alpaka-based solution
PR validation:
The new unit test passes.
Backport status
Backport of #44387 to 14.0.x for data taking.