Skip to content

Move stream objects from cuda_stream cython to stream cython#2256

Merged
bdice merged 12 commits intorapidsai:mainfrom
brandon-b-miller:fix-1782
Feb 25, 2026
Merged

Move stream objects from cuda_stream cython to stream cython#2256
bdice merged 12 commits intorapidsai:mainfrom
brandon-b-miller:fix-1782

Conversation

@brandon-b-miller
Copy link
Copy Markdown
Contributor

Closes #1782

@coderabbitai
Copy link
Copy Markdown

coderabbitai bot commented Feb 17, 2026

Note

Reviews paused

It looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the reviews.auto_review.auto_pause_after_reviewed_commits setting.

Use the following commands to manage reviews:

  • @coderabbitai resume to resume automatic reviews.
  • @coderabbitai review to trigger a single review.

Use the checkboxes below for quick actions:

  • ▶️ Resume reviews
  • 🔍 Trigger review
📝 Walkthrough

Walkthrough

Replaced local CudaStream/CudaStreamFlags definitions with implementations in rmm.pylibrmm.stream, added an internal _OwningStream, reexported aliases in the deprecated rmm.pylibrmm.cuda_stream (with an import-time DeprecationWarning), and updated tests and bindings to use the new flags/ownership API.

Changes

Cohort / File(s) Summary
Public package init
python/rmm/rmm/pylibrmm/__init__.py
Switched CudaStreamFlags import to rmm.pylibrmm.stream; updated copyright year.
Stream implementation & types
python/rmm/rmm/pylibrmm/stream.pyx, python/rmm/rmm/pylibrmm/stream.pxd, python/rmm/rmm/pylibrmm/stream.pyi
Added public CudaStreamFlags IntEnum, introduced internal final _OwningStream RAII wrapper (nogil accessors), extended Stream to accept flags and hold _OwningStream.
Deprecated cuda_stream module
python/rmm/rmm/pylibrmm/cuda_stream.pyx, python/rmm/rmm/pylibrmm/cuda_stream.pxd
Removed local enum/class defs; reexported CudaStream (alias of _OwningStream) and CudaStreamFlags from stream; emit DeprecationWarning on import; updated pxd to alias imported owning type.
Librmm binding updates
python/rmm/rmm/librmm/cuda_stream.pxd
Exposed cuda_stream_flags enum, added cuda_stream(cuda_stream_flags) constructor and synchronize/synchronize_no_throw methods to C++ bindings.
Tests
python/rmm/rmm/tests/test_stream.py
Repointed tests to rmm.pylibrmm.stream.CudaStreamFlags, added test for rmm.pylibrmm.cuda_stream deprecation warning; minor header update.
Pre-commit config
.pre-commit-config.yaml
Included .pyi files in copyright verification hook.

Estimated code review effort

🎯 4 (Complex) | ⏱️ ~45 minutes

Possibly related issues

🚥 Pre-merge checks | ✅ 4 | ❌ 1

❌ Failed checks (1 warning)

Check name Status Explanation Resolution
Docstring Coverage ⚠️ Warning Docstring coverage is 0.00% which is insufficient. The required threshold is 80.00%. Write docstrings for the functions missing them to satisfy the coverage threshold.
✅ Passed checks (4 passed)
Check name Status Explanation
Title check ✅ Passed The title clearly and concisely summarizes the main change: moving stream objects from cuda_stream Cython module to stream Cython module, which directly corresponds to the primary objective of the PR.
Description check ✅ Passed The description references the closed issue #1782, which is directly related to the changeset's objective of deprecating cuda_stream and moving code to stream.
Linked Issues check ✅ Passed The PR successfully implements the requirements from issue #1782: deprecates rmm.pylibrmm.cuda_stream with a DeprecationWarning, relocates CudaStreamFlags and CudaStream (as _OwningStream) to the stream module, and maintains backward compatibility through reexports.
Out of Scope Changes check ✅ Passed All changes are directly related to the stated objective of moving stream objects from cuda_stream to stream and deprecating the cuda_stream module; no unrelated modifications were introduced.

✏️ Tip: You can configure your own custom pre-merge checks in the settings.

✨ Finishing Touches
🧪 Generate unit tests (beta)
  • Create PR with unit tests
  • Post copyable unit tests in a comment

Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out.

❤️ Share

Comment @coderabbitai help to get the list of available commands and usage tips.

Copy link
Copy Markdown

@coderabbitai coderabbitai bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actionable comments posted: 2

🧹 Nitpick comments (2)
python/rmm/rmm/pylibrmm/stream.pyx (1)

41-53: Consider a null guard in value() / is_valid() for robustness.

Both accessors call c_obj.get()[0].<method>() without checking for a null pointer. In practice this is safe since @cython.final prevents subclassing and __cinit__ always sets c_obj before any accessor can be called. However, since is_valid() is semantically a validity predicate, it's conventional to make it safe to call even when the underlying resource is absent.

🛡️ Optional defensive hardening
 cdef cudaStream_t value(self) except * nogil:
+    if not self.c_obj:
+        with gil:
+            raise RuntimeError("CudaStream is not initialized")
     return self.c_obj.get()[0].value()

 cdef bool is_valid(self) except * nogil:
+    if not self.c_obj:
+        return False
     return self.c_obj.get()[0].is_valid()
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.

In `@python/rmm/rmm/pylibrmm/stream.pyx` around lines 41 - 53, The accessors
value() and is_valid() call self.c_obj.get()[0].* without null checks; add a
guard that first obtains the pointer via self.c_obj.get() and if it is NULL
return a safe default (for value() return a null cudaStream_t/0 and for
is_valid() return False) to make these methods robust when c_obj is absent;
update the cdef methods value and is_valid to check the pointer before indexing
and call into the underlying cuda_stream only when non-NULL.
python/rmm/rmm/tests/test_stream.py (1)

96-97: Import path update is correct; add a test for the deprecation warning.

The CudaStreamFlags path update is right. However, there is no test that verifies import rmm.pylibrmm.cuda_stream emits a DeprecationWarning, which the coding guidelines require when implementing deprecation features. Consider adding a test such as:

def test_cuda_stream_module_deprecation():
    import importlib, sys, warnings
    sys.modules.pop("rmm.pylibrmm.cuda_stream", None)
    with warnings.catch_warnings(record=True) as w:
        warnings.simplefilter("always")
        importlib.import_module("rmm.pylibrmm.cuda_stream")
    assert any(issubclass(warning.category, DeprecationWarning) for warning in w)

Would you like me to open a new issue to track adding this deprecation test, or generate the full test function?

🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.

In `@python/rmm/rmm/tests/test_stream.py` around lines 96 - 97, Add a unit test
named test_cuda_stream_module_deprecation that ensures importing the deprecated
module rmm.pylibrmm.cuda_stream emits a DeprecationWarning: clear any cached
module via sys.modules.pop("rmm.pylibrmm.cuda_stream", None), use
importlib.import_module to import it inside a
warnings.catch_warnings(record=True) with warnings.simplefilter("always"), then
assert that at least one recorded warning has category DeprecationWarning (use
issubclass(warn.category, DeprecationWarning)). This test should live alongside
existing tests (e.g., near tests referencing CudaStreamFlags) so the import path
change is exercised.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.

Inline comments:
In `@python/rmm/rmm/pylibrmm/cuda_stream.pyx`:
- Around line 10-15: Update the type stub so it stops importing CudaStreamFlags
from the deprecated module: in cuda_stream_pool.pyi replace the import that
references rmm.pylibrmm.cuda_stream with an import from rmm.pylibrmm.stream (the
module that now re-exports CudaStreamFlags) so the deprecation warning no longer
fires at package load time.

In `@python/rmm/rmm/pylibrmm/stream.pyi`:
- Line 1: Update the SPDX file header in stream.pyi to include 2026 instead of
2025 so it matches other modified files; locate the top-of-file comment starting
with "SPDX-FileCopyrightText" in stream.pyi and change the year range from
"2020-2025" to "2020-2026" to keep headers consistent across the PR.

---

Nitpick comments:
In `@python/rmm/rmm/pylibrmm/stream.pyx`:
- Around line 41-53: The accessors value() and is_valid() call
self.c_obj.get()[0].* without null checks; add a guard that first obtains the
pointer via self.c_obj.get() and if it is NULL return a safe default (for
value() return a null cudaStream_t/0 and for is_valid() return False) to make
these methods robust when c_obj is absent; update the cdef methods value and
is_valid to check the pointer before indexing and call into the underlying
cuda_stream only when non-NULL.

In `@python/rmm/rmm/tests/test_stream.py`:
- Around line 96-97: Add a unit test named test_cuda_stream_module_deprecation
that ensures importing the deprecated module rmm.pylibrmm.cuda_stream emits a
DeprecationWarning: clear any cached module via
sys.modules.pop("rmm.pylibrmm.cuda_stream", None), use importlib.import_module
to import it inside a warnings.catch_warnings(record=True) with
warnings.simplefilter("always"), then assert that at least one recorded warning
has category DeprecationWarning (use issubclass(warn.category,
DeprecationWarning)). This test should live alongside existing tests (e.g., near
tests referencing CudaStreamFlags) so the import path change is exercised.

@bdice
Copy link
Copy Markdown
Collaborator

bdice commented Feb 18, 2026

@brandon-b-miller Were you able to assess the alternative proposal in that issue?

We might be able to remove the CudaStream class completely and move all functionality to the main public Stream class.

I'd like to have one stream class unless there's a really compelling reason to have two.

@bdice
Copy link
Copy Markdown
Collaborator

bdice commented Feb 18, 2026

