-
Notifications
You must be signed in to change notification settings - Fork 4.6k
Remove PortableHostMultiCollection and PortableDeviceMultiCollection #49734
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
Remove PortableHostMultiCollection and PortableDeviceMultiCollection #49734
Conversation
|
cms-bot internal usage |
|
type ngt |
|
A new Pull Request was created by @Electricks94 for master. It involves the following packages:
@Martin-Grunewald, @Moanwar, @civanch, @cmsbuild, @ctarricone, @fwyzard, @gabrielmscampos, @jfernan2, @kpedro88, @makortel, @mandrenguyen, @mdhildreth, @mmusich, @nothingface0, @rseidita, @srimanob can you please review it and eventually sign? Thanks. cms-bot commands are listed here |
|
enable gpu |
|
test parameters:
|
DataFormats/Portable/README.md
Outdated
| ``` | ||
| The layouts should not be added as parameters for the device collection. Those script can be use equally with the | ||
| single layout collections or multi layout collections. | ||
| Multi layout collections have been removed and replaced by `SoABlocks`. See the `DataFormats/SoATemplate` |
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.
Seems broken off ?
|
|
||
| collectionName = sys.argv[1] | ||
|
|
||
| # TODO: do we need to change something here becuase of the removal of PortableMultiCollection? |
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 don't think this ever supported PortableMultiCollections.
Do we need a special syntax for the SoA blocks ?
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.
For the SoA blocks no special syntax is needed. It is equivalent to a normal PortableCollection. I adapted the file to be consistent with #48824. Which means first Collection declaration. Then Collection::Layout followed by the SoALayout that is aliased by Collection::Layout. Lastly the definition with edm::Wrapper
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.
Then, can you remove the TODO ?
|
|
||
| import sys | ||
|
|
||
| # TODO: do we need to change something here becuase of the removal of PortableMultiCollection? |
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 think you should remove the part if len(layouts) > 1:
|
|
||
| using TestSoA3 = TestSoALayout3<>; | ||
|
|
||
| GENERATE_SOA_BLOCKS(SoABlocks2, SOA_BLOCK(firstLayout, TestSoALayout), SOA_BLOCK(secondLayout, TestSoALayout2)) |
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.
maybe just
| GENERATE_SOA_BLOCKS(SoABlocks2, SOA_BLOCK(firstLayout, TestSoALayout), SOA_BLOCK(secondLayout, TestSoALayout2)) | |
| GENERATE_SOA_BLOCKS(SoABlocks2, SOA_BLOCK(first, TestSoALayout), SOA_BLOCK(second, TestSoALayout2)) |
?
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.
Same below.
| // Either int32_t for normal layouts or std::array<int32_t, N> for SoABlocks layouts | ||
| auto size() const { | ||
| return layout_.metadata().size(); | ||
| ; |
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.
| ; |
|
|
||
| // generic SoA-based product in host memory | ||
| // TODO: Should we define a size alias for the number of elements? Also should be switch to unsigned and/or 64 bit? | ||
| // Exeeding the size of int32_t fails without clear error messages. |
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.
This needs to be an int32_t (actually a plain int) because of how ROOT stores the columns 🤷🏻♂️ .
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 understand. How about allowing any integral type for the constructor of the PortableCollections and then implementing a check if the value can be savely casted to int? If it is save to cast we would do so and pass it on to the Layout, otherwise we throw an error.
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 just realised that the PortableCollections have already a constructor that takes std::integral as input type for the elements. So the idea would be just to align all other constructors as well and add a check for the cast.
| alpaka::memset(std::forward<TQueue>(queue), *buffer_, 0x00); | ||
| } | ||
|
|
||
| // part of the ROOT read streamer |
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.
Why remove the comment ?
| // Either int32_t for normal layouts or std::array<int32_t, N> for SoABlocks layouts | ||
| auto size() const { | ||
| return layout_.metadata().size(); | ||
| ; |
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.
| ; |
|
+1 |
|
+simulation |
|
+dqm |
|
This pull request is fully signed and it will be integrated in one of the next master IBs (test failures were overridden). This pull request will now be reviewed by the release team before it's merged. @mandrenguyen, @ftenchini, @sextonkennedy (and backports should be raised in the release meeting by the corresponding L2) |
|
+1 |
|
This PR caused 4 DQM integration test to fail, see #49892 |
|
I guess it's to be expected if the DQM tests reuse old streamer files as input ? As far as I know there is no guarantee that streamer files are readable with any release different from what was used to write them. |
Correct. |
…tions_160X [Backport to 16_0_X (#49734)] Remove PortableHostMultiCollection and PortableDeviceMultiCollection
This PR removes
PortableHostMultiCollectionandPortableDeviceMultiCollectionand replaces them with SoABlocks (see #48629).This has two major advantages:
In a future PRs the code can be simplified even more because with kernels that use a BlocksView as input can work on the entire data. Hence, it is not necessarry anymore to have every sub-view as input. In this PR, however, I tried to make as few changes as possible to not blow up the PR too much.
@felicepantaleo @leobeltra fyi