-
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
GPUvsCPU: updates to modules and workflows for pixel reconstruction #37617
Conversation
enable gpu |
+code-checks Logs: https://cmssdt.cern.ch/SDT/code-checks/cms-sw-PR-37617/29386
|
A new Pull Request was created by @AdrianoDee for master. It involves the following packages:
@emanueleusai, @makortel, @fwyzard, @ahmad3213, @cmsbuild, @jfernan2, @clacaputo, @slava77, @jpata, @pmandrik, @micsucmed, @rvenditti can you please review it and eventually sign? Thanks. cms-bot commands are listed here |
view->m_hitsModuleStart = m_hitsModuleStart; | ||
|
||
//store transfer | ||
if constexpr (std::is_same<Traits, cms::cudacompat::GPUTraits>::value) { |
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.
just a suggestion:
if constexpr (std::is_same<Traits, cms::cudacompat::GPUTraits>::value) { | |
if constexpr (std::is_same_v<Traits, cms::cudacompat::GPUTraits>) { |
The c++ part looks good to me :-) |
enable gpu |
please test |
DQM/SiPixelPhase1Heterogeneous/python/SiPixelPhase1HeterogenousDQM_FirstStep_cff.py
Outdated
Show resolved
Hide resolved
@mmusich you are right, I hadn't pushed the modifications to @fwyzard may be helpful: find here the dependency graphs for
For the suggestion to move Anyway, I'll squash all the commits at the end of the review. |
+reconstruction
|
1 similar comment
+reconstruction
|
Thanks @AdrianoDee for the dependency graphs. Here is a comparison of the The difference is that the Here is the same comparison for the The |
+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, @qliphy (and backports should be raised in the release meeting by the corresponding L2) |
@AdrianoDee with these changes, how does the |
+1 |
@fwyzard, I checked the dependency graphs (in pdf here) and the tasks and there is a misalignment in the names in the local reco:
For the rest (and the final result) they run the same. Is this naming mismatch a possible issue? In case I'm trying to figure out a possible workaround given that, as far as I have understood, |
Also, do we need a backport of this? I missed the ORP today, sorry. |
No, I don't think so. |
Actually, I don't know if that would be a problem -- is it something you tried and didn't work ? |
@AdrianoDee There is a IB issue after this PR gets merged, would you please have a check? cmsRun: /data/cmsbld/jenkins/workspace/build-any-ib/w/tmp/BUILDROOT/5525f5050df49f51ff7c29cd32fd5dea/opt/cmssw/slc7_amd64_gcc10/cms/cmssw/CMSSW_12_4_X_2022-05-03-2300/src/RecoLocalTracker/SiPixelRecHits/plugins/SiPixelRecHitSoAFromLegacy.cc:144: virtual void SiPixelRecHitSoAFromLegacy::produce(edm::StreamID, edm::Event&, const edm::EventSetup&) const: Assertion |
@qliphy on it |
PR description:
Updates for pixel only workflows:
.502
and.503
to avoid running the CPU chain when a GPU is available (due to the fact that the DQM consumes hit SoA and that is only available on CPU);running both CPU and GPU chains in.503
;introducinggpuPixelValidation
modifier;dropping.501
that only generates confusion.PR validation:
Running
.502
and.503
.