-
Notifications
You must be signed in to change notification settings - Fork 891
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
[FEA] Consider removing default value from stream
parameter in detail functions.
#9854
Comments
I'm an increasingly big fan of requiring streams to be explicit. |
In most cases, stream can be explicit. |
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. |
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. |
If the |
Agree they should be explicit where possible. |
) Updates the developer guide to address #9854. With this change, any new or refactored detail APIs should have no default stream parameter. We may resolve that issue or perhaps leave it open until a majority of code has no default stream. Authors: - Bradley Dice (https://github.com/bdice) Approvers: - Nghia Truong (https://github.com/ttnghia) - MithunR (https://github.com/mythrocks) - David Wendt (https://github.com/davidwendt) URL: #10136
@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 |
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. |
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 |
@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. |
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. |
The general structure of all libcudf APIs is:
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 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. |
I agree with the logic above 👍. Thanks all for working on this (and the related PRs 😃 ). |
@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. |
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. Authors: - Vyas Ramasubramani (https://github.com/vyasr) Approvers: - Tobias Ribizel (https://github.com/upsj) - Nghia Truong (https://github.com/ttnghia) - Yunsong Wang (https://github.com/PointKernel) URL: #11967
Contributes to #9854. None of these changes should affect users, nor do they impose a particularly onerous burden on libcudf developers (just some extra passing through `mr` or `cudf::get_default_stream()`. Authors: - Vyas Ramasubramani (https://github.com/vyasr) Approvers: - Nghia Truong (https://github.com/ttnghia) - David Wendt (https://github.com/davidwendt) URL: #12888
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. |
This PR closes #9854, removing all default stream parameters in detail APIs. This increases stream-safety by removing the ability to accidentally use the default stream when a detail API is called without an explicit stream parameter when a user-provided stream should have been passed through. Authors: - Vyas Ramasubramani (https://github.com/vyasr) Approvers: - Bradley Dice (https://github.com/bdice) - Yunsong Wang (https://github.com/PointKernel) - David Wendt (https://github.com/davidwendt) URL: #12943
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?The text was updated successfully, but these errors were encountered: