[FEA] Consider removing default value from stream parameter in detail functions.

[nvdbaranec]

Based on @karthikeyann's work on this PR #9767 I'm wondering if it makes sense to consider removing the defaults for the stream parameters in various detail functions. It is pretty surprising how often these are getting missed.

The most common case seems to be in factory functions and various ::create functions. Maybe just do it for those?

[jrhemstad]

I'm an increasingly big fan of requiring streams to be explicit.

[karthikeyann]

In most cases, stream can be explicit.
There are a few cases where there are other default arguments before stream. so they cannot be removed.

[vyasr]

Since they are detail APIs we could always move streams earlier in the parameter list in those cases. It might make for a slightly incongruous API though.

[bdice]

Would we treat memory resource defaults in the same way?

I also support explicit streams (and memory resources?) in detail APIs. We can easily lift that default value to the level of the public APIs, which can just call detail APIs with the default values. The developer guide currently says to include stream and memory resource defaults in detail APIs, and we'd need to update that.

[ttnghia]

If the stream parameter is not explicit, you can use either the public API or its internal version in the same way. It's confusing. The reason we create the detail version is to use user-provided stream. So, removing the default value for stream is reasonable and a good practice.

[harrism]

Agree they should be explicit where possible.

[vyasr]

@nvdbaranec @harrism @jrhemstad @ttnghia @bdice I want to clarify the intent here: how do we envision this in a world where libcudf actually exposes a stream-ordered API? The eventual plan is to remove the current public APIs and replace them with the detail versions containing a stream, i.e. if we currently have cudf::function(...) and cudf::detail::function(..., stream = cudf::default_stream_value, mr =...) we will eventually delete cudf::function and promote cudf::detail::function from the detail namespace into cudf. At that point, any functions still in detail APIs should definitely not have default streams since that leaves room for errors in not forwarding streams down the call stack, but what about those APIs that become public? I assume that we still want those to have default stream parameters. Users of libcudf should not be forced to provide a stream; if they are happy running everything on the default stream, we should continue to facilitate that without requiring any additional work on the user's part to update their function calls, right?

[bdice]

I discussed this with @jrhemstad recently. We want to retain the detail/public split even after exposing streams, for the purposes of nvtx ranges and similar profiling at the public API level. Public APIS will still call detail APIs, but the stream will be part of the public API and have a default stream value. The stream will be a required parameter (no default value) for the detail API. No user code changes should be required, unless argument ordering conventions require us to change something that we do not want to work around.

[vyasr]

Could you elaborate on the profiling needs that @jrhemstad sees here? Do we want to be able to call the detail APIs from other places without adding NVTX ranges? It seems a little odd that we would need to make this distinction unless we have an issue with cudf::foo calling cudf::bar and inserting a second NVTX range, but perhaps I'm missing some context here. Is that kind of thing worth the overhead of maintaining a second set of APIs so that we can call cudf::detail::bar instead from cudf::foo?

[bdice]

@vyasr I think the decision to move away from organizing our APIs as public/detail is out of the scope for the project of exposing streams. I am in favor of keeping the current approach while we add streams to the public API. The nvtx ranges are one consideration, but there are others. The public APIs are currently fairly short, and do a good job of separating the (lengthy) implementations from their definitions so it is easy to find all public APIs in a file. In some places, multiple public APIs call a common detail API with a parameter to indicate the desired behavior. It is easier to make sweeping changes to docstrings and assess scope of future changes when the public APIs are cleanly separated.

[vyasr]

That's fine, we can have the organization discussion separately. This discussion gives me enough information for how to proceed on the streams front for now.

[jrhemstad]

Could you elaborate on the profiling needs that @jrhemstad sees here?

The general structure of all libcudf APIs is:

namespace cudf{
void function(...){
   CUDF_FUNC_RANGE();
   return detail::function(...);
}

Having this consistent pattern of a public function just calling a detail function is convenient for all sorts of things. The most immediate is having a place to put the CUDF_FUNC_RANGE without having to put it in the actual implementation where it would be easier to forget.

In general, it's just convenient to have a level of indirection between your public API and actual implementation. It enables you to do all sorts of things like insert custom logic for every public API.

If we didn't already have this pattern I wouldn't advocate to go through the work of adding it. But since we already have it, I think it's worthwhile to preserve.

[ttnghia]

I agree with the logic above 👍. Thanks all for working on this (and the related PRs 😃 ).

[vyasr]

@nvdbaranec @harrism @jrhemstad @ttnghia @bdice I've opened #11967 to address this. I'd appreciate some feedback regarding how we want to handle the cases where there are other default parameters. Let's continue that discussion on the PR.

[vyasr]

I've created #12943 to remove all remaining default stream parameters, which will close this issue. In #11967 we discussed removing all defaults from detail APIs. That is a somewhat larger undertaking since it will require removing default memory resource usage almost everywhere, so in the interest of not blocking progress on streams work I will create a separate issue to allow that to be worked on in parallel.