-
Notifications
You must be signed in to change notification settings - Fork 221
Revert "Implement IPC-enabled events. (#1145)" #1173
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
This reverts commit 20f29e9.
|
/ok to test 1d5bd94 |
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.
Greptile Overview
Greptile Summary
This PR completely reverts commit 20f29000000000.07 which implemented IPC-enabled events for cuda.core.experimental. The revert removes all IPC event functionality from the Event and Stream classes by deleting IPCEventDescriptor, get_ipc_descriptor(), from_ipc_descriptor(), multiprocessing serialization support, and the _ipc_enabled flag. Test infrastructure added for IPC events is removed (test files, helper modules including buffers.py, latch.py, logging.py) or restored to previous implementations. The remaining IPC memory tests revert from PatternGen to IPCBufferTestHelper and add platform compatibility checks using supports_ipc_mempool() to gracefully skip tests on systems where the driver rejects IPC-enabled mempool creation. The Event.is_ipc_supported property now raises NotImplementedError instead of returning a boolean, marking IPC support as still work-in-progress. The revert affects only the experimental API and leaves the broader cuda-bindings layer unchanged.
Potential Issues:
-
Breaking API change in
Event.is_ipc_supported: The property changed from returning a boolean to raisingNotImplementedError(lines 165-167 in_event.pyx). Any code checkingif event.is_ipc_supported:will now crash instead of returning False. Consider deprecation warnings before breaking changes in experimental APIs. -
Type safety regression in
DeviceMemoryResourceOptions.max_size: Changed fromcython.size_t(unsigned) tocython.int(signed) in_memory.pyxline 514. Negative values could now be passed where memory sizes should always be non-negative, potentially causing undefined behavior in CUDA driver calls. -
Missing platform check in
TestIPCSharedAllocationHandleAndBufferObjects: Unlike the other three IPC test classes (lines 19-22, 60-63, 112-115 intest_memory_ipc.py), this test class at line 163 lacks thesupports_ipc_mempool()check, which may cause test failures on WSL or platforms with limited IPC support. -
Test helper instantiation pattern creates potential state bugs: In
test_send_buffers.pyandtest_serialize.py,IPCBufferTestHelperinstances are created multiple times per buffer (once for fill, once for verify) rather than reused. If the helper's internal state depends on initialization context (stream creation, scratch buffer allocation), this could lead to subtle verification failures. -
API semantic equivalence assumption: The revert assumes
PatternGen.fill_buffer(seed=True)is equivalent toIPCBufferTestHelper.fill_buffer(flipped=True). If the data patterns differ, tests will pass but validate incorrect buffer contents. The inconsistent API intest_memory_ipc.py(some tests useflipped=True/False, others usestarting_from=<int>) suggests incomplete refactoring.
Confidence: 3/5 - This is a clean revert with clear intent, but the breaking API change in is_ipc_supported, the type regression in max_size, and potential test coverage gaps reduce confidence. The revert removes a significant feature, so thorough testing on all supported platforms (especially WSL and multi-GPU systems) is critical before merge.
Additional Comments (1)
-
cuda_core/tests/memory_ipc/test_memory_ipc.py, line 163 (link)logic: Missing IPC mempool support check. Other test classes guard with
if not supports_ipc_mempool(ipc_device):but this one doesn't.
20 files reviewed, 5 comments
This comment has been minimized.
This comment has been minimized.
|
This reverts commit 20f29e9.