Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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
Implement a chunked_pack API #13260
Implement a chunked_pack API #13260
Changes from 16 commits
a8ae8cc
3ea2844
2201de5
fe3bd77
4a4da2f
bd87dd5
4aae464
bd389ae
3b83b03
425a313
f7b18d3
cc9acb4
9b3a591
60a29b0
8c02d02
0480ef1
8f9dbbd
e2ca459
394ab62
c4864f1
3922d48
78361a8
be5fb01
e0686ff
2a822e6
8cf0afa
2ebb68b
0e10fdc
8c344a4
a9dfbc7
7a73d57
dae5ceb
ff6ce21
b47f2c7
b1fba4e
5ef8bd3
ab98d70
8ec29fa
f13156e
d959039
e16306c
b8dcff8
a41db75
bf208b4
3a3e9cd
File filter
Filter by extension
Conversations
Jump to
There are no files selected for viewing
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.
Even with documentation, I think this behavior is unexpected given the general approach that libcudf takes. I understand why we might need to support this, but I would prefer it be communicated very clearly in the API and not rely solely on documentation. Can we modify the struct to have two memory resources, the usual
mr
and an extratemp_mr
or so? They can both default to the per-device resource. That way the caller gets a very clear indication that the memory resource used for temp allocations is also controllable in this particular instance.If all allocations made by this function are temporary because it's just splitting an existing buffer, then maybe just renaming the mr makes sense.
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.
chunked_pack
never allocates result buffers. So I am going to replacemr
in this api withtemp_mr
to make it clear, as you suggestThere 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.
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.
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.
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.