cuda.core: fix pool setup and builder teardown under stream capture - #2838
Draft
Andy-Jost wants to merge 17 commits into
Draft
cuda.core: fix pool setup and builder teardown under stream capture#2838Andy-Jost wants to merge 17 commits into
Andy-Jost wants to merge 17 commits into
Conversation
…cannot be raised Write down how cuda.core handles CUDA failures (docs/source/error_handling.rst for users, a "Failure handling" section in AGENTS.md and _cpp/DESIGN.md for contributors) and bring the code into line with it: - Add cuda.core.CUDAWarning, emitted for CUDA errors that cannot be raised (destructors, CUDA callbacks, cleanup after an earlier failure). The C++ handle layer reports through one helper that uses the Python warnings machinery when the interpreter is usable, delivers an escalated warning as an unraisable exception, and falls back to stderr otherwise. CUDA_ERROR_DEINITIALIZED is not reported. - Wrap every destroy call made from a deleter (pw_*) so its failure is reported instead of discarded, including memory pools, green contexts, graphs, graph execs, graphics resources, the linker, user objects, the NVRTC/NVVM/nvJitLink handles and file descriptors; release the GIL around the compiler-handle destroys like the CUDA ones. - When the caller's context cannot be restored after a successful operation, undo the creation and raise a CUDAError that says which context is current; report the same failure as a warning in deleters; report a skipped context-sensitive undo instead of leaking silently. - Add context_get_device and graph_node_set_params so Stream_get_ctx_device and _set_definition_node_params stop hand-rolling cuCtxPush/Pop/SetCurrent. The node update now publishes its attachment before raising a restoration failure, closing a window that left the node referencing released owners. - Device.set_current(ctx) switches with a single cuCtxSetCurrent, so a failure leaves the previous context current and the call works without one. - Report failed cuStreamEndCapture in GraphBuilder.__dealloc__ and failed child-graph rollbacks; warn from _mr_dealloc_callback instead of printing. - Add a test hook that makes the next context restoration fail, tests for the policy, and release notes for 1.3.0. Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>
The "Errors and warnings" section was inserted between the texture classes and the texture option dataclasses, which moved OpaqueArrayOptions, MipmappedArrayOptions and TextureObjectOptions under cuda.core in the docs index and failed test_api_docs_consistency on every CI platform. Place the section after the texture section instead. Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>
…notes Review follow-ups on the error-handling policy: - A failure that happens while an exception is being raised is no longer reported out of band. When both an operation and the restoration of the caller's context fail, the operation's CUDAError is raised with the restoration failure attached; when only the restoration fails, its error is raised with the context explanation attached. The attachment is a PEP 678 note on Python 3.11+ and is appended to the message on 3.10. The thread-local detail is keyed to the status it was recorded for, so it cannot attach to an unrelated error if that status is never raised. - A failed rollback inside a Cython `except` block is attached to the exception being handled through note_or_report_cuda_error(), which falls back to a CUDAWarning when nothing is being handled or notes are unavailable. - Reporting stays reserved for destructors and CUDA callbacks; CUDAWarning's docstring and the docs say so. - DESIGN.md explains the two status conventions of the C++ layer (handle factories use thread-local err, everything else returns CUresult) and the abort-helper guidance in AGENTS.md asks for a faulthandler-style traceback. - Drop the release-relative "in this release" wording from the stable docs. Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>
…back Rebased onto the reviewed head of NVIDIA#2750. Adjustments the rebase needed: - The review's warning for an undo skipped after a failed context restoration is routed through report_cuda_error(), so it carries the CUDA status and becomes a CUDAWarning like every other non-raising report. - invoke_in_context and invoke_in_context_or_undo now reject empty handles themselves, so context_get_device drops its own guard like the other helpers did; enter_context's no-op for empty handles is documented as used only by graph_node_set_params. - _SynchronousMemoryResource moved to its own module; the error-handling test imports it from there. The review's two teardown tests asserted that stderr stayed empty; under the policy a teardown failure is a CUDAWarning, so they assert that no CUDAWarning is issued instead (and are marked thread_unsafe because warning capture is process-global). - report_message() flushes stderr after its last-resort fprintf, so the text is not lost if the process dies right after (review comment). Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>
…policy # Conflicts: # cuda_core/cuda/core/_memory/_buffer.pyx # cuda_core/tests/test_memory.py
… stderr The host-only Buffer tests from NVIDIA#2773 asserted that nothing containing "Warning" reached stderr. Under the error handling policy a teardown failure is a CUDAWarning, not stderr text, so that assertion no longer checks anything. Use assert_no_cuda_warning() around allocate/close instead (marked thread_unsafe, as warning capture is process-global). The spawned-process variant checks inside the child, since warnings do not cross processes; a failure surfaces as the non-zero exit code the parent already asserts on. Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>
… copy from the merged wheel build_hooks.py maps a Cython module to its C++ by name. It now also accepts a directory: every .cpp under cuda/core/_cpp/<stem>/ compiles into the one extension for _<stem>.pyx, with the legacy single file _cpp/<stem>.cpp kept as the fallback (tensor_map is unchanged). With no such directory in the tree yet, the sources are exactly today's, so this part is inert on its own. The cuda.core._cpp package-data globs become recursive so headers in nested directories ship, and .gitignore stops ignoring .cpp files under cuda/core/_cpp/ so new sources are visible to git. ci/tools/merge_cuda_core_wheels.py stops retaining a third, top-level copy of _resource_handles and the top-level _cpp/ and _include/ headers in the merged cu12+cu13 wheel. cuda/core/__init__.py rewrites __path__ to the versioned subpackage before any import reaches them, so that copy (about 308 KB uncompressed in the 1.2.0 wheel) was never imported; the comment defending it referred to an import removed in NVIDIA#1463. A step in build-wheel.yml now asserts that the merged wheel's top level holds only __init__.py, _version.py and the two versioned trees. Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>
…e pathlib Review follow-up. The top-level layout assertion moves from build-wheel.yml into ci/tools/merge_cuda_core_wheels.py, which now derives the kept cuNN directories from its input wheels and raises if anything else remains under cuda/core/ after the removal. _extension_sources uses pathlib.
# Conflicts: # cuda_core/docs/source/release/1.3.0-notes.rst
…x) into the _rt refactor base
The module holds cuda.core's runtime support layer: resource handles, the
driver function-pointer table, error reporting and deferred cleanup. Rename
it to _rt and move its C++ under _cpp/rt/ in namespace cuda_core::rt. Every
function keeps its body, signature and symbol name; only the namespace
qualifier and the file names change.
- git mv _resource_handles.{pyx,pxd,pyi} to _rt.*; resource_handles.{hpp,cpp}
to _cpp/rt/rt.{hpp,cpp}; the three design notes to _cpp/rt/.
- The 53 cimporting Cython files change only their cimport line.
- Delete _CUDA_DRIVER_API_V1_NAME, a capsule-name constant nothing reads.
- Regenerate the stub (stubgen-pyx).
The dynamic symbol table of the built extension equals the old one under
s/cuda_core::/cuda_core::rt::/, and __pyx_capi__ has the same keys.
A Py_DECREF callback shaped for cuUserObjectCreate, superseded by the make_opaque_py ownership path and called from nowhere. Its four declarations go: the C++ prototype and body, the .pxd cdef and the .pyx extern declaration.
…sion's sources in parallel Two build changes the C++ split needs. Every extension now lists the headers under cuda/core/_cpp/<stem>/ as `depends`. A cimporting extension compiles against the header its .pxd names, and cythonize copies each `depends` entry into its build directory, so the copied header finds its sibling includes beside it. Listing the whole directory avoids parsing includes; every extension rebuilds when one of these headers changes, exactly as editing the one monolithic header did. setuptools compiles the sources of one extension serially and parallelizes only across extensions, so a multi-source extension becomes the critical path. build_ext now fans the per-object compile calls of every extension out to one shared thread pool of `nthreads` workers. MSVC keeps the stock path.
The monolithic header becomes: types.hpp (handle aliases, tagged values, Prepared* types, the inline as_cu/as_intptr accessors), py.hpp (the one file that includes <Python.h>: py_is_finalizing, make_py and as_py, and the prototypes that take or return PyObject*), driver_api.hpp (the p_* table and the version-gated shims), error.hpp (thread-local error state and the non-propagating reporting API), api.hpp (every other prototype, one banner per resource family), plus two umbrellas: rt.hpp, named only by _rt.pyx, and handles.hpp, named only by _rt.pxd, whose include closure is types.hpp and py.hpp. Every declaration moves verbatim; the only additions are the file boilerplate, one banner in py.hpp and `// Implemented in <file>` lines on the prototypes whose body lives outside their family source. The generator (anchored on the monolith's text) and its check mode live in the maintainer's notes; the dynamic symbol table and __pyx_capi__ of the built extension are unchanged.
The monolithic source becomes twelve translation units: one resource family per file (context, stream, event, memory, program, graph, graph_exec, texture), the driver table with its version-gated shims (driver_api.cpp), the error state and non-propagating reporting (error.cpp), and the two Python-coupled bodies (py_report.cpp, py_deferred_cleanup.cpp). Two headers hold the helpers the files share and Cython never names, in namespace cuda_core::rt::detail: context_scope.hpp (enter/restore/exit_context and the invoke_in_context templates) and internal.hpp (HandleRegistry, WarnOnFailure with the pw_* wrappers, DeallocationStream, DeferredCleanupItem and the declarations of the promoted helpers). py.hpp gains the GIL guards; error.hpp declares the thread-local `err` that error.cpp now defines. Every definition moves verbatim. Helpers that were static or in an anonymous namespace and are now called across files become external with a declaration; everything local to one file keeps its anonymous namespace. driver_api.cpp and error.cpp compile without a Python include path. Verification: the generator's check mode reproduces all 21 files from the monoliths; the dynamic symbol table gains exactly the nine promoted detail:: functions and the err object and loses nothing; __pyx_capi__ is unchanged. tests/test_rt_layout.py pins the layout rules.
DESIGN.md, GRAPH_ATTACHMENTS.md and REGISTRY_DESIGN.md moved with the code; this keeps them from misleading: the module name, the file layout, the cdef extern examples, and the heading that still named the deleted _CUDA_DRIVER_API_V1 capsule. AGENTS.md learns the directory form of _cpp/<name>/ and the new paths. One release note for the renamed private module and its shipped .pxd.
DeviceMemoryResource(device) with no options raises the release threshold of the driver's pool with cuMemPoolGetAttribute and cuMemPoolSetAttribute. The driver refuses both as potentially unsafe calls while the calling thread is inside a global or thread-local capture, and it invalidates the capture. Device.memory_resource constructs the resource lazily, so a first allocation could invalidate a capture in progress. Make the two calls in relaxed capture mode and restore the thread's previous mode afterwards. A failure to restore the mode is attached to the propagating error as a note. Ending an invalidated capture made the builder destroy a graph the driver had already destroyed. cuStreamEndCapture returns a NULL graph for an invalidated (or unjoined) capture and releases the capture graph itself, but the builder kept the owning handle it took from cuStreamGetCaptureInfo and its deleter called cuGraphDestroy again: a use-after-free that segfaulted at close() or garbage collection. Add invalidate_root_graph_state to retire the hierarchy when the driver discards the root graph, and route end_building(), close() and __dealloc__ through one GB_end_capture helper that settles graph ownership from the end-capture result. end_building() now ends an invalidated capture and raises the driver error; the builder then holds no graph (new CAPTURE_INVALIDATED state), and complete(), debug_dot_print(), graph_definition, embed() and Graph.update() say so. close() closes the builder before raising. end_building() on a forked builder is rejected with RuntimeError instead of invalidating the capture. Fixes NVIDIA#2834. Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>
Andy-Jost
added
bug
Contributor
|
Auto-sync is disabled for draft pull requests in this repository. Workflows must be run manually. Contributors can view more details about this message here. |
Contributor
Author
|
/ok to test |
Contributor
|
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
Fixes #2834, both parts.
DeviceMemoryResource(device)can be constructed, andDevice.memory_resourcefirst touched, while the calling thread is inside aglobalorthread_localstream capture, without failing or invalidating the capture. Ending, closing, or collecting aGraphBuilderwhose capture was invalidated no longer segfaults.Both minimal reproducers from #2834 were run against this branch on a local GPU host: every scenario of the first now leaves the capture active, and the second survives 200 invalidate-and-collect rounds in each capture mode.
Changes
_memory_pool.pyx:MP_raise_release_thresholdmakes itscuMemPoolGetAttribute/cuMemPoolSetAttributecalls in relaxed capture mode viacuThreadExchangeStreamCaptureModeand restores the thread's previous mode. Capture mode is per thread, so this needs no stream and has no effect when the thread is not capturing. A failed restore is attached to the propagating error as a note, per the error handling policy. This mirrors the libcu++ fix in [libcu++] Make resolving a default memory pool legal under stream capture cccl#11360._cpp/rt/graph.cpp,api.hpp,_rt.pxd,_rt.pyx: newinvalidate_root_graph_state(h_root)retires a hierarchy whose root graph CUDA destroyed itself, so the owning handle's deleter no longer callscuGraphDestroyon it._graph_builder.pyx:GB_end_capturereplacesGB_end_capture_if_neededand settles graph ownership from thecuStreamEndCaptureresult. A NULL graph means the driver discarded the capture graph; the builder drops its handle and enters the newCAPTURE_INVALIDATEDstate.end_building()ends an invalidated capture and raises the driver error,close()closes the builder and then raises, and__dealloc__reports as before.complete(),debug_dot_print(),graph_definition,embed()andGraph.update()reject a graph-less builder with a clear message.end_building()on a forked builder raisesRuntimeErrorinstead of ending capture on the wrong stream, which invalidated the whole capture.CUDAWarning; forkedend_building()is rejected.Related Work
🤖 Generated with Claude Code