Implement cuda.bench.State.set_stream(provider) - #443
Open
oleksandr-pavlyk wants to merge 6 commits into
Open
Conversation
The stream provide type is expected to implement __cuda_stream__ protocol, and not be an instance of cuda.bench.CudaStream. The reason is that cuda.bench.CudaStream type is not user-constructible. It is returns by cuda.bench.State.get_stream method. It stores reference to stream stored in nvbench::state referenced from cuda.bench.State. Disallowing cuda.bench.CudaStream as argument to set_stream avoid footguns.
This comment was marked as outdated.
This comment was marked as outdated.
This comment was marked as outdated.
This comment was marked as outdated.
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Enterprise Run ID: 📒 Files selected for processing (3)
📝 WalkthroughSummary by CodeRabbit
WalkthroughThe PR adds the ChangesExternal CUDA stream support
Assessment against linked issues
Comment |
Replace pybind11 keep_alive with an explicit per-state provider cache so repeated State.set_stream calls release previously installed providers instead of retaining every historical stream provider.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
Add test for repeated set_stream of the same external owning stream Python object, with expectation that it does not double-free.
Docstring should not require that external stream provider must own stream, only that it keeps it alive. Added notes that both run_all_benchmarks and State.exec evaluate with GIL held. Add note about intended way of using State.set_stream
This comment was marked as outdated.
This comment was marked as outdated.
This comment was marked as outdated.
This comment was marked as outdated.
Collaborator
Author
|
@PointKernel This PR fixes the limitation mentioned in NVIDIA/cuvs#2371 (comment) that prompted filing for #440 |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
The stream provide type is expected to implement
__cuda_stream__protocol, and not be an instance of typecuda.bench.CudaStream.The reason is that
cuda.bench.CudaStreamtype is not user-constructible. It is returned bycuda.bench.State.get_streammethod. It stores reference to the stream stored innvbench::statereferenced fromcuda.bench.State.Disallowing instances of
cuda.bench.CudaStreamas argument toset_streamavoids footguns.Closes #440