Open vyasr opened 11 months ago
Hi,
I took a look yesterday and from what I saw the API for the column headers already expose the cuda stream in the API. However I do not see a direct test for it.
I took a look yesterday and from what I saw the API for the column headers already expose the cuda stream in the API. However I do not see a direct test for it.
The stream tests are located here: https://github.com/rapidsai/cudf/tree/branch-23.12/cpp/tests/streams It is certainly possible we missed some tests. Which APIs did you find that are missing tests?
There are a small set of APIs that had streams added before we made the current push for adding streams, largely in cases where cuspatial needed them. Those predate the stream testing. We would welcome adding stream testing for those APIs if you find missing ones!
Source code of groupby should be updated to use stream from top level APIs. https://github.com/rapidsai/cudf/pull/14354#discussion_r1384175665 Related https://github.com/rapidsai/cudf/issues/11463 https://github.com/rapidsai/cudf/issues/13737
Is your feature request related to a problem? Please describe. Currently most libcudf APIs operate implicitly on the default CUDA stream. As documented in #925, we would like to expose stream-ordered APIs publicly. Prior to doing so, we needed to do a great deal of work to ensure that libcudf's internals were properly stream-ordered. Among the important tasks here were removing implicit APIs that ran on a stream (see #11968) and implementing a strategy for verifying that streams are properly forwarded from APIs all the way down to the lowest level functions (see #11943). Now that all such work that we are aware of has been completed, libcudf should start exposing streams in public APIs.
We originally considered adding stream support via a monolithic feature branch where APIs would be modified one at a time. From various discussions, however, we decided that the overhead of maintaining a feature branch would grow too large to be worthwhile, so we have instead decided to proceed by incrementally exposing streams in libcudf APIs over time. PRs adding new APIs should always include a stream in the public API. Existing APIs will be transitioned one header file at a time, with PRs going directly into the current main branch of cudf.
The purpose of this PR is to document progress on the task of transitioning existing headers, as well as to describe the process by which new stream-ordered APIs should be added.
Describe the solution you'd like When adding stream support to an API, the stream parameter should be placed just before the mr parameter with a default value of
cudf::get_default_stream()
. A new test should be added that validates that the API properly forwards the stream to all CUDA calls, which can be done by following the instructions at the bottom of libcudf's testing developer docs.To divide up the work, we consider public headers organized by directories (all relative to
cpp/include/cudf
). Devs may assign themselves a directory within which to own all headers and the public APIs contained therein.files (#)
Additional context Before we began making public stream parameters, libcudf public APIs would be paired with detail APIs that are nearly identical except for having a required stream parameter. In some cases, though, the public API may not currently have a detail "mirror" function but instead call multiple detail APIs. In those cases we may need to make a case-by-case determination of whether a new detail function should also be added. Some discussion of whether we want this pattern to be present everywhere, and the reasons why we might want that, are discussed in #14276.
Public stream API readiness: