fix: close issue #1863 Fix Cython CUDA stream pointer conversion#2254
Open
yihong0618 wants to merge 2 commits into
Open
fix: close issue #1863 Fix Cython CUDA stream pointer conversion#2254yihong0618 wants to merge 2 commits into
yihong0618 wants to merge 2 commits into
Conversation
Convert Python stream pointer integers through uintptr_t before casting to cudaStream_t. This prevents Cython from treating the int object address as the CUDA stream handle when using Resources(stream=stream.ptr) or MultiGpuResources(stream=stream.ptr). Signed-off-by: yihong0618 <zouzou0208@gmail.com>
|
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 (1)
💤 Files with no reviewable changes (1)
📝 WalkthroughSummary by CodeRabbit
WalkthroughTwo Cython files ( ChangesStream Pointer Casting Fix
Estimated code review effort🎯 2 (Simple) | ⏱️ ~10 minutes 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with 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.
Inline comments:
In `@python/cuvs/cuvs/tests/test_resources.py`:
- Around line 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.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Enterprise
Run ID: 04361215-a002-4a8b-9c0d-c90a3da66fcd
📒 Files selected for processing (3)
python/cuvs/cuvs/common/mg_resources.pyxpython/cuvs/cuvs/common/resources.pyxpython/cuvs/cuvs/tests/test_resources.py
Comment on lines
+12
to
+17
| 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() |
There was a problem hiding this comment.
🩺 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
Signed-off-by: yihong0618 <zouzou0208@gmail.com>
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
1 participant
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.
Convert Python stream pointer integers through uintptr_t before casting to cudaStream_t. This prevents Cython from treating the int object address as the CUDA stream handle when using Resources(stream=stream.ptr) or MultiGpuResources(stream=stream.ptr).
the reason as cython doc:
Cython type/cast syntax:
https://cython.readthedocs.io/en/latest/src/userguide/language_basics.html#statements-and-expressions
Cython pointer type documentation:
https://cython.readthedocs.io/en/latest/src/userguide/language_basics.html#types
The CPython documentation also states that extracting a C integer value from a Python int requires APIs such as PyLong_AsSize_t / PyLong_AsUnsignedLongLong:
PyLong_AsSize_t:
https://docs.python.org/3/c-api/long.html#c.PyLong_AsSize_t
At the same time, CPython also provides a dedicated PyLong_AsVoidPtr, which shows that converting a “Python integer to a C pointer value” is an explicit conversion step, and is not the same as directly casting a PyObject * to a pointer.
PyLong_AsVoidPtr:
https://docs.python.org/3/c-api/long.html#c.PyLong_AsVoidPtr
for the code side
the generated code
the new code make sure we get the value instead of the adress pyobject value