Repository navigation
Conversation
…alMemoryFdBufferDescriptor The class carries POSIX file descriptors for VirtualMemoryResource buffers. A fabric-handle descriptor will sit beside it, so its name now says which transport it is. No alias: the class first appears in the unreleased 1.3.0. No behavior change. Written by Codex (gpt-6.1-sol); reviewed by Claude. Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>
Add VirtualMemoryFabricBufferDescriptor, a plain-data descriptor that carries one opaque 64-byte CUmemFabricHandle per physical allocation, the allocation sizes, the buffer size, and the exporting device's UUID. VirtualMemoryResource exports it when config.handle_type is "fabric", and Buffer.from_ipc_descriptor imports it through the shared _VirtualMemoryDescriptor interface that the POSIX fd descriptor also implements, so both transports run one import loop. Fabric descriptors travel through plain pickle and multiprocessing without the file-descriptor machinery and pin nothing: the exporter keeps the buffer alive until every importer has imported it. A stale or forged handle fails at import with a driver error and a note. Resource construction checks Device.properties.handle_type_fabric_supported and raises RuntimeError when the device lacks an NVLink fabric or an accessible IMEX channel. from_handles() rebuilds a descriptor received through a transport of your own. A location mismatch at import names the owning device by UUID. Sharing is supported on one node; multi-node sharing is untested. The project's CI has no fabric-capable runner; the release note says so. Part of NVIDIA#2980. Written by Codex (gpt-6.1-sol); reviewed by Claude. Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>
Andy-Jost
added
P0
|
No actionable comments were generated in the recent review. Summary
Change: Feature Merge Risk:
 |
![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
-
-
- Source mode: Auto
- Repository: NVIDIA/cuda-python
- Branch: main
- File: .coderabbit.yaml
- Commit: 2e6aa4e
- Layers: Repository YAML
- Review profile: CHILL
- Plan: Enterprise
- Run ID:
2567ea85-9d0d-47b7-8ab6-6d6eac163733
-
cuda_core/cuda/core/_cpp/rt/VMM_DESIGN.mdcuda_core/cuda/core/_memory/_ipc.pxdcuda_core/cuda/core/_memory/_ipc.pyicuda_core/cuda/core/_memory/_ipc.pyxcuda_core/cuda/core/_memory/_virtual_memory_resource.pyicuda_core/cuda/core/_memory/_virtual_memory_resource.pyxcuda_core/docs/source/api_private.rstcuda_core/docs/source/release/1.3.0-notes.rstcuda_core/tests/conftest.pycuda_core/tests/memory_ipc/test_vmm_fabric.pycuda_core/tests/memory_ipc/test_vmm_ipc.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.
-
|
Zero the 64-byte handle struct before a fabric export. The driver fills its fields and leaves the padding, so two exports of one buffer carried different stack bytes (seen on B300); the export now returns the same bytes for the same allocation, and no stack bytes travel in a descriptor. The fabric test fixtures allocate once per device after the attribute check. Inside a container that has the IMEX channel device node the attribute is true while the GPU has no fabric, and the allocation is refused; that is what the CI runners do. A refused physical allocation on a fabric resource now carries a note about the fabric and IMEX requirements, and the docs say that the attribute can be true where the allocation still fails. The import-error tests check cuda.core's note, not the driver's status, which varies with the IMEX setup. Garbage handles use fixed patterns instead of os.urandom. The release note names the POSIX fd descriptor as the one that pins. One note per import failure. _handles is readonly on both descriptors. Written by Claude after the de novo review, CodeRabbit, CI, and the Hopper and B300 runs. Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>
There was a problem hiding this comment.
Actionable comments posted: 2
-
-
- Source mode: Auto
- Repository: NVIDIA/cuda-python
- Branch: main
- File: .coderabbit.yaml
- Commit: 2e6aa4e
- Layers: Repository YAML
- Review profile: CHILL
- Plan: Enterprise
- Run ID:
d22607f1-f983-48de-9326-f14eb1a4e683
-
cuda_core/cuda/core/_cpp/rt/VMM_DESIGN.mdcuda_core/cuda/core/_memory/_ipc.pxdcuda_core/cuda/core/_memory/_ipc.pyicuda_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/conftest.pycuda_core/tests/memory_ipc/test_vmm_fabric.py
-
- cuda_core/docs/source/release/1.3.0-notes.rst
- cuda_core/cuda/core/_cpp/rt/VMM_DESIGN.md
- cuda_core/cuda/core/_memory/_virtual_memory_resource.pyi
Included review availability: This review used your included allowance. Your plan provides up to 12 included reviews per hour; 10 remain after this review.
-
…r helper Cython reported the stray line as unreachable code, and the wheel builds treat Cython warnings as errors. The return goes back to the note helper it belonged to. Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>
The fixture probe skipped every fabric test on any CUDAError from the probe allocation, which would hide a real failure (an out-of-memory condition, for one) as 'no fabric here'. It now skips on the statuses that mean the device has no usable fabric (NOT_PERMITTED on the CI runners, INVALID_VALUE on an H200 without an IMEX channel, NOT_SUPPORTED) and lets anything else propagate, per the test guide's rule on capability probes. Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>
There was a problem hiding this comment.
Actionable comments posted: 1
-
-
- Source mode: Auto
- Repository: NVIDIA/cuda-python
- Branch: main
- File: .coderabbit.yaml
- Commit: 590a8bf
- Layers: Repository YAML
- Review profile: CHILL
- Plan: Enterprise
- Run ID:
2629eefa-b0a5-4280-a743-a6c75ccd5260
-
cuda_core/tests/conftest.pycuda_core/tests/memory_ipc/test_vmm_ipc.py
Included review availability: This review used your included allowance. Your plan provides up to 12 included reviews per hour; 11 remain after this review.
-
…bric Every conflict was the rename that this branch already carried as f128963; main's version of each conflicted file equals that commit's, so the branch side was kept.
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to GitHub limitations.
1.3.0-notes.rst:88-92
cuda_core/docs/source/release/1.3.0-notes.rst:88-92
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick winsuggestion: Limit the file-descriptor description to
"posix_fd". The introduction now includes"fabric"resources, but these sentences still say that every export produces POSIX file descriptors and cannot use plainpickle. A fabric export produces a fabric handle that can travel as data. Separate the two transport descriptions so readers do not apply FD-handling instructions to fabric descriptors. (docs.nvidia.com) As per coding guidelines, “Keep user-facing behaviors coherent with docs and samples, especially around stream semantics, memory ownership, and compile/link flows.”Source: Coding guidelines
-
-
- Source mode: Auto
- Repository: NVIDIA/cuda-python
- Branch: main
- File: .coderabbit.yaml
- Commit: f9212dd
- Layers: Repository YAML
- Review profile: CHILL
- Plan: Enterprise
- Run ID:
2e1bc7ca-b386-43d3-835b-9c6f4623ec84
-
cuda_core/cuda/core/_cpp/rt/VMM_DESIGN.mdcuda_core/cuda/core/_memory/_ipc.pxdcuda_core/cuda/core/_memory/_ipc.pyicuda_core/cuda/core/_memory/_ipc.pyxcuda_core/cuda/core/_memory/_virtual_memory_resource.pyicuda_core/cuda/core/_memory/_virtual_memory_resource.pyxcuda_core/docs/source/api_private.rstcuda_core/docs/source/release/1.3.0-notes.rstcuda_core/tests/memory_ipc/test_vmm_ipc.py
Included review availability: This review used your included allowance. Your plan provides up to 12 included reviews per hour; 11 remain after this review.
-
The fixture probe skipped the fabric tests on three driver statuses from the whole probe allocation. A status from the address reservation, the mapping, or the stream setup could therefore pass as 'no fabric here' and hide a real failure on a fabric-capable device. The probe now skips only when the exception carries the note that VirtualMemoryResource attaches to a refused physical allocation with a fabric handle (appended to the message on Python 3.10), and lets every other CUDAError propagate. The 1.3.0 release note described every export as a POSIX file descriptor that plain pickle cannot carry. It now describes the two transports separately: file descriptors for "posix_fd", plain-data fabric handles for "fabric". Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>
…ation fails The docstrings, the allocation note, the release note, and the fixture comment explained the over-reporting attribute with a container that has the IMEX channel device node. The CI evidence says otherwise: the runner VMs disable NVLink and have no channel node, and the attribute is still true on the r595 and 610 drivers there. The 13.4 driver source adds the channel subscription to the attribute; no driver checks for an NVLink fabric. The text now states that, without the container example. Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>
| _IPC_HANDLE_TYPES = ( | ||
| frozenset({VirtualMemoryHandleType.POSIX_FD, VirtualMemoryHandleType.FABRIC}) | ||
| if platform.system() == "Linux" else frozenset() | ||
| ) |
There was a problem hiding this comment.
One thing I noticed is that there's technically two places that a descriptor... "describes" what it is. It carries an internal _handle_type, and it captures that information in the subclass type. This matters because there's two now and the import guard at line 1047 validates only the field, by comparing it against the resource's configured type. So you can build a bad VirtualMemoryFdBufferDescriptor whose field is forged to say fabric which then dispatches to the fd implementation, which blows up.
The code smell here abstractly is that there is one fact with two representations. The guard asks "does this descriptor agree with this resource?" and never asks "does this descriptor agree with itself?" Internal consistency between the two copies is simply assumed, so the moment they can diverge, the external check validates the wrong thing.
There was a problem hiding this comment.
We should make the class the authority
if isinstance(desc, VirtualMemoryFabricBufferDescriptor) != (
handle_type == <int>cydriver.CU_MEM_HANDLE_TYPE_FABRIC):
raise ValueError(
f"the descriptor claims handle_type={str(cfg.handle_type)!r} but carries "
f"{type(desc).__name__} handles; it was not produced by this resource's transport"
)There was a problem hiding this comment.
I refactored this so each subclass defines its handle type, removing the duplicated per-instance state. I generally don’t think we need to defend against users tampering with internal data structures; constructors and API methods should maintain the invariants.
|
|
||
| cdef int _raise_allocation_error(const cydriver.CUmemAllocationProp* prop) except -1: | ||
| """Raise the status of a failed physical allocation; a fabric resource gets the IMEX note.""" | ||
| if prop.requestedHandleTypes == cydriver.CU_MEM_HANDLE_TYPE_FABRIC: |
There was a problem hiding this comment.
Peek last error here right? If I OOM a fabric resource I should obtain those details id assume.
| if prop.requestedHandleTypes == cydriver.CU_MEM_HANDLE_TYPE_FABRIC: | |
| if (prop.requestedHandleTypes == cydriver.CU_MEM_HANDLE_TYPE_FABRIC | |
| and peek_last_error() in ( | |
| cydriver.CUDA_ERROR_NOT_PERMITTED, | |
| cydriver.CUDA_ERROR_NOT_SUPPORTED, | |
| cydriver.CUDA_ERROR_INVALID_VALUE, | |
| )): |
There was a problem hiding this comment.
Agree. I redid the function signature to make it more direct as well.
| allocation holds its own driver references and is independent of the | ||
| exporter. Importing a stale or forged handle fails with a driver error. | ||
|
|
||
| Sharing is supported on one node; multi-node sharing is untested. Fabric |
There was a problem hiding this comment.
The untested thing raises an eyebrow from me. Ideally we have some way of testing the hot paths.
One maybe overkill idea is to simulate the fabric environment by building our own driver library that lies about the situation. Some back and forth with an agent can quickly produce something vibecoded here that works, see the gist below as an example
Something like this is fairly heavy but the three options seem to all be heavy:
- ship documented untested code (a little scary feeling given its multi node/ multi GPU critical code)
- Deploy real multinode CI infra for a very limited slice of features
- Simulate somehow
FWIW, the above gets me from 18 skips down to 1 on a regular machine.
There was a problem hiding this comment.
Thanks for putting this together. I agree the CI coverage gap is not ideal, and we have few good options. This approach would provide useful coverage of cuda-core’s integration logic. I'd say the added scope and maintenance burden seem significant enough to warrant a separate discussion and PR, with a chance for others to weigh in. Does that work for you?
| ) | ||
| def test_is_ipc_enabled(vmm_ipc_device, options, expected): | ||
| """Only POSIX file descriptors on device memory have a transport; host-located memory is not shared yet.""" | ||
| """POSIX file descriptors on device memory have a transport; host-located memory is not shared yet.""" |
There was a problem hiding this comment.
This PR trades dropping pytest.param({"handle_type": "fabric"}, False, id="fabric"), from the above for tests that skip unless we're on fabric enabled hardware. The side effect of this is that reverting the central change doesn't cause a test failure. We should just add
pytest.param({"handle_type": "fabric"}, True, id="fabric"),
Back.
| mr = _resource(device) | ||
| assert mr.is_ipc_enabled | ||
| for location in ("host", "host_numa", "host_numa_current"): | ||
| host = _resource(device, location_type=location, handle_type=None if location == "host" else "fabric") |
There was a problem hiding this comment.
I can't see how this test ever runs. Here we're constructing either a host_numa resource with no numa_id or a host_numa_current, both seem to be rejected unconditionally in _check_config. I believe anywhere the fixture doesn't skip this test due to not being on the right hardware, this fails before asserting anything fabric related.
There was a problem hiding this comment.
Fixed. A valid host_numa fabric case now checks is_ipc_enabled=False in the configuration test.
| fabric, fd = _resource(device), _resource(device, handle_type="posix_fd") | ||
| # Empty descriptors would need no import call; even these must reject a mismatch. | ||
| for exporting, importing in ((fabric, fd), (fd, fabric)): | ||
| with exporting.allocate(0) as buf, pytest.raises(ValueError, match="configured like the exporter's"): |
There was a problem hiding this comment.
If all we do is allocate(0) this test doesn't appear to need a fabric environment so it need not be present under the skipped tests (if thats the route we choose)
| prefix = VirtualMemoryFabricBufferDescriptor.from_handles(desc.handles, desc.sizes, size=1) | ||
| with Buffer.from_ipc_descriptor(mr, prefix, stream=device.default_stream) as alias: | ||
| assert alias.size == 1 < sum(prefix.sizes) | ||
| assert not any(hasattr(desc, name) for name in ("fds", "close", "is_closed")) |
There was a problem hiding this comment.
Nit: 2/3rds of the assertion is vacuous, because if this seeks to exclude the fd descriptor, fds is true, close and is_closed are both False.
|
|
||
|
|
||
| def _notes(exc): | ||
| # Python 3.10 appends the note to the message instead of using PEP 678. |
There was a problem hiding this comment.
Aren't we off 3.10 now?, guessing this can be removed
There was a problem hiding this comment.
Good call. This PR was started before we removed the Python 3.10 coverage. I filed #3374 to sweep the code for obsolete Python 3.10 constructs before the release and I'll update this PR to remove these things.
| except CUDAError as exc: | ||
| # Only a refused physical allocation means "no fabric here" (NOT_PERMITTED on the CI runners, | ||
| # INVALID_VALUE on an H200 without an IMEX channel). VirtualMemoryResource attaches its note about | ||
| # the fabric and IMEX requirements at that stage alone (on Python 3.10 it lands in the message); |
brandon-b-miller
left a comment
brandon-b-miller
left a comment
There was a problem hiding this comment.
Negative results (agent attacks)
The refactored POSIX-fd transport held under 54 attacks on real hardware. Mid-loop chunk failures leaked neither file descriptors nor GPU memory; descriptor exhaustion during export rolled back to an exact 63-fd baseline, confirming the code's own claim that "the handle owns the fd now; if append fails, the handle closes it once"; hostile teardown orders, reference cycles, and survival to interpreter shutdown were clean; import from a thread with no CUDA context works; and a five-chunk cross-process round trip verified all 1,310,720 words of a position-dependent pattern, which proves the shared loop maps chunks at the right offsets rather than merely returning a pointer.
Mid-loop unwinding leaks nothing and is symmetric. The import loop runs at lines 1104-1154 and the address reservation is at 1156, after it, so a failure at chunk i > 0 happens with i imported allocations and no reservation. Measured four ways with free device memory from cuMemGetInfo and a live-reservation count — a real Python-level raise (closing an IPCAllocationHandle mid-loop) and a real driver-level failure both returned to baseline exactly, with the 4 MiB mid-test delta showing the measurement could have seen a one-chunk leak.
|
This looks pretty good. Found a few places that could use some hardening or are losing some details. Biggest thing is the testing for sure, which seems like the only major decision item from my side. |
Summary
Buffers of a
VirtualMemoryResourceconfigured withhandle_type="fabric"can now be shared between processes on one node. This adds the fabric transport that #2980 tracks, beside the POSIX fd transport from #3028.Changes
VirtualMemoryFabricBufferDescriptor(listed inapi_private.rst): one opaque 64-byteCUmemFabricHandleper physical allocation, the allocation sizes, the buffer size, and the exporting device's UUID. It is plain data:pickleandmultiprocessingcarry it without the file-descriptor machinery, it compares and hashes by value, andfrom_handles()rebuilds one received through a transport of your own, with validation. The export zeroes the handle struct first, so two exports of one allocation carry the same bytes and no stack bytes travel in a descriptor._VirtualMemoryDescriptor, soBuffer.from_ipc_descriptorand the resource's import run one loop for both transports. The fd descriptor keeps its ownership and identity semantics and itsfrom_fds().VirtualMemoryResourceconstruction checksDevice.properties.handle_type_fabric_supportedforhandle_type="fabric"and raisesRuntimeErrorthat names the NVLink fabric and IMEX channel requirements, before an allocation can fail opaquely. A device can report support while the allocation is refused (the attribute does not check for an NVLink fabric, and older drivers do not check for an IMEX channel); a refused allocation then carries a note that says so.is_ipc_enabledisTruefor"fabric"on Linux with device-located memory.VMM_DESIGN.mdsay so wherever descriptor lifetime is described._ipc.pyiand_virtual_memory_resource.pyiare regenerated by the stub hook.tests/memory_ipc/test_vmm_fabric.pycovers export shape, descriptor validation and pickling, Queue, Pipe, Process-argument, and Pool round trips for descriptors and buffers, a second-GPU import with peer access, handle-type mismatch, garbage and stale handles, re-export of imports, a queued buffer that does not pin, and the unsupported-device error. The fabric fixtures skip on devices without fabric handle support, and on devices that report it but refuse the allocation (one probe allocation per device).Validation
Fabric handles need an NVLink fabric and an accessible IMEX channel. The project's CI has no such runner, so the fabric cases skip there; the release note says that this release's fabric support is not covered by CI. On the CI runners the attribute is true while every fabric allocation is refused with a driver error: the runner VMs disable NVLink and have no IMEX channel, and the attribute checks neither on those drivers. That is why the fixtures probe instead of trusting the attribute.
Validation on NVIDIA internal hardware: a two-GPU B300 system ran the
memory_ipcsuite andtest_memory.pytwice: on the first commit (478 passed, 21 skipped; the one failure found the uninitialized padding in the exported handle struct) and on the fix and on the PR head withmainmerged (479 passed, 21 skipped each), and a Hopper system without fabric support ran the fabric and fd suites (30 passed, 17 skipped, with the unsupported-device error test among the passes). The fabric suite also passed inside a container on the B300 system (a stock CUDA base image with the GPUs injected by the NVIDIA container CLI and the IMEX channel device node mounted; 18 passed, 1 skipped, three runs). Multi-node sharing is out of scope and untested.Related Work
Part of #2980. Follows #3028 and #3229; this PR includes the #3229 commit until that PR merges.
🤖 Generated with Claude Code