Skip to content
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

[QST] Should detail APIs with corresponding public APIs with a default stream be removed #14276

Closed
vyasr opened this issue Oct 13, 2023 · 3 comments
Labels
question Further information is requested

Comments

@vyasr
Copy link
Contributor

vyasr commented Oct 13, 2023

What is your question?
With the ongoing addition of streams to various public libcudf APIs, a question that has been raised multiple times (most recently #14263 (comment) and #14261 (comment)) has been whether or not the corresponding detail API should be removed. Specifically, it concerns the following API structure:

namespace cudf {
namespace detail {
auto foo(..., rmm::cuda_stream_view stream);
}
auto foo(..., rmm::cuda_stream_view stream = cudf::get_default_stream()) {
    return foo(..., stream);
}
}

The question is whether maintaining cudf::detail::foo has any value when cudf::foo is equivalent and simply provides a default stream. The closest to an official discussion/decision on this was in #9854 (comment), where it was determined that we would like to preserve the public/detail pairs so we could inject specific public-facing logic into the public APIs while keeping the detail APIs leaner (with the specific example raised being adding nvtx ranges to public but not detail APIs). Since the question keeps coming up, though, and since we are only now in the process of actually adding these stream APIs and therefore exposing a broader range of libcudf devs to the concern, I wanted to open a dedicated issue in case anyone had new thoughts or concerns about this approach. If not, we can close this issue as simply documenting our policy that we intend to keep the separation between public and detail APIs.

@vyasr vyasr added question Further information is requested Needs Triage Need team to review and classify labels Oct 13, 2023
@vyasr vyasr added this to the Enable streams milestone Oct 13, 2023
@bdice
Copy link
Contributor

bdice commented Oct 13, 2023

I would like to keep the status quo here. In addition to the previously discussed reasons such as NVTX ranges, maintaining detail APIs that are called by the public API preserves flexibility in implementation and clarity in header inclusions that would be otherwise lost. I am happy to close this issue as a documentation of our architectural choice once consensus is achieved.

@jrhemstad
Copy link
Contributor

Looking back at the thread @vyasr linked, I'd forgotten I'd written this response #9854 (comment), buts it's pretty much verbatim what I was planning to write here again 🙂. Since we already have it, it's convenient to keep it.

@vyasr
Copy link
Contributor Author

vyasr commented Oct 24, 2023

Seems like enough approval for the status quo here, I'll close this issue as documenting our choice.

@vyasr vyasr closed this as completed Oct 24, 2023
@bdice bdice removed the Needs Triage Need team to review and classify label Mar 4, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
question Further information is requested
Projects
None yet
Development

No branches or pull requests

3 participants