-
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
Move dictionary declarations of Alpaka serial backend data products to the host classes_def.xml #40792
Conversation
+code-checks Logs: https://cmssdt.cern.ch/SDT/code-checks/cms-sw-PR-40792/34220
|
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 |
hold (maybe I should have opened this as draft...) |
enable gpu |
@cmsbuild, please test |
Pull request has been put on hold by @makortel |
@smuzaffar Would it be feasible to modify the Alpaka library build rules such that the dictionaries for the Alternatively we could decide that we'd never generate dictionaries for the |
What happens today if In fact, what happens if |
The error I mentioned in the PR description
Apparently the build completes successfully, in both cases where the
I though I did |
So what actually happens in my test when I remove the But the build gets later stuck in
which looks the same as in root-project/root#11383.
I guess the dependence on |
@makortel @fwyzard ,
to only build
to build only |
+1 Summary: https://cmssdt.cern.ch/SDT/jenkins-artifacts/pull-request-integration/PR-32b74b/30672/summary.html Comparison SummarySummary:
GPU Comparison SummarySummary:
|
f31b550
to
7d20923
Compare
@cmsbuild, please test |
+1 Summary: https://cmssdt.cern.ch/SDT/jenkins-artifacts/pull-request-integration/PR-32b74b/30752/summary.html Comparison SummarySummary:
GPU Comparison SummarySummary:
|
+heterogeneous |
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 |
ping bot |
We're getting warnings (including in the test jobs for this PR):
and unit test failures:
that appear to be related to this PR? |
I'd expect those to disappear in a full build (the latest IB is still a patch build). @smuzaffar This PR removed the |
right, a full build should fix this
although we have protection against edm plugins (via poison plugin cache) but we do not poison shared libraires itself. I think we can fix this as well by creating poison libs but I am not sure how root will behave (it still might go through the LD_LIBRARY_PATH and load the shared library from release area). Note that these warnings are from root and not from cms plugin system |
Ah, the poison libraries were only for plugins. I started to wonder, because when we have moved dictionaries around in the past, I didn't recall any unit tests to fail because of that (but only the IB duplicate dictionary checker complaining until the next full build). |
PR description:
This PR moves the dictionary declarations of Alpaka serial backend data products to the host
classes_def.xml
. The "serial" data products are intended to be consumable by non-Alpaka EDModules as well, so having all of their definitions in the main library is more consistent. See more discussion in #40690 (comment) .I noticed though that without the
classes_serial_def.xml
andclasses_serial.h
files the build fails withand just to get started I included both with dummy content.
PR validation:
Unit tests pass (on a machine without GPU)