Skip to content

fix: close issue #1863 Fix Cython CUDA stream pointer conversion #2254

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.

Sign up for GitHub

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

Open
yihong0618 wants to merge 2 commits into
base: main
Choose a base branch
from
+27 −9
Open

fix: close issue #1863 Fix Cython CUDA stream pointer conversion #2254

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
460c312
fix: close issue #1863 Fix Cython CUDA stream pointer conversion
yihong0618 Jun 23, 2026
6381ff1
fix: pre-commit
yihong0618 Jun 23, 2026
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
12 changes: 7 additions & 5 deletions python/cuvs/cuvs/common/mg_resources.pyx
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,12 +1,13 @@
#
# SPDX-FileCopyrightText: Copyright (c) 2025, NVIDIA CORPORATION.
# SPDX-FileCopyrightText: Copyright (c) 2025-2026, NVIDIA CORPORATION.
# SPDX-License-Identifier: Apache-2.0
#
# cython: language_level=3

from cuda.bindings.cyruntime cimport cudaStream_t
from libc.stdint cimport uintptr_t

from cuvs.common.c_api cimport (
cudaStream_t,
cuvsMultiGpuResourcesCreate,
cuvsMultiGpuResourcesCreateWithDeviceIds,
cuvsMultiGpuResourcesDestroy,
Expand Down Expand Up @@ -84,11 +85,12 @@ cdef class MultiGpuResources:
else:
check_cuvs(cuvsMultiGpuResourcesCreate(&self.c_obj))

if stream:
check_cuvs(cuvsStreamSet(self.c_obj, <cudaStream_t>stream))
if stream is not None:
check_cuvs(cuvsStreamSet(
self.c_obj, <cudaStream_t><uintptr_t>stream))

def sync(self):
check_cuvs(cuvsStreamSync(self.c_obj))
check_cuvs(cuvsStreamSync(self.c_obj))

def set_memory_pool(self, percent_of_free_memory):
"""
Expand Down
9 changes: 5 additions & 4 deletions python/cuvs/cuvs/common/resources.pyx
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,14 +1,15 @@
#
# SPDX-FileCopyrightText: Copyright (c) 2024, NVIDIA CORPORATION.
# SPDX-FileCopyrightText: Copyright (c) 2024-2026, NVIDIA CORPORATION.
# SPDX-License-Identifier: Apache-2.0
#
# cython: language_level=3

import functools

from cuda.bindings.cyruntime cimport cudaStream_t
from libc.stdint cimport uintptr_t

from cuvs.common.c_api cimport (
cudaStream_t,
cuvsResources_t,
cuvsResourcesCreate,
cuvsResourcesDestroy,
Expand Down Expand Up @@ -54,8 +55,8 @@ cdef class Resources:

def __cinit__(self, stream=None):
check_cuvs(cuvsResourcesCreate(&self.c_obj))
if stream:
check_cuvs(cuvsStreamSet(self.c_obj, <cudaStream_t>stream))
if stream is not None:
check_cuvs(cuvsStreamSet(self.c_obj, <cudaStream_t><uintptr_t>stream))

def sync(self):
check_cuvs(cuvsStreamSync(self.c_obj))
Expand Down
15 changes: 15 additions & 0 deletions python/cuvs/cuvs/tests/test_resources.py
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,15 @@
# SPDX-FileCopyrightText: Copyright (c) 2026, NVIDIA CORPORATION.
# SPDX-License-Identifier: Apache-2.0


import cupy as cp

from cuvs.common import Resources


def test_resources_syncs_cupy_stream_pointer():
# gh-issue: 1836 should not segfault when syncing a stream pointer from cupy
stream = cp.cuda.Stream()
resources = Resources(stream=stream.ptr)

resources.sync()
Comment on lines +10 to +15

@coderabbitai coderabbitai Bot Jun 23, 2026

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

🩺 Stability & Availability | 🟠 Major | ⚡ Quick win

HIGH: Missing regression test for MultiGpuResources(stream=stream.ptr) cast path.

Have you considered adding a sibling regression test for MultiGpuResources with a raw CuPy stream pointer? This PR modifies that constructor path too, so without coverage a similar pointer-cast bug could regress into a crash unnoticed.

Proposed test addition 
 import cupy as cp
 import cuvs.common.resources as resources_mod
 
-from cuvs.common import Resources
+from cuvs.common import MultiGpuResources, Resources
 
 
 def test_resources_syncs_cupy_stream_pointer():
     # gh-issue: 1836 should not segfault when syncing a stream pointer from cupy
     stream = cp.cuda.Stream()
     resources = Resources(stream=stream.ptr)
 
     resources.sync()
+
+
+def test_multi_gpu_resources_syncs_cupy_stream_pointer():
+    stream = cp.cuda.Stream()
+    resources = MultiGpuResources(stream=stream.ptr, device_ids=[0])
+    resources.sync()

As per coding guidelines, HIGH issues include substantial missing edge-case coverage in tests for changed paths.

🤖 Prompt for AI Agents 
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@python/cuvs/cuvs/tests/test_resources.py` around lines 12 - 17, Add a sibling
regression test for the MultiGpuResources constructor path with a raw CuPy
stream pointer. Create a new test function following the same pattern as
test_resources_syncs_cupy_stream_pointer that instantiates MultiGpuResources
with a CuPy stream pointer (stream.ptr) and calls sync() on it. This ensures
coverage for the MultiGpuResources constructor's pointer-cast path which was
also modified in this PR, preventing similar pointer-cast bugs from regressing
undetected.

Source: Coding guidelines

Loading