It seems that CudaStream is an owning object while Stream is a non-owning class (more like CCCL's stream_ref, perhaps) that uses self._owner to possibly manage the lifetime of another object (a held CudaStream).

I think maybe the pathway here is to make CudaStream private (I see no public usage of the class in RAPIDS) and rename it to _OwningStream? I don't think we want to fuse it with the Stream class because it would come at the cost of a more complex destructor, which is hard to make promises about in Python.

@brandon-b-miller
Copy link
Copy Markdown
Contributor Author

@brandon-b-miller Were you able to assess the alternative proposal in that issue?

We might be able to remove the CudaStream class completely and move all functionality to the main public Stream class.

I'd like to have one stream class unless there's a really compelling reason to have two.

Not yet, let's take a look at that 👍 . I'll follow up here.

@brandon-b-miller brandon-b-miller added non-breaking Non-breaking change improvement Improvement / enhancement to an existing function labels Feb 18, 2026
@brandon-b-miller
Copy link
Copy Markdown
Contributor Author

It seems that CudaStream is an owning object while Stream is a non-owning class (more like CCCL's stream_ref, perhaps) that uses self._owner to possibly manage the lifetime of another object (a held CudaStream).

I think maybe the pathway here is to make CudaStream private (I see no public usage of the class in RAPIDS) and rename it to _OwningStream? I don't think we want to fuse it with the Stream class because it would come at the cost of a more complex destructor, which is hard to make promises about in Python.

Done. I think long term as this becomes a cuda-core stream the _OwningStream concept gets changed out for a StreamHandle that eventually owns a cuda::stream.

Copy link
Copy Markdown

@coderabbitai coderabbitai bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actionable comments posted: 1

🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.

Inline comments:
In `@python/rmm/rmm/pylibrmm/cuda_stream.pyx`:
- Around line 6-18: Re-export CudaStream from rmm.pylibrmm.stream alongside
CudaStreamFlags so deprecated imports keep working: import CudaStream and
CudaStreamFlags from rmm.pylibrmm.stream and add "CudaStream" to __all__; also
update the DeprecationWarning text to mention both CudaStream and
CudaStreamFlags (e.g., "rmm.pylibrmm.cuda_stream is deprecated; use
rmm.pylibrmm.stream for CudaStream and CudaStreamFlags.") and keep stacklevel=2.

Copy link
Copy Markdown

@coderabbitai coderabbitai bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actionable comments posted: 2

🧹 Nitpick comments (1)
python/rmm/rmm/pylibrmm/cuda_stream.pyx (1)

8-14: Consider adding inline comments explaining the dual import + sys.modules pattern.

The cimport on line 8 satisfies the .pxd Cython-type interface for callers that cimport CudaStream, while the sys.modules injection on line 14 is the Python-level workaround. Without explanation a future reader may remove one of these thinking it's redundant.

📝 Suggested inline comments
+# cimport alias satisfies cuda_stream.pxd's CudaStream type declaration for
+# Cython callers that do `from rmm.pylibrmm.cuda_stream cimport CudaStream`.
 from rmm.pylibrmm.stream cimport _OwningStream as CudaStream

 import sys

 from rmm.pylibrmm.stream import CudaStreamFlags, _OwningStream

+# Cython cimport aliases are not automatically Python-level module attributes;
+# inject manually so `from rmm.pylibrmm.cuda_stream import CudaStream` works.
 sys.modules[__name__].CudaStream = _OwningStream
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.

In `@python/rmm/rmm/pylibrmm/cuda_stream.pyx` around lines 8 - 14, Add concise
inline comments around the dual import pattern to explain why both the Cython
cimport and the Python sys.modules assignment are required: note that the
cimport of CudaStream satisfies Cython/.pxd consumers (cdef/cimport usage) while
the sys.modules[__name__].CudaStream = _OwningStream line exposes the same
symbol at the Python runtime for importers that use "from
rmm.pylibrmm.cuda_stream import CudaStream"; reference the specific symbols
_OwningStream and CudaStream and call out that they must remain in sync and not
be removed as they serve different import mechanisms.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.

Inline comments:
In `@python/rmm/rmm/pylibrmm/cuda_stream.pyx`:
- Around line 16-21: Update the deprecation warning so it clarifies both
symbols' migrations: state that CudaStreamFlags moved to rmm.pylibrmm.stream and
that CudaStream has been made internal (renamed to _OwningStream) with Stream
from rmm.pylibrmm.stream as the public replacement; reference the symbols
CudaStream, CudaStreamFlags, _OwningStream and Stream in the message and keep
the DeprecationWarning and stacklevel unchanged.

In `@python/rmm/rmm/pylibrmm/stream.pyx`:
- Around line 19-31: Update the Cython declarations and constructors to accept
and forward the CUDA stream flags: add a flags parameter to the pxd declaration
for cuda_stream (e.g., change cuda_stream() except + to
cuda_stream(cuda_stream::flags flags = cuda_stream::flags::sync_default) except
+ or equivalent using the exposed enum), then modify _OwningStream.__cinit__ to
accept a flags argument and pass it into the underlying cpp cuda_stream
constructor, and update Stream.__init__ and _init_with_new_cuda_stream to accept
a flags parameter and propagate it through to _OwningStream (and any place that
constructs a new cuda_stream) so CudaStreamFlags.NON_BLOCKING becomes usable
from Python.

---

Nitpick comments:
In `@python/rmm/rmm/pylibrmm/cuda_stream.pyx`:
- Around line 8-14: Add concise inline comments around the dual import pattern
to explain why both the Cython cimport and the Python sys.modules assignment are
required: note that the cimport of CudaStream satisfies Cython/.pxd consumers
(cdef/cimport usage) while the sys.modules[__name__].CudaStream = _OwningStream
line exposes the same symbol at the Python runtime for importers that use "from
rmm.pylibrmm.cuda_stream import CudaStream"; reference the specific symbols
_OwningStream and CudaStream and call out that they must remain in sync and not
be removed as they serve different import mechanisms.

@brandon-b-miller brandon-b-miller added the 3 - Ready for review Ready for review by team label Feb 24, 2026
@brandon-b-miller
Copy link
Copy Markdown
Contributor Author

@bdice I think this should be ready for another look.

@bdice bdice requested a review from a team as a code owner February 24, 2026 23:46
@bdice bdice requested a review from jameslamb February 24, 2026 23:46
Copy link
Copy Markdown

@coderabbitai coderabbitai bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

♻️ Duplicate comments (1)
python/rmm/rmm/pylibrmm/cuda_stream.pyx (1)

16-22: Verify CudaStream is publicly exported from rmm.pylibrmm.stream before the deprecation message directs users there.

The warning text "use the new rmm.pylibrmm.stream module for CudaStreamFlags and CudaStream" is only accurate if CudaStream (not just _OwningStream) is importable from rmm.pylibrmm.stream. If stream.pyx only exposes _OwningStream and no CudaStream alias, the guidance is misleading and users following it will get an ImportError.

#!/bin/bash
# Confirm CudaStream is a public name in stream.pyx (not just _OwningStream).
echo "=== Module-level CudaStream alias in stream.pyx ==="
rg -n "^CudaStream\s*=" python/rmm/rmm/pylibrmm/stream.pyx

echo -e "\n=== __all__ in stream.pyx (if defined) ==="
rg -n "__all__" python/rmm/rmm/pylibrmm/stream.pyx

echo -e "\n=== CudaStream in stream.pyi type stub ==="
rg -n "CudaStream" python/rmm/rmm/pylibrmm/stream.pyi

If CudaStream is not exported from stream.pyx, the deprecation message should be updated to clarify that CudaStream is now internal and that Stream (or _OwningStream) is the low-level equivalent, e.g.:

 warnings.warn(
     "rmm.pylibrmm.cuda_stream is deprecated; "
     "use the new rmm.pylibrmm.stream module for "
-    "CudaStreamFlags and CudaStream.",
+    "CudaStreamFlags. CudaStream has been made internal; "
+    "use rmm.pylibrmm.stream.Stream instead.",
     DeprecationWarning,
     stacklevel=2,
 )
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.

In `@python/rmm/rmm/pylibrmm/cuda_stream.pyx` around lines 16 - 22, The
deprecation warning directs users to rmm.pylibrmm.stream for CudaStream but may
be incorrect if stream.pyx doesn't publicly export CudaStream; verify that
stream.pyx exposes a module-level CudaStream alias (or includes it in __all__)
and that stream.pyi contains a CudaStream stub; if CudaStream is not exported,
update the warning text in cuda_stream.pyx to point users to the correct public
name (e.g., Stream) or explain that only an internal _OwningStream exists,
referencing the symbols CudaStream, _OwningStream, __all__, stream.pyx and
stream.pyi so the author can locate and either add the public alias or change
the deprecation message accordingly.
🧹 Nitpick comments (2)
python/rmm/rmm/pylibrmm/cuda_stream.pyx (2)

12-14: _OwningStream leaks as an accessible name through the deprecated module.

from rmm.pylibrmm.stream import CudaStreamFlags, _OwningStream places _OwningStream in this module's namespace, making rmm.pylibrmm.cuda_stream._OwningStream reachable even though it is intentionally private. If the cimport refactor above is not taken, at minimum del the name after using it for the alias assignment.

♻️ Proposed fix
 from rmm.pylibrmm.stream import CudaStreamFlags, _OwningStream

 sys.modules[__name__].CudaStream = _OwningStream
+del _OwningStream
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.

In `@python/rmm/rmm/pylibrmm/cuda_stream.pyx` around lines 12 - 14, The module
currently imports the private symbol _OwningStream into its namespace and then
exposes it via sys.modules[...] assignment, leaking
rmm.pylibrmm.cuda_stream._OwningStream; after performing the alias assignment
(sys.modules[__name__].CudaStream = _OwningStream) remove the private name from
the module namespace (e.g., del _OwningStream) so only CudaStream (and
CudaStreamFlags) remain accessible; locate the import of _OwningStream and the
sys.modules assignment to implement the deletion.

8-8: Redundant cimport — nothing in this file uses CudaStream as a Cython type.

The cimport on line 8 binds _OwningStream as CudaStream at the C/Cython level, but this file defines no cdef functions, extension types, or typed variables that consume it. The actual Python-importable alias is established by sys.modules[__name__].CudaStream = _OwningStream on line 14.

♻️ Proposed simplification
-from rmm.pylibrmm.stream cimport _OwningStream as CudaStream
-
-import sys
+import sys

-from rmm.pylibrmm.stream import CudaStreamFlags, _OwningStream
+from rmm.pylibrmm.stream import CudaStream, CudaStreamFlags

-sys.modules[__name__].CudaStream = _OwningStream

This eliminates the cimport, drops the sys.modules manipulation, and imports CudaStream directly as a Python name — assuming stream.pyx exports it as a public alias (see verification below).

🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.

In `@python/rmm/rmm/pylibrmm/cuda_stream.pyx` at line 8, This file unnecessarily
uses a Cython cimport of _OwningStream as CudaStream and then mutates
sys.modules to expose it; instead remove the cimport and the
sys.modules[__name__].CudaStream assignment and import the Python-visible symbol
directly from the stream module (e.g., import CudaStream from
rmm.pylibrmm.stream or the public alias exported there), updating any references
to use that imported Python name (_OwningStream CudaStream and the sys.modules
manipulation are the symbols to remove/replace).
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.

Duplicate comments:
In `@python/rmm/rmm/pylibrmm/cuda_stream.pyx`:
- Around line 16-22: The deprecation warning directs users to
rmm.pylibrmm.stream for CudaStream but may be incorrect if stream.pyx doesn't
publicly export CudaStream; verify that stream.pyx exposes a module-level
CudaStream alias (or includes it in __all__) and that stream.pyi contains a
CudaStream stub; if CudaStream is not exported, update the warning text in
cuda_stream.pyx to point users to the correct public name (e.g., Stream) or
explain that only an internal _OwningStream exists, referencing the symbols
CudaStream, _OwningStream, __all__, stream.pyx and stream.pyi so the author can
locate and either add the public alias or change the deprecation message
accordingly.

---

Nitpick comments:
In `@python/rmm/rmm/pylibrmm/cuda_stream.pyx`:
- Around line 12-14: The module currently imports the private symbol
_OwningStream into its namespace and then exposes it via sys.modules[...]
assignment, leaking rmm.pylibrmm.cuda_stream._OwningStream; after performing the
alias assignment (sys.modules[__name__].CudaStream = _OwningStream) remove the
private name from the module namespace (e.g., del _OwningStream) so only
CudaStream (and CudaStreamFlags) remain accessible; locate the import of
_OwningStream and the sys.modules assignment to implement the deletion.
- Line 8: This file unnecessarily uses a Cython cimport of _OwningStream as
CudaStream and then mutates sys.modules to expose it; instead remove the cimport
and the sys.modules[__name__].CudaStream assignment and import the
Python-visible symbol directly from the stream module (e.g., import CudaStream
from rmm.pylibrmm.stream or the public alias exported there), updating any
references to use that imported Python name (_OwningStream CudaStream and the
sys.modules manipulation are the symbols to remove/replace).

ℹ️ Review info

Configuration used: Path: .coderabbit.yaml

Review profile: CHILL

Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between ce2a938 and 6346a4c.

📒 Files selected for processing (3)
  • .pre-commit-config.yaml
  • python/rmm/rmm/pylibrmm/cuda_stream.pyx
  • python/rmm/rmm/pylibrmm/stream.pyi
🚧 Files skipped from review as they are similar to previous changes (1)
  • python/rmm/rmm/pylibrmm/stream.pyi

@bdice bdice removed the request for review from jameslamb February 25, 2026 02:16
@bdice bdice moved this to In Progress in RMM Project Board Feb 25, 2026
Copy link
Copy Markdown

@coderabbitai coderabbitai bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Caution

Some comments are outside the diff and can’t be posted inline due to platform limitations.

⚠️ Outside diff range comments (1)
python/rmm/rmm/pylibrmm/stream.pyx (1)

58-84: ⚠️ Potential issue | 🟡 Minor

Validate flags when obj is provided.

Line 58 accepts flags for all calls, but Lines 74-84 ignore it whenever obj is not None. That silently drops caller intent and makes misconfiguration hard to detect.

🔧 Suggested guard
 def __init__(self, obj=None, flags=CudaStreamFlags.SYNC_DEFAULT):
@@
-    if obj is None:
+    if obj is None:
         self._init_with_new_cuda_stream(flags)
+    elif flags != CudaStreamFlags.SYNC_DEFAULT:
+        raise ValueError(
+            "flags is only supported when obj is None; "
+            "pass an existing stream via obj or create a new stream with flags."
+        )
     elif isinstance(obj, Stream):
         self._init_from_stream(obj)
Based on learnings: "Incorrect CUDA stream handling in Python bindings - ensure stream parameters are propagated correctly and stream synchronization is proper".
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.

In `@python/rmm/rmm/pylibrmm/stream.pyx` around lines 58 - 84, The __init__
currently accepts a flags argument but ignores it when obj is provided; update
__init__ to validate or propagate flags for non-None obj: in the branches that
call _init_from_stream, _init_from_cuda_stream_protocol,
_init_from_numba_stream, and _init_from_cupy_stream check whether flags is the
default (CudaStreamFlags.SYNC_DEFAULT) or matches any flags obtainable from the
provided obj (e.g. via a __cuda_stream__ attribute or stream property) and if
not raise a clear ValueError/TypeError; alternatively, if the underlying stream
wrapper supports creating a new stream with the requested flags, call the
appropriate _init_with_new_cuda_stream(flags) or adjust the wrapper initializer
to accept flags so caller intent isn’t silently dropped.
🧹 Nitpick comments (1)
python/rmm/rmm/tests/test_stream.py (1)

129-131: Make the deprecation assertion module-specific.

Line 129 currently accepts any DeprecationWarning, so unrelated warnings can make this pass. Consider asserting the expected message substring too.

🔧 Suggested tightening
-    assert any(
-        issubclass(warning.category, DeprecationWarning) for warning in w
-    )
+    assert any(
+        issubclass(warning.category, DeprecationWarning)
+        and "rmm.pylibrmm.cuda_stream is deprecated" in str(warning.message)
+        for warning in w
+    )
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.

In `@python/rmm/rmm/tests/test_stream.py` around lines 129 - 131, The current
assertion accepts any DeprecationWarning; tighten it by checking both the
category and the expected message substring in the collected warnings list `w`
so only the specific deprecation from this module passes. Replace the assertion
using `issubclass(warning.category, DeprecationWarning)` with one that also
checks the message (e.g. `any(issubclass(warning.category, DeprecationWarning)
and "expected deprecation message" in str(warning.message) for warning in w)`)
in the `test_stream.py` test that currently references `w`.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.

Outside diff comments:
In `@python/rmm/rmm/pylibrmm/stream.pyx`:
- Around line 58-84: The __init__ currently accepts a flags argument but ignores
it when obj is provided; update __init__ to validate or propagate flags for
non-None obj: in the branches that call _init_from_stream,
_init_from_cuda_stream_protocol, _init_from_numba_stream, and
_init_from_cupy_stream check whether flags is the default
(CudaStreamFlags.SYNC_DEFAULT) or matches any flags obtainable from the provided
obj (e.g. via a __cuda_stream__ attribute or stream property) and if not raise a
clear ValueError/TypeError; alternatively, if the underlying stream wrapper
supports creating a new stream with the requested flags, call the appropriate
_init_with_new_cuda_stream(flags) or adjust the wrapper initializer to accept
flags so caller intent isn’t silently dropped.

---

Nitpick comments:
In `@python/rmm/rmm/tests/test_stream.py`:
- Around line 129-131: The current assertion accepts any DeprecationWarning;
tighten it by checking both the category and the expected message substring in
the collected warnings list `w` so only the specific deprecation from this
module passes. Replace the assertion using `issubclass(warning.category,
DeprecationWarning)` with one that also checks the message (e.g.
`any(issubclass(warning.category, DeprecationWarning) and "expected deprecation
message" in str(warning.message) for warning in w)`) in the `test_stream.py`
test that currently references `w`.

ℹ️ Review info

Configuration used: Path: .coderabbit.yaml

Review profile: CHILL

Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between 6346a4c and e43195c.

📒 Files selected for processing (6)
  • python/rmm/rmm/librmm/cuda_stream.pxd
  • python/rmm/rmm/pylibrmm/cuda_stream.pyx
  • python/rmm/rmm/pylibrmm/stream.pxd
  • python/rmm/rmm/pylibrmm/stream.pyi
  • python/rmm/rmm/pylibrmm/stream.pyx
  • python/rmm/rmm/tests/test_stream.py

@bdice bdice merged commit 63ab12e into rapidsai:main Feb 25, 2026
77 checks passed
@github-project-automation github-project-automation bot moved this from In Progress to Done in RMM Project Board Feb 25, 2026
TomAugspurger added a commit to TomAugspurger/rapidsmpf that referenced this pull request Feb 25, 2026
rapidsai/rmm#2256 deprecated
`rmm.pylibrmm.cuda_stream` in favor of `rmm.pylibrmm.stream` for
importing `CudaStreamFlags`.

This should fix the import warnings showing up in
https://github.com/rapidsai/cudf/actions/runs/22411827835/job/64890756380?pr=21538.
rapids-bot bot pushed a commit to rapidsai/rapidsmpf that referenced this pull request Feb 25, 2026
rapidsai/rmm#2256 deprecated `rmm.pylibrmm.cuda_stream` in favor of `rmm.pylibrmm.stream` for importing `CudaStreamFlags`.

This should fix the import warnings showing up in
https://github.com/rapidsai/cudf/actions/runs/22411827835/job/64890756380?pr=21538.

Authors:
  - Tom Augspurger (https://github.com/TomAugspurger)

Approvers:
  - Bradley Dice (https://github.com/bdice)
  - Richard (Rick) Zamora (https://github.com/rjzamora)

URL: #888
bdice added a commit to bdice/rmm that referenced this pull request Mar 25, 2026
The rmm.pylibrmm.cuda_stream module was deprecated in PR rapidsai#2256 (closing
rapidsai#1782) when stream objects were moved to rmm.pylibrmm.stream. The
module was left as a backwards-compatibility shim re-exporting
CudaStream and CudaStreamFlags. Remove the shim module files (.pyx,
.pxd, .pyi), the associated deprecation test, and the CMake build
entry.
bdice added a commit that referenced this pull request Mar 31, 2026
## Summary
- Remove the deprecated `rmm.pylibrmm.cuda_stream`
backwards-compatibility shim module (`.pyx`, `.pxd`, `.pyi`)
- Remove the associated deprecation test
(`test_cuda_stream_module_deprecation`)
- Remove `cuda_stream.pyx` from the Cython build sources in
CMakeLists.txt

The module was deprecated in #2256 (closing #1782) when stream objects
were moved to `rmm.pylibrmm.stream`. Users should use
`rmm.pylibrmm.stream.Stream` and `rmm.pylibrmm.stream.CudaStreamFlags`
instead.

## Checklist
- [x] I am familiar with the [Contributing
Guidelines](https://github.com/rapidsai/rmm/blob/HEAD/CONTRIBUTING.md).
- [x] New or existing tests cover these changes.
- [x] The documentation is up to date with these changes.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

3 - Ready for review Ready for review by team improvement Improvement / enhancement to an existing function non-breaking Non-breaking change

Projects

Status: Done

Development

Successfully merging this pull request may close these issues.

[FEA] Deprecate rmm.pylibrmm.cuda_stream and move associated code to rmm.pylibrmm.stream

2 participants