-
Notifications
You must be signed in to change notification settings - Fork 245
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
Add more CUDADataFormats packages for the Patatrack integration #1395
Conversation
See - cms-sw/cmssw#31703: Patatrack integration - common data formats (5/N) - cms-sw/cmssw#31704: Patatrack integration - calorimeters shared code (6/N) - cms-sw/cmssw#31719: Patatrack integration - ECAL local reconstruction (7/N) - cms-sw/cmssw#31720: Patatrack integration - HCAL local reconstruction (8/N) - cms-sw/cmssw#31721: Patatrack integration - Pixel local reconstruction (9/N) - cms-sw/cmssw#31722: Patatrack integration - Pixel track reconstruction (10/N) - cms-sw/cmssw#31723: Patatrack integration - Pixel vertex reconstruction (11/N)
A new Pull Request was created by @fwyzard (Andrea Bocci) for branch master. @cmsbuild, @smuzaffar, @mrodozov can you please review it and eventually sign? Thanks. |
"CUDADataFormats/BeamSpot", | ||
"CUDADataFormats/CaloCommon", |
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.
if these are supposed to be used in reco CUDA, I think that a reco signature is needed as well.
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.
@fwyzard , any idea if these will be used by reco CUDA?
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.
yes
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.
can you please update the PR to add these under reconstruction too
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.
shall I also add under "heterogeneous" all packages that do or will contain any kind of CUDA code ?
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.
that is a good question, dependency of a category X should not be responsibility of that category X.
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.
Sorry, what I meant is: should heterogeneous (currently, only CUDA) code be signed by the heterogeneous
category ?
If the answer is "no", then what about the data formats just added under reconstucrtion
?
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 have a vague recollection that the last time this was discussed (sorry no reference) the outcome was not to add heterogeneous
on every package with CUDA code, but to be assigned by other L2s when necessary. I'm fine with moving all other CUDADataFormats
than CUDADataFormats/Common
and CUDADataFormats/StdDictionaries
only under reconstruction
.
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.
Let me add (post-merge) that after some further thought I think I contradicted myself in above (assign heterogneous
on PRs on demand, but leave CUDADataFormats
packages out from heterogeneous
), and leaving all CUDADataFormats
packages (also) under heterogeneous
is more consistent.
Pull request #1395 was updated. |
Pull request #1395 was updated. |
As requested by @slava77 .
Pull request #1395 was updated. |
See: