Feature/remove default streams

[vyasr]

Description

Default stream parameters can lead to subtle bugs that are hard to track down if public APIs start exposing streams. Removing the defaults ensures that streams are properly forwarded through everywhere that they should be.

This PR partially addresses #9854. It does not change the cases where removing the default value from a stream parameter would necessitate changing the order of parameters in the function signature due to the presence of other default parameters. That work will be done in a follow-up PR.

Checklist

• I am familiar with the Contributing Guidelines.
• New or existing tests cover these changes.
• The documentation is up to date with these changes.

[vyasr]

Note to reviewers:

I have added a TODO: ... comment next to every stream parameter where I was not able to remove the default because of earlier default parameters in the function signature. Please have a look through those so that we can figure out what strategy we want to adopt for those cases so that I can apply a consistent approach across all of our APIs.

[codecov]

Codecov Report

Base: 87.40% // Head: 88.15% // Increases project coverage by +0.74% 🎉

Coverage data is based on head (e0c0aaf) compared to base (f72c4ce).
Patch has no changes to coverable lines.

Additional details and impacted files

@@               Coverage Diff                @@
##           branch-22.12   #11967      +/-   ##
================================================
+ Coverage         87.40%   88.15%   +0.74%     
================================================
  Files               133      133              
  Lines             21833    21995     +162     
================================================
+ Hits              19084    19389     +305     
+ Misses             2749     2606     -143     

Impacted Files	Coverage Δ
python/strings_udf/strings_udf/__init__.py	86.27% <0.00%> (-10.61%)	⬇️
python/cudf/cudf/io/text.py	91.66% <0.00%> (-8.34%)	⬇️
python/cudf/cudf/core/_base_index.py	82.20% <0.00%> (-3.35%)	⬇️
python/strings_udf/strings_udf/_typing.py	94.73% <0.00%> (-1.06%)	⬇️
python/cudf/cudf/testing/dataset_generator.py	72.83% <0.00%> (-0.42%)	⬇️
python/dask_cudf/dask_cudf/backends.py	84.90% <0.00%> (-0.37%)	⬇️
python/dask_cudf/dask_cudf/core.py	73.92% <0.00%> (-0.21%)	⬇️
python/cudf/cudf/io/orc.py	92.94% <0.00%> (-0.09%)	⬇️
python/cudf/cudf/__init__.py	90.69% <0.00%> (ø)
python/cudf/cudf/utils/ioutils.py	79.47% <0.00%> (ø)
... and 22 more

Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here.

☔ View full report at Codecov.
📢 Do you have feedback about the report comment? Let us know in this issue.

[bdice]

Regardless of how we proceed with API conventions, we should merge almost all the changes in .cu and .cpp files from this PR that add an explicit stream to detail API calls (the changes in headers where API defaults were removed are worth more consideration/debate). There are a lot of places internally and in tests/benchmarks where detail APIs are being called without providing a stream, but we should provide that argument even if it has a default in the detail API.

[bdice]

I posed an idea to @vyasr offline, about whether we should remove all default parameters from detail APIs in libcudf. It might sound daunting / undesirable at first - but consider this argument.

We want to remove default streams from the detail APIs. However, many functions have stream near the end and have default parameter values preceding it, so we can't provide a stream without providing all the parameters preceding it (due to C++ calling rules -- Python would let you specify an arbitrary keyword argument out of order). Therefore, at present, essentially all calls to detail APIs must specify all parameters up to the stream, and potentially also the memory resource mr. To be clear: when calling a detail API, the caller should be passing a stream. This requires the caller to specify all arguments preceding that stream parameter, so you're not able to make use of detail API defaults in the current state anyway.

If we were to remove default parameter values from all detail APIs, we would see the following consequences:

• All defaults are defined and documented only once, by public APIs (rather than once in public and once in detail APIs)
  • Pro: this is explicit. Con: changing defaults requires refactoring every call site, but this is already true for the status quo.
• If no default parameters are provided, we would never have to worry about whether a detail API was being called with a stream or mr when it needs it. See PRs like Fix missing streams #9767 for examples of where this has caused problems.
• We still face an API break when exposing stream parameters to the public APIs, because it would go before the mr parameter (no different than the status quo). The public APIs would have a default value for stream, but callers currently passing a non-default mr would need to refactor to provide a stream.

Alternatives:

• Move stream to the front of the detail parameter list. Then all defaulted parameters can be at the end. cudf::do_stuff(stream, input, ...);
  • However, not all APIs require a stream.
• Put stream in the middle of the parameter list, after the required parameters but before the parameters with default values.
  • @vyasr and I agree this would look gross.
• Only remove default stream values from detail APIs without default parameters preceding the stream parameter. This is only a partial solution to [FEA] Consider removing default value from stream parameter in detail functions. #9854 and leaves the library somewhat inconsistent but is the least work and would offer some limited benefit for correctness (requiring passing streams explicitly in some cases, with the expectation that callers of detail APIs should always pass a stream).

[upsj]

Sounds like a sensible change to me, LGTM

[PointKernel]

@bdice What an explanation! 🔥

I started reviewing this PR yesterday and first thought it was a good idea to put stream in the middle of the parameter list. This way requires the minimum change to the existing implementation but makes the positioning of the stream parameter too flexible to track. Putting stream right before the first parameter with a default value is less laborious but sounds a bit fishy.

After seeing a lot of code with default values before the stream parameter, now I'm inclined to remove all default parameters in detail APIs. This solution sounds scary in the first place as it requires numerous changes across the whole cpp repo, but it will bring more consistency and (maybe?) make the code easier to maintain/improve.

To restrict the scope of this PR, we could focus on the simple cases only where there are no other default values before stream. Then we can do a file-by-file or repo-by-repo refactor for the complex ones in subsequent PRs. @vyasr @bdice What do you think?

[ttnghia]

Encapsulating the CUDA launch parameters and similar related data into a raft Handle-like object.

I just know this (from @vyasr from Slack).

This gives me a new idea for another approach. Look at the thrust:: APIs:

thrust::func_name(execution_policy const& exec, ...);

They have an execution policy object as the first parameter.

In cudf, most cudf APIs require two execution-related parameters: stream and mr. It is tedious and lengthy to provide/pass around these two together, but we frequently have to do that. If we can combine them into some cudf_execution_policy object (or any other name):

struct execution_policy {
explicit execution_policy(....) {}

rmm::cuda_stream_view stream = cudf::get_default_stream();
rmm::mr::device_memory_resource* mr = rmm::mr::get_current_device_resource();
};

and then the cudf APIs can always require that execution policy object as their first parameter. By doing so, we will have a very thrust:: style APIs like:

cudf::sort(cudf::execution_policy const& exec, table_view const& input, ...);

Note that the execution_policy parameter should not have a default value. The users may construct a default value for it (like cudf::get_default_execution_policy(), or specify the stream and mr they already have to construct it, then pass it in.

[vyasr]

@ttnghia yes you've got the gist of how the "handle" or "execution policy" object would work, and you're right that it would make for a more thrust-like API.

Thanks everyone for a great discussion here! In the interest of not letting this work go stale, there seems to be consensus that we should try and get the uncontroversial changes in this PR merged. I have updated the PR accordingly. Once a Java codeowner like @ttnghia approves, I will merge this PR and then start putting together examples of the different approaches we have discussed taking for the cases where removing a default stream parameter would require changes to other default parameters in a signature. We can continue the discussion there.

[ttnghia]

Java changes LGTM (github didn't show Java approval?).

[PointKernel]

LGTM

[vyasr]

@gpucibot merge