Repository navigation
Conversation
A DeviceMemoryResource sent through multiprocessing, and a pickled VirtualMemoryResource, named their device by ordinal, and the VirtualMemoryResource also sent its peers as ordinals. Ordinals are per process, so a receiver with a different CUDA_VISIBLE_DEVICES rebuilt the resource on the wrong GPU or granted peer access to the wrong GPU. Send the owner and the peers by UUID and map them back in the receiver with a new private helper. Raise a clear RuntimeError when no visible device has the UUID. The tests reverse the device order in the child through CUDA_VISIBLE_DEVICES and check the imported buffer's device UUID. Closes NVIDIA#3097 Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>
The check-cython-not-none hook from main requires it on a def parameter annotated with a cdef class. Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>
Andy-Jost
added
bug
📝 Summary
Priority: ➖ Normal Change: Bug fix · Severity of issue fixed: Medium Merge Risk: 🔵 Low · up to
|
![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
ℹ️ Review info
⚙️ Run configuration
- Configuration used: Repository: NVIDIA/cuda-python/.coderabbit.yaml
- Review profile: CHILL
- Plan: Enterprise
- Run ID:
72c7d735-1982-4195-8b61-d8894498ad10
📒 Files selected for processing: 8
cuda_core/cuda/core/_device.pyicuda_core/cuda/core/_device.pyxcuda_core/cuda/core/_memory/_device_memory_resource.pyicuda_core/cuda/core/_memory/_device_memory_resource.pyxcuda_core/cuda/core/_memory/_virtual_memory_resource.pyicuda_core/cuda/core/_memory/_virtual_memory_resource.pyxcuda_core/docs/source/release/1.3.0-notes.rstcuda_core/tests/memory_ipc/test_device_uuid.py
Included review availability: This review used your included allowance. Your plan provides up to 12 included reviews per hour; 10 remain after this review.
| # Device ordinals are per process, so the owning device and the peers | ||
| # travel by UUID and the receiver maps them to its own devices. | ||
| owner_uuid = self.device.uuid if self.device is not None else None | ||
| peer_uuids = tuple(Device(peer).uuid for peer in self.config.peers) |
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🟡 Minor | ⚡ Quick win
important: Preserve peers from one-shot iterables.
If VirtualMemoryResourceOptions.peers is a generator, allocation consumes it in _fill_access. A later buffer pickle serializes no peer UUIDs. The receiving resource then omits access for a peer that could access the exported buffer. Store peers as a reusable tuple when the resource accepts its configuration, and test a buffer transfer with generator-supplied peers.
Contributor
|
This branch has not been deployed
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.
Summary
A
DeviceMemoryResourcesent throughmultiprocessing, and a pickledVirtualMemoryResource, named their device by ordinal, and theVirtualMemoryResourcealso sent itspeersas ordinals. Device ordinals are per process, so a receiver with a differentCUDA_VISIBLE_DEVICESrebuilt the resource on the wrong GPU, or granted peer access to the wrong GPU and only faulted later. This PR sends the owning device and the peers by UUID and maps them back in the receiving process.Changes
_device.pyx: new private helper_device_from_uuid()that scans the visible devices once and raises aRuntimeErrornaming the missing UUID and the visible devices when nothing matches._device_memory_resource.pyx: themultiprocessingreducer sends(type, device UUID, allocation handle)and rebuilds throughfrom_allocation_handle()on the receiver's device with that UUID._virtual_memory_resource.pyx:__reduce__sends the owner UUID (Nonefor host-located memory), the peer UUIDs, and the options withoutpeers; the receiver rebuilds with its own ordinals..pyistubs, and a 1.3.0 release-note entry.tests/memory_ipc/test_device_uuid.py: the child process sees the two GPUs in reversed order through a UUID-formCUDA_VISIBLE_DEVICES, and asserts that the imported buffer's device has the parent's UUID, for pool-backed buffers, VMM buffers, and a VMM resource withpeers. A negative test hides the owning GPU and checks the error. The tests need two visible devices and skip otherwise.Device.__reduce__still pickles the ordinal. Changing it would alter behavior for code that sends aDeviceto mean "this ordinal", so it is left for a separate decision.Testing
Validated on a two-GPU Linux host: the memory IPC suite plus
test_memory.pypass, including the five new tests.Related Work
Closes #3097. Follow-up to #3028 (reported in review there by @juenglin and @brandon-b-miller).
🤖 Generated with Claude Code