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
[REVIEW] Proposal for "proper" C/C++ API (issue #92) #247
[REVIEW] Proposal for "proper" C/C++ API (issue #92) #247
Changes from all commits
25997e8
5271ff2
5bf55f5
74e6039
2f20e45
84ad2e2
8099e38
e9f6771
2f273e5
28ab359
4a8ba29
8ba1c8d
16c41a5
1fcf45a
e2f662f
1286ef1
b139e5e
3ebd4b0
d90f276
0854765
effef57
81d2637
ff4fdc2
c784628
d0eef1e
694e956
dd2e2a2
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.
If you're providing a custom allocator for host memory, it's straightforward to provide an allocator for
std::vector
and similar containers. You could then even provide an alias likecuml::vector
that is astd::vector
using your custom allocator.We do this in cuDF with
rmm::device_vector
->thrust::device_vector
using RMM for the allocator.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 thought about that quite a bit. The advantage of passing in an allocator as a runtime argument over a template parameter are:
With a template argument we would need to reference a global object. Does that make sense to you?
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.
All good points.
I suppose since you are providing a host allocator and host_buffer, there's little reason for someone to use a
std::vector
.Another point in favor of "allocators as parameters" vs "template parameters" is that it allows stateful allocators, e.g., an allocator w/ a stream. This wouldn't be possible with an allocator as a template argument.
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.
You don't have to reference a global argument with the latest Thrust
device_vector
-- the constructor has an allocator parameter. The same is true ofstd::vector
: https://en.cppreference.com/w/cpp/container/vector/vector (see constructor #2 for example)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.
Recommend you comment on why these are recommended over just using
thrust::device_vector
andstd::vector
orthrust::host_vector
with your custom allocator. If there isn't a good reason to not use them, why create a new class?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.
Agree that should be clarified in the guide. I will add something.
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 especially now like the idea about writing our algos as:
void cumlAlgo(const cumlHandle_impl& h, ...)
. This avoids a ton ofgetImpl
calls in the downstream which would have been necessary, otherwise!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 is a really cool use of RAII. Love it.
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.
Consider adding:
Enables your
thrust::
algorithm calls to be one-liners. Your users will be happier.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 see your point, but I am not convinced that we should add this function. Regarding passing in
const cumlHandle_impl& h
instead ofstd::shared_ptr<deviceAllocator> allocator
does my argument that the execution policy does not depend oncumlHandle_impl
made above convince you?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 see an argument about this above. Above what?
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 I assumed you a reading top to bottom, propably github decided to hide part of the conversation. I was refering this reply made above:
Reply begin:
Regarding passing in ML::cumHandle_impl&: This is partly just my preference to be explicit, because the execution policy only needs the allocator and nothing else from handle. Besides that it avoids introducing a dependency of ML::exec_policy that is not really needed.
Regarding the one liner: I agree that this would be nicer. @jrhemstad said that this unfortunately requires thurst 1.9.4 or newer as the thrust execution policies with older thrust versions can't be initiated with temporaries. The current workaround with auto pointers also does not allow this because the auto pointer gets out of scope before the object is used. To my understanding another nice thing with thrust 1.9.4+ for this would be that one would not need to repeat the stream. We could provide a helper function that could be used like this:
thrust::for_each(ML::exec_policy(alloc, stream), ... );
Reply end
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.
To be even more explicit I should have propably written: "Regarding passing in
ML::cumHandle_impl&
instead ofstd::shared_ptr<deviceAllocator> allocator
: This is partly just my preference to be explicit, because the execution policy only needs the allocator and nothing else from handle"