[REVIEW] Execution policy class

[viclafargue]

Changes currently planned for the RAFT code include updating the RAFT handle for it to store a singleton of a thrust execution policy. An execution policy class would prove useful to write a clearer code. This PR is a proof of concept for it.

cc @divyegala

[jrhemstad]

to store a singleton of a thrust execution policy

I'm curious why you wish to do this. Are you concerned about the overhead of constructing an execution policy on the fly with the existing rmm::exec_policy function?

[viclafargue]

to store a singleton of a thrust execution policy

I'm curious why you wish to do this. Are you concerned about the overhead of constructing an execution policy on the fly with the existing rmm::exec_policy function?

We are planning on adding a raft::handle_t::get_thrust_policy() method to the RAFT handle and use it everywhere a thrust execution policy is required. This would be to provide an abstraction over the rmm::exec_policy and would prevent any depreciation. Indeed, if the RMM code is modified, we would only have to update this method in the RAFT handle. But this idea is still in discussion. The idea of having a singleton is for both efficiency and also to be coherent in the way resources are created and provided to developers in the RAFT handler. Indeed, cuBlas, cuSparse and cuSolver handles are stored as singleton as well.

[jrhemstad]

raft::handle_t::get_thrust_policy() could just return a call to rmm::exec_policy() as it is today, right?

I don't think you have to worry about the overhead of constructing the execution policy as it will all get inlined/optimized away anyways.

That said, I'm not opposed to an exec_policy class, but I don't want to have to change all of the existing calls to rmm::exec_policy to rmm::create_exec_policy.

[divyegala]

@jrhemstad if the overhead isn't very high of creating, then I agree that raft::handle_t::get_thrust_policy() could just call rmm::exec_policy(). But it would be nice to hide that behind an rmm::exec_policy class that will handle deprecations to both the RMM and thrust APIs. Furthermore, I agree that a factory method here isn't the right approach.

In that case, rmm::exec_policy could be a class that has an implicit conversion operator to thrust's policy class, and is coupled with a stream view instance. Would that work?

Edit: It would look something like this:

class exec_policy {

    explicit exec_policy(cuda_stream_view stream_) : 
        stream(stream_) { }

    operator thrust_exec_policy_t() {
         return ...;
    }

    cuda_stream_view stream;
};

[viclafargue]

I renamed the rmm::exec_policy function back to its original name, and implemented the suggested class.

[harrism]

Why is deprecation such a concern? rmm::exec_policy has only had one deprecation in the life of the library, and I think the current API should be quite stable.

[harrism]

To pass the style checks please merge the latest from upstream into your PR branch.

[viclafargue]

Why is deprecation such a concern? rmm::exec_policy has only had one deprecation in the life of the library, and I think the current API should be quite stable.

It might indeed never come useful if the API is left unchanged. However, I guess it's probably better to be prepared for such eventuality. The change might come from a change in the Thrust library.

[divyegala]

I believe with this, we can have a non-breaking API update without changing the way this was originally working. cc @jrhemstad @harrism who will be able to help us best here

[harrism]

It would be nice to follow Divye's suggestion to make this a replacement for the existing function that works identically. Also, I believe _t names are meant to be reserved for use by standard library types.

[viclafargue]

I removed the function and renamed the class as exec_policy. After some discussion with Divye, it appeared that the conversion function couldn't work because of templates in thrust. Instead the class is now designed to inherit from the thrust execution policy.

[divyegala]

This looks good for RAFT and doesn't break the existing code. LGTM!

[viclafargue]

rerun tests

[viclafargue]

The PR should be ready to be merged.

[harrism]

Thanks!

[galipremsagar]

@gpucibot merge