-
Notifications
You must be signed in to change notification settings - Fork 4.4k
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
Fix implicit host-to-device copy for ES data products with labels in Alpaka ESProducers #41019
Conversation
…Alpaka ESProducers Taking the "appendToDataLabel" and module label from the module's ParameterSet was, by far, the easiest way to fix, supported by the hindsight that having all modules to pass the ParameterSet to the base class' constructor would have made some features much easier and cleaner to develop.
+code-checks Logs: https://cmssdt.cern.ch/SDT/code-checks/cms-sw-PR-41019/34541
|
A new Pull Request was created by @makortel (Matti Kortelainen) for master. It involves the following packages:
@cmsbuild, @makortel, @fwyzard can you please review it and eventually sign? Thanks. cms-bot commands are listed here |
enable gpu |
@cmsbuild, please test |
+1 Summary: https://cmssdt.cern.ch/SDT/jenkins-artifacts/pull-request-integration/PR-d47084/31177/summary.html Comparison SummarySummary:
GPU Comparison SummarySummary:
|
Hmh, many phase 2 workflows show differences in some JetMET histograms. |
@cmsbuild, please test Let's see if they persist |
(Just for the record, I saw something similar yesterday, #41016 (comment).) |
+1 Summary: https://cmssdt.cern.ch/SDT/jenkins-artifacts/pull-request-integration/PR-d47084/31214/summary.html Comparison SummarySummary:
GPU Comparison SummarySummary:
|
The JetMET difference didn't repeat, but the latest incarnation of #39803 came visible. |
@fwyzard Do you have any comments? |
I trust you on this :-) |
+heterogeneous |
type bug-fix |
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. @perrotta, @dpiparo, @rappoccio (and backports should be raised in the release meeting by the corresponding L2) |
+1 |
PR description:
It was noticed during the Patatrack hackathon that the implicit host-to-device copy of EventSetup data products did not work correctly in Alpaka ESProducers when the data product had a label set (either via explicit argument to
setWhatProduced()
or via theappendToDataLabel
configuration parameter). The test code that I added in #40813 turned out to have a flaw, because of which the problem was not caught earlier.Requiring the concrete ESProducer classes to explicitly call the base
ESProducer
class' constructor with theParameterSet
argument is a (minor) breaking change, but this was (by far) easiest way to fix the problem.PR validation:
The unit tests succeeds on machines with and without a GPU.
If this PR is a backport please specify the original PR and why you need to backport that PR. If this PR will be backported please specify to which release cycle the backport is meant for:
To be backported to 13_0_X (using the same commit).