Repository navigation
test(cuda.core): gate the init_cuda teardown gc.collect() on live owned-resource counts - #3053
Conversation
Tests that allocate through Device.memory_resource /
Device.allocate or construct any memory resource without init_cuda did
Device() + set_current() by hand, leaving a stale cached
default MemoryResource on the thread-local Device: no teardown
gc.collect(), no cuCtxSynchronize, and the context never popped.
The stale slot is the aliasing route for the planned
live-resource-count gc.collect() gate, and the tests violate the
tests/AGENTS.md rule that constructing a memory resource counts as
touching CUDA.
Route both groups onto init_cuda (using the yielded device) so the
fixture teardown clears the cached slot via del _tls.devices:
- device.memory_resource allocators: 6 test_from_buffer* functions in
test_utils.py and test_buffer_external_device in test_memory.py.
- resource constructors: 28 functions in test_memory.py (buffers,
dlpack, vmm, pinned/managed ipc, legacy pinned, abstract stubs)
and 4 launch-scalar tests in test_launcher.py.
test_buffer_external_device and test_buffer_external_pinned_alloc still
select Device(n - 1) as before; init_cuda only supplies the context
and teardown. The no-context tests (test_close_with_default_stream_requires_context,
test_from_handle_mr_default_stream_requires_context) keep their no_current_context()
body. Assertions are unchanged.
Add a sentence to tests/AGENTS.md that allocating through
device.memory_resource or Device.allocate, or constructing any memory
resource, counts as touching CUDA and requires init_cuda.
…oundaries
The snapshot gc.collect() gate is sound only if no older
MemoryResource is released during a test, otherwise a mid-test
baseline release could mask a leak. Three tests retained a
closed (but not destroyed) memory resource past their teardown,
which would ratchet the baseline and later cancel a new leak:
- tests/memory_ipc/test_errors.py: ChildErrorHarness keeps the
fixture's buffer/mr/device references on the pytest-retained
item._instance for the session; drop them from __dict__ so the closed
MR objects do not persist.
- tests/test_error_handling.py::test_escalated_cudawarning_from_cleanup_is_not_a_crash:
the unraisable hook args hold exc_traceback whose frame holds
``mr`` (never closed); clear the list, drop the locals, and
close the pool.
- tests/test_memory.py::fill_env: the fixture returns the MR to
the test via a tuple; convert it to a generator that dels ``mr`` in
its teardown so the closed MR does not persist in the
pytest fixture cache.
These are the baseline-retention fixes prerequisite for the planned
live-resource-count gc.collect() gate (see plan
mr-instance-count-gc-gate, "Soundness conditions of the snapshot
gate").
…oryResource count Replace the always-on gc.collect() in the init_cuda teardown with a runtime gate. MemoryResource keeps a process-wide atomic count of live instances (incremented in __cinit__, decremented in __dealloc__) exposed through a private accessor. init_cuda snapshots the count at setup and collects at teardown only when the count has grown. Also warn when a test leaves a MemoryResource alive past teardown (UnreleasedMemoryResourceWarning) or releases one that was alive at setup (BaselineMemoryResourceReleasedWarning), add tests for the counter, and update tests/AGENTS.md.
|
/ok to test c4f065b |
| # in this test: collect in that case too. | ||
| _release_device_default_memory_resources() | ||
| live_after_release = _live_memory_resource_count() | ||
| if live_after_release != live_at_setup: |
There was a problem hiding this comment.
Conditional gc.collect. This where the cost savings come from.
|
|
/ok to test fc62237 |
fd9a0c7 to
b19d0ae
Compare
|
/ok to test b19d0ae |
test_latchkernel constructed 3 DummyUnifiedMemoryResource instances (via make_scratch_buffer) plus a LegacyPinnedMemoryResource (via LatchKernel) without init_cuda. On Windows and aarch64 the ctypes-pointer/Buffer/MR chains form cycles that survive refcounting, so the live-MemoryResource count stays elevated past the test and the next init_cuda test (test_oom_diagnostics_probe_basics_is_live_and_cheap) is blamed by the leftover check (UnreleasedMemoryResourceWarning, escalated to an error). Migrating to init_cuda lets the teardown gate collect the cyclic leftovers, matching the pattern from 68f832c for the other MR-constructing tests.
b19d0ae to
b102d26
Compare
|
/ok to test b102d26 |
📝 Summary
Priority: ⬇️ Low Change: Other Merge Risk: 🟡 Moderate · up to Rare allocation and driver-cleanup failures can leave GPU resources retained while the lifecycle gate reports clean state. These localized fixes should be applied before merge.
Comment |
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:
758efc2b-aff0-4124-8160-a9d263def74b
📒 Files selected for processing (15)
cuda_core/cuda/core/_memory/_buffer.pyicuda_core/cuda/core/_memory/_buffer.pyxcuda_core/pytest.inicuda_core/tests/AGENTS.mdcuda_core/tests/conftest.pycuda_core/tests/helpers/memory.pycuda_core/tests/memory/test_init_cuda_collect_gate.pycuda_core/tests/memory/test_resource_live_count.pycuda_core/tests/memory_ipc/test_errors.pycuda_core/tests/test_error_handling.pycuda_core/tests/test_green_context.pycuda_core/tests/test_helpers.pycuda_core/tests/test_launcher.pycuda_core/tests/test_memory.pycuda_core/tests/test_utils.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.
Move the init_cuda teardown gc.collect() gate's instrumentation out of a Python MemoryResource object counter and into the C++ RAII layer, split into two process-wide atomic counts: owned memory pools (cuMemPoolCreate or imported, not borrowed references) and VA reservations (cuMemAddressReserve). These are the resources issue NVIDIA#2381 is about; borrowed pool references (the device default pool) and MemoryResource subclasses that hold no pool or reservation are no longer counted. - Add std::atomic counts incremented at the owning RAII create sites (wrap_mempool_owned in memory.cpp, create_va_reservation_handle in virtual_memory.cpp) and decremented in their deleters, with live_owned_mempool_count / live_va_reservation_count accessors declared in api.hpp and bridged through _rt.pyx / _rt.pxd. - _buffer.pyx: drop _mr_live_count and MemoryResource.__cinit__/__dealloc__; expose _live_owned_mempool_count / _live_va_reservation_count. - conftest.py: snapshot a (owned_pools, va_reservations) tuple; collect when either component differs; warn per-component. Drop the device-default bookkeeping, since the default pool is a borrowed reference and is no longer counted. - Rewrite the counter tests for the two counters (owned pool, borrowed pool not counted, cycle, VA reservation, concurrency); the gate tests script _live_resource_counts for deterministic skip/collect/warning coverage.
|
/ok to test 0651f45 |
- test_owned_pool_in_cycle_stays_counted_until_collect: create the cyclic resource and assert it is still counted inside the gc.disable() block, so an automatic collection cannot break the cycle before the "+1" assertion. - test_concurrent_owned_pool_construct_and_drop_returns_to_start: record worker-thread exceptions and fail on them; a raising worker only kills its own thread, leaving the count balanced and the test passing silently.
- helpers/memory.py: define VMM_HANDLE_TYPE once from IS_WINDOWS and use it in test_memory.py, graph/test_graph_definition_lifetime.py and memory/test_resource_live_count.py, replacing four local copies (one of which used platform.system()). - test_init_cuda_collect_gate.py: patch conftest globals through _patch_conftest_global, which asserts the name exists. monkeypatch.setitem silently adds a missing key, so a rename in conftest would have left the gate unpatched and a test passing without exercising it. - tests/AGENTS.md: state that the leftover and baseline-release checks track only owned memory pools and VA reservations, name both warnings escalated in pytest.ini, and note that default-pool wrappers, pool-less MemoryResource subclasses and their buffers are not counted.
…wording - pytest.ini: escalate BaselineMemoryResourceReleasedWarning to an error alongside UnreleasedMemoryResourceWarning, matching tests/AGENTS.md. - conftest.py: print labelled counts (owned_pools=..., va_reservations=...) in both warnings; replace the stale GraphMemoryResource example in the leftover-check comment. - helpers/memory.py: describe both warnings in terms of owned memory pools and VA reservations. - test_resource_live_count.py: release the VA test buffer with a context manager so a failed assertion does not leak the reservation. - test_init_cuda_collect_gate.py: assert on the warning class only.
|
/ok to test bc94ed4 |
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to GitHub limitations.
🟠 Major · important: Preserve the owned-pool count when destruction fails. · memory.cpp:82-87
cuda_core/cuda/core/_cpp/rt/memory.cpp:82-87
🩺 Stability & Availability | 🟠 Major | ⚡ Quick winimportant: Preserve the owned-pool count when destruction fails.
pw_cuMemPoolDestroyreturns the driver status, but this deleter discards it. If destruction fails, the pool can remain live. The unconditional decrement then makesinit_cudasee unchanged counts, so it can skipgc.collect()and lose the signal used by the unreleased-resource check. Decrement only after successful destruction, or afterCUDA_ERROR_DEINITIALIZEDwhen the driver can no longer retain the pool.Suggested fix
- pw_cuMemPoolDestroy(b->resource); - g_live_owned_mempools.fetch_sub(1, std::memory_order_relaxed); + const auto status = pw_cuMemPoolDestroy(b->resource); + if (status == CUDA_SUCCESS || status == CUDA_ERROR_DEINITIALIZED) { + g_live_owned_mempools.fetch_sub(1, std::memory_order_relaxed); + }
🟡 Minor · important: Add rollback before allocating MemoryPoolBox. · memory.cpp:98-105
cuda_core/cuda/core/_cpp/rt/memory.cpp:98-105
🩺 Stability & Availability | 🟡 Minor | ⚡ Quick winimportant: Add rollback before allocating
MemoryPoolBox.
cuMemPoolCreateandcuMemPoolImportFromShareableHandlereturn a live pool beforewrap_mempool_ownedallocatesMemoryPoolBox. If that host allocation throws, the custom deleter is not installed andg_live_owned_mempoolsis not incremented. The pool remains undisposed and invisible to the teardown gate. Keep a rollback guard active untilrawis allocated. Release it before constructingstd::shared_ptr; the existing deleter handles control-block allocation failures.Suggested fix
static MemoryPoolHandle wrap_mempool_owned(CUmemoryPool pool, int owner_device) { + struct PoolRollback { + CUmemoryPool pool; + int owner_device; + + ~PoolRollback() noexcept { + if (pool != nullptr) { + clear_mempool_peer_access(pool, owner_device); + pw_cuMemPoolDestroy(pool); + } + } + } rollback{pool, owner_device}; + auto* raw = new MemoryPoolBox{pool}; + rollback.pool = nullptr; // Count before constructing the shared_ptr: if its control-block allocation // throws, it runs the deleter on raw, whose decrement then balances this.
🟡 Minor · important: Roll back the VA reservation when VaReservationBox… · virtual_memory.cpp:99-105
cuda_core/cuda/core/_cpp/rt/virtual_memory.cpp:99-105
🩺 Stability & Availability | 🟡 Minor | ⚡ Quick winimportant: Roll back the VA reservation when
VaReservationBoxallocation fails.
cuMemAddressReservesucceeds beforenew VaReservationBox{{ptr}, size}allocates host memory. If that allocation throws,h_resremains empty, so the caller's handle cleanup cannot run. The reservation remains allocated andg_live_va_reservationsremains unchanged.Suggested fix
- auto* raw = new VaReservationBox{{ptr}, size}; + VaReservationBox* raw; + try { + raw = new VaReservationBox{{ptr}, size}; + } catch (...) { + pw_cuMemAddressFree(ptr, size); + throw; + }
ℹ️ Review info
⚙️ Run configuration
- Configuration used: Repository: NVIDIA/cuda-python/.coderabbit.yaml
- Review profile: CHILL
- Plan: Enterprise
- Run ID:
c43e98f0-baaa-47ff-8788-5a7ce887f21e
📒 Files selected for processing (5)
cuda_core/pytest.inicuda_core/tests/conftest.pycuda_core/tests/helpers/memory.pycuda_core/tests/memory/test_init_cuda_collect_gate.pycuda_core/tests/memory/test_resource_live_count.py
🚧 Files skipped from review as they are similar to previous changes (2)
- cuda_core/tests/helpers/memory.py
- cuda_core/tests/conftest.py
Included review availability: This review used your included allowance. Your plan provides up to 12 included reviews per hour; 9 remain after this review.
… in the VMM IPC tests init_cuda no longer runs gc.collect() at every teardown (NVIDIA#3053), so objects left by earlier tests (child pipes, queues, descriptors) can be collected in the middle of a test that counts process-wide file descriptors, and the count then drops below its baseline. Collect before taking the baseline. Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>
* cuda.core: share VirtualMemoryResource buffers between processes Buffers of a VirtualMemoryResource whose handle_type is "posix_fd" can now be exported and imported like pool-backed buffers. Buffer.ipc_descriptor exports every physical allocation that backs a VirtualMemoryBuffer as a file descriptor, Buffer.from_ipc_descriptor imports the descriptor with a VirtualMemoryResource of the receiving process, and a buffer or its descriptor can be sent through multiprocessing directly. A grown buffer imports as one contiguous range with the same byte layout. The import reuses MemAllocationHandle: import_mem_allocation_handle wraps cuMemImportFromShareableHandle in the same box and cuMemRelease deleter as create_mem_allocation_handle, with the importer's access descriptors. VirtualMemoryIPCBufferDescriptor carries one IPCAllocationHandle and one size per allocation and owns the file descriptors; multiprocessing duplicates them into the receiving process. VirtualMemoryResource pickles as (device, options), which lets a buffer pickle as (resource, descriptor). Fabric handles are not shared yet: no available test system reports fabric support or exposes an IMEX channel, so that path could not be verified. Part of #2980. Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com> * cuda.core: compare the descriptor's chunk count with a signed length The CI wheel build compiles with -Werror and -Wsign-compare, and the VMM import path compared a size_t count with the Py_ssize_t that len() returns. Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com> * cuda.core: mark two VMM IPC tests thread-unsafe Under pytest-run-parallel the same test body runs in several threads. The alias test checks mapping state by address, and a freed address can be reused by another thread's allocation; the descriptor test checks a closed file descriptor number, which another thread can reuse. Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com> * cuda.core: validate and harden VirtualMemoryResource IPC imports Address the review of the VirtualMemoryResource IPC path. Import: check every imported allocation's location against the resource's before mapping (the same device, or the same host location type) and raise a ValueError that names both; reject a descriptor with a size but no allocations; attach notes to driver failures that say which chunk failed and why a peer-controlled size fails at the map. The imported buffer no longer keeps the descriptor: the driver copies the shared allocation into a handle of its own and keeps no reference to the file descriptor, so the descriptors live only as long as the caller holds the descriptor object. Export: an imported buffer, and a buffer grown from one, cannot be exported again because the driver exports only allocations created with the requested handle type; say so instead of surfacing INVALID_VALUE. modify_allocation() rejects a config whose handle_type differs from the resource's, so every chunk of a buffer is exportable the same way. __reduce__ preserves subclasses. Tests: parametrize the memory_ipc suite over VirtualMemoryResource (VirtualMR), with the pool-only scenarios guarded or skipped; PatternGen works on the first `size` bytes of a larger buffer; the fd-leak harness asserts on the success path; new VMM-only tests for many-chunk ranges, a second device with and without peers, descriptor validation and rollback, re-export, empty buffers, and a DLPack consumer. Docs: file descriptor and memory lifetime, multiprocessing-only transport, trust boundary, device rule, re-export limit; VirtualMemoryIPCBufferDescriptor in api_private; the from_ipc_descriptor stub has its mr type again. Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com> * cuda.core: no descriptor cache, no re-export, raw-fd transport for VMM IPC Apply the decisions on the VirtualMemoryResource IPC design. No exporter-side cache. VirtualMemoryBuffer.ipc_descriptor exports on every access and returns a new descriptor that owns its file descriptors; they close when the descriptor is released. Pickling a buffer for multiprocessing builds a transient descriptor, so a sender holds no file descriptors between sends. A spawned Process is the one exception: it receives the file descriptors when it is created, after pickling, so the transient descriptor stays on the spawning Popen until the Process object is released. A descriptor pins the physical memory while it exists, and nothing else does. No re-export of imported memory. Every physical allocation records whether cuMemImportFromShareableHandle produced it (mem_allocation_is_imported), and modify_allocation reuses the allocation handles of its input, so the mark reaches every alias, in-place grow, and move. Exporting such a buffer, directly or through pickling, raises a RuntimeError before any driver call that says the buffer contains memory imported from another process, that it cannot be exported again, and that the caller should forward the descriptor it imported from or copy into a buffer it owns. A Queue feeder thread reports the same error and drops the item. Raw-fd transport. VirtualMemoryIPCBufferDescriptor exposes handle_type, sizes, handles, and fds, and from_fds() rebuilds a descriptor in the receiving process from file descriptors received another way (the integers are duplicated; IPCAllocationHandle objects are shared). The descriptor's own pickling stays multiprocessing-only. No ipc_enabled option: handle_type is the switch and is_ipc_enabled the query, and the docs say so. Tests: file descriptor counts return to baseline on both sides, export under a lowered RLIMIT_NOFILE, the re-export errors for imported and derived buffers (direct, Pipe, Process arguments, Queue feeder thread), and raw-fd round trips through os.dup and a Unix socket with SCM_RIGHTS. Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com> * docs(cuda.core): record the rejected retention design for VMM IPC forwarding Write down in VMM_DESIGN.md the opt-in design that would let an imported VirtualMemoryResource buffer be forwarded (keep the received handles with the imported buffer and duplicate them on export), why the option would belong on the resource, and its cost, so that it does not have to be re-derived. Not implemented; nothing needs it yet. Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com> * test(cuda.core): import resource inside the fd-limit test The module-level import of the POSIX-only resource module made pytest fail to collect tests/memory_ipc/test_vmm_ipc.py on Windows, so the Windows rows ran no cuda.core tests at all. Every test in the module is skipped on Windows by its fixture; only the import has to move. Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com> * cuda.core: VMM IPC review fixes; no sharing of host-located virtual memory Take ownership of each exported or duplicated file descriptor before appending its handle, so a failed append cannot close the descriptor twice. Validate imported chunk sizes against the minimum granularity that cuMemMap requires rather than the resource's granularity option. Name both causes in the note on a failed import: a handle that is not a CUDA allocation of the expected type, or an owning device that this process cannot use. Report is_ipc_enabled False for host-located resources: host NUMA virtual memory has no verified allocation path, so it is not shared yet; the docs and the design note say so. Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com> * cuda.core: VMM IPC review fixes: posix_fd-only from_fds, scoped docs, no test skips - from_fds() takes no handle_type; a file descriptor can only carry posix_fd - The descriptor, resource, and release-note text scope the pin, pickle, and same-device rules to POSIX file descriptors, and the fds docstrings state the descriptor's lifetime rule - The 1.3.0 notes record that a per-call modify_allocation config must keep the resource's handle_type - VMM_DESIGN.md records the fabric direction: a descriptor class of its own - TestManyChunkBuffer and TestReexport no longer skip when the driver refuses the decoy's address hint; the grow relocates in that case too Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com> * test(cuda.core): collect garbage before each file-descriptor baseline in the VMM IPC tests init_cuda no longer runs gc.collect() at every teardown (#3053), so objects left by earlier tests (child pipes, queues, descriptors) can be collected in the middle of a test that counts process-wide file descriptors, and the count then drops below its baseline. Collect before taking the baseline. Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com> * test(cuda.core): mark the VMM re-export test thread-unsafe TestReexport._derived asserts that a grow moved the buffer after a decoy took the adjacent range. Under pytest-run-parallel another thread can free that range between the decoy attempt and the grow, so the grow extends in place and the assertion fails. Mark the test thread_unsafe like the other address-probing tests in the file. Also record in VMM_DESIGN.md the numbers behind the no-cache export rule and the age of the multiprocessing machinery the Process path relies on. Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com> --------- Co-authored-by: Claude Fable 5.1 <noreply@anthropic.com>
Description
The
init_cudateardown added by #2484 runsgc.collect()after every test to return memory-pool VA reservations before the next test (issue #2381). Withgc.freeze()already in place, a measured full-suite run still spends ~18.0 s of ~86.1 s (20.9%) in that unconditional collect, and the cost tracks session age rather than what a test allocates. The collect is only needed when a test leaves an owned pool or VA reservation alive in a reference cycle; a resource whose last reference is a local dies by refcount at function return and needs no collect.This PR replaces the always-on collect with a runtime gate driven by process-wide counts of live owned driver resources, plus the test-hygiene changes it requires. Logical pieces:
test(cuda.core): request init_cuda for memory resource tests— route tests that construct or allocate from a memory resource withoutinit_cudaonto it, so they pop the context and clear theDevice's cached default resource. Documents the rule intests/AGENTS.md.test(cuda.core): release baseline-retained memory resources at test boundaries— fix three tests (ChildErrorHarness, the escalated-cudawarning test, thefill_envfixture) that kept a closed resource alive past teardown.test(cuda.core): gate the init_cuda teardown gc.collect() on live count—init_cudasnapshots a live count at setup and collects at teardown only when it changed, after dropping the item'sfuncargs. AddsUnreleasedMemoryResourceWarningandBaselineMemoryResourceReleasedWarning(the latter escalated to an error inpytest.ini), counter unit tests, andtests/AGENTS.mdupdates.test(cuda.core): count live owned pools and VA reservations in C++— per review, move the counting out of a PythonMemoryResourceobject counter into the C++ RAII layer, split into two process-wide atomic counts: owned memory pools (cuMemPoolCreate/imported) and VA reservations (cuMemAddressReserve), exposed through_live_owned_mempool_count/_live_va_reservation_count.init_cudasnapshots a(owned_pools, va_reservations)tuple; borrowed pool references (the device default) andMemoryResourcesubclasses that hold no pool or reservation are no longer counted, so the per-Devicecached-default bookkeeping is removed.Approach
A runtime count is preferred over a per-test
owns_poolmarker plus a lint rule (as in #3045): it needs no annotation to stay in sync, covers resources constructed indirectly through fixtures/helpers, and cannot silently skip a needed collect from a forgotten marker. Counting owned pools and VA reservations at their C++ driver create/destroy sites — rather than using the PythonMemoryResourceobject as a proxy — measures the resources #2381 is about, ignores borrowed pool references, and still counts an owned pool or reservation that outlives its Python resource (for example through a graph in a reference cycle holding allocation handles), which the object counter missed. The cost is test-only instrumentation: two atomic counters and two underscore-prefixed accessors, no per-object field and no layout change.Measured results (RTX 6000 Ada, Linux, pinned
pytest-randomlyseed 42)These numbers were taken with the earlier per-object counter (piece 3); the C++ counts (piece 4) track strictly fewer resources, so the gate skips a superset of those teardowns and a full-suite re-measurement is a follow-up. Correctness under the C++ counter was re-verified on
test_memory.py,test_error_handling.py, andmemory_ipc/with both warnings escalated to errors, with no misfires.Soundness and scope
The snapshot gate is sound as long as no older owned resource is released during a test (which could cancel a new leak); piece 2 removes the known retention sources, the gate collects on any count change (not just growth), and it warns when a count falls below the snapshot. Still not covered: cyclic non-resource garbage that holds device memory without its own owned pool or VA reservation (for example a cyclic
Bufferon the borrowed device-default pool), and resources from module/class-scoped fixtures. Verification here was single-GPU Linux; the Windows MCDM/WDDM CI rows remain the real OOM check.Checklist