-
Notifications
You must be signed in to change notification settings - Fork 46
Revert #536 "perf: remove context threading in various pointer abstractions" #611
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
Conversation
NVIDIA#536)" This reverts commit 9a56516. This changed the public API of `MemoryPointer` and related classes, and the context that they held was used by Arrow (see apache/arrow#48259 (comment)): > Numba interop tests fail with: ``` arrow-dev/lib/python3.12/site-packages/pyarrow/tests/test_cuda_numba_interop.py:233: _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ > ??? E TypeError: MemoryPointer.__init__() got multiple values for argument 'pointer' ``` This commit reverts the change, as it was intended to improve performance without changing functionality, but has had a functional change as a side effect. Following the merge of this PR, we should be able to remove some of the `@require_context` decorators with some more targeted changes.
|
Auto-sync is disabled for ready for review pull requests in this repository. Workflows must be run manually. Contributors can view more details about this message here. |
|
/ok to test |
Greptile OverviewGreptile SummaryThis PR cleanly reverts PR #536, which removed the Key Changes:
Justification: Confidence Score: 5/5
Important Files ChangedFile Analysis
Sequence DiagramsequenceDiagram
participant User as External User/Arrow
participant API as from_cuda_array_interface()
participant Context as CUDA Context
participant MemPtr as MemoryPointer
participant DevArray as DeviceNDArray
Note over User,DevArray: Before Revert (PR #536 - BROKEN)
User->>API: Call with cuda-array-interface
API->>MemPtr: MemoryPointer(pointer, size, owner)
Note over MemPtr: No context parameter!
MemPtr-->>API: Memory pointer instance
API->>DevArray: Create DeviceNDArray
DevArray-->>User: Return array
Note over User: ❌ Arrow breaks: multiple values for 'pointer'
Note over User,DevArray: After Revert (Current PR - FIXED)
User->>API: Call with cuda-array-interface
API->>Context: current_context()
Context-->>API: Context instance
API->>MemPtr: MemoryPointer(context, pointer, size, owner)
Note over MemPtr: Context restored as first param
MemPtr->>MemPtr: Store context reference
MemPtr-->>API: Memory pointer instance
API->>DevArray: Create DeviceNDArray
DevArray-->>User: Return array
Note over User: ✅ Arrow works correctly
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
5 files reviewed, no comments
|
Following this PR: create an issue to track the work of re-removing |
- Revert NVIDIA#536 "perf: remove context threading in various pointer abstractions" (NVIDIA#611) - fix: empty array type mismatch between host and device (NVIDIA#612) - fix: warp vote operations must use a constant int for the `mode` parameter (NVIDIA#606)
### What changes are included in this PR? 1. Add compatibility fix for CUDA 13 in C++ CUDA tests 2. Add CI builds with CUDA 13 3. Disable Numba interop tests because of a regression in Numba-CUDA: see NVIDIA/numba-cuda#611 ### Are these changes tested? Yes, on CI. ### Are there any user-facing changes? No. * GitHub Issue: #47677 Authored-by: Antoine Pitrou <[email protected]> Signed-off-by: Raúl Cumplido <[email protected]>
This reverts commit 9a56516.
This changed the public API of
MemoryPointerand related classes, and the context that they held was used by Arrow (see apache/arrow#48259 (comment)):This commit reverts the change, as it was intended to improve performance without changing functionality, but has had a functional change as a side effect. Following the merge of this PR, we should be able to remove some of the
@require_contextdecorators with some more targeted changes.