cuda.core: make Device methods use their bound context - #2750
Conversation
|
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. |
|
/ok to test |
This comment has been minimized.
This comment has been minimized.
a09bebe to
838f850
Compare
Run context-sensitive Device operations against the Device's bound context while preserving caller state. Centralize context-aware cleanup and synchronous allocation handling so resource lifetimes remain correct.
838f850 to
83a3d46
Compare
|
/ok to test |
| // Run a creation operation and undo it if context restoration fails. | ||
| // Context-independent undo always runs. Context-sensitive undo runs only | ||
| // after verifying that the target context remains current; otherwise the | ||
| // resource leaks rather than risking cleanup in the wrong context. | ||
| template <typename Fn, typename Undo> | ||
| CUresult invoke_in_context_or_undo(const ContextHandle& h_context, Fn&& operation, | ||
| Undo&& undo, bool undo_requires_target_context) noexcept { | ||
| ASSERT_NOTHROW_INVOCABLE(Fn&&); | ||
| ASSERT_NOTHROW_INVOCABLE(Undo&&); | ||
| CUcontext previous = nullptr; | ||
| int changed = 0; | ||
| CUresult status = enter_context(h_context, &previous, &changed); | ||
| if (status != CUDA_SUCCESS) { | ||
| return status; | ||
| } | ||
| status = std::invoke(std::forward<Fn>(operation)); | ||
| CUresult composite = exit_context(previous, changed, status); | ||
| if (status == CUDA_SUCCESS && composite != CUDA_SUCCESS) { | ||
| bool undo_ok = true; | ||
| if (undo_requires_target_context) { | ||
| CUcontext current = nullptr; | ||
| undo_ok = p_cuCtxGetCurrent(¤t) == CUDA_SUCCESS | ||
| && current == as_cu(h_context); | ||
| } | ||
| if (undo_ok) { | ||
| std::invoke(std::forward<Undo>(undo)); | ||
| } | ||
| status_ = p_cuCtxSetCurrent(target); | ||
| changed_ = status_ == CUDA_SUCCESS; | ||
| } | ||
| return composite; | ||
| } |
There was a problem hiding this comment.
I played a lot of defense here, and it is easy to see how complex this gets as errors cascade. I partly question the value of this kind of code. Would it be reasonable to call std::abort instead when a key invariant cannot be maintained? If context restoration fails, then whether or not the undo succeeds, the wrong context may remain current, and the program is in big trouble either way.
| event_registry.unregister_handle(b->resource); | ||
| GILReleaseGuard gil; | ||
| p_cuEventDestroy(b->resource); | ||
| pw_cuEventDestroy(b->resource); |
There was a problem hiding this comment.
Replaced deallocation functions (p_*) in destructors with wrapped versions that issue warnings on failure (pw_*).
| def assert_device_operations_use_bound_context(device): | ||
| """Check that Device operations use its bound context and preserve the ambient context.""" | ||
| bound_context = device.context | ||
| ambient_context_handle = current_context_handle() |
There was a problem hiding this comment.
| ambient_context_handle = current_context_handle() | |
| ambient_context_handle = current_context_handle() | |
| assert int(bound_context.handle) != ambient_context_handle, ( | |
| "Precondition failed: the device's bound context must not be the " | |
| "current (ambient) context." | |
| ) |
- Fix _SynchronousMemoryResource to record a deallocation token bound to its own context, so Buffer teardown enters the right context regardless of what is current, and works with no context current. Move the class to its own module and resolve the primary context lazily. - LegacyPinnedMemoryResource.device_id returns -1 as documented; texture creation over a pinned buffer works again. - Device.set_current delegates a foreign-device context to its owning device instead of raising, so the save/restore idiom round-trips across devices. - Guard empty context handles once in invoke_in_context(_or_undo); warn when an undo is skipped because the target context is gone. - context_synchronize and context_get_stream_priority_range release the GIL like the other helpers. Rename array/mipmap box accessors to get_box. - Tests: query the driver (cuStreamGetCtx, cross-context cuEventRecord) instead of comparing cached metadata; add sync(), set_current round-trip, pinned texture, and synchronous-resource teardown tests; register device_x2 with the parallel-test plugin. - Move the release note to 1.3.0 and describe sync() as acting on the bound context. Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>
|
The latest upload addresses Ralf's feedback. |
handle_return() takes the whole result tuple, and cuCtxGetDevice() returns a CUdevice that never compares equal to an int; both made the cross-context event-record check raise instead of run (or skip) as intended. Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>
Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>
…text The uniform empty-context guard added for review item 6 broke create_event_handle_noctx, which relied on an empty handle meaning "current context". That helper was itself a NVIDIA#2311-class bug: Stream.wait(stream) and the foreign-array/tensor import paths created their temporary ordering event in whatever context was current, and cuEventRecord rejects an event from a different context than the stream it is recorded on, so cross-device waits failed unless the right device happened to be current. Replace it with create_event_handle_for_stream, which resolves the stream's owning context via cuStreamGetCtx and creates the event there. Callers now check the returned handle and surface the real creation error. With that, every creation helper requires a context and no exception remains. 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>
| # so pushing/popping a foreign-device context is delegated to | ||
| # the device that owns it; its own bookkeeping (_context, | ||
| # _has_inited) is what should track this push, not ours. | ||
| return Device(ctx._device_id).set_current(ctx) |
There was a problem hiding this comment.
Worth a sentence in the docstring: delegating here means dev.set_current(foreign_ctx) makes foreign_ctx current on the thread, but does not update dev._has_inited/dev._context -- only Device(ctx._device_id)'s bookkeeping is updated. So immediately after this delegation, dev itself is still considered uninitialized from its own perspective (e.g. dev.create_stream() would still raise "not yet initialized" even though a context is now current on the thread). That's consistent with the comment's reasoning, but it's a non-obvious behavioral subtlety for callers relying on the old set_current contract and would be worth documenting explicitly in the docstring above.
This comment has been minimized.
This comment has been minimized.
The 1.3.0 notes file arrived with NVIDIA#2750, and this change targets that milestone, so fold the entries in there and drop the placeholder file. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
This comment has been minimized.
This comment has been minimized.
…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>
This comment has been minimized.
This comment has been minimized.
2 similar comments
This comment has been minimized.
This comment has been minimized.
|
…cannot be raised (#2759) * cuda.core: define the error handling policy and report failures that 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> * cuda.core: keep the texture autosummary contiguous in api.rst 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> * cuda.core: attach secondary failures to the propagating exception as 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> * cuda.core: follow the review of #2750 and flush the stderr fallback Rebased onto the reviewed head of #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> * cuda.core tests: check host-only buffer teardown for CUDAWarning, not stderr The host-only Buffer tests from #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> * cuda.core docs: cuda.core never terminates the process The policy text reserved std::abort for an internal invariant violation and specified how such a helper would have to behave. The decision is that cuda.core never terminates the process: an internal invariant violation is raised as a RuntimeError where an exception can propagate, reported as a CUDAWarning where it cannot, and the affected resource is leaked. Users who want fail-fast behavior escalate the warning category themselves. An implicit abort (an exception escaping noexcept code) remains a bug, not a policy choice. Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com> * cuda.core: report cleanup failures after restoring the caller's context cleanup_in_context() reported an activation or operation failure before it switched back to the caller's context. A CUDAWarning runs user code (warning filters, showwarning), so that code observed the cleanup context instead of the caller's. Emit both reports after the restoration attempt; the report order and the return value are unchanged. Review follow-up on #2759. Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com> * cuda.core: name the resource in cleanup reports, export CUDAError, note the detail limitation Review follow-ups on #2759: - Cleanup reports name the resource handle ("cuMemFreeAsync(0x...) failed ..."). Python's warning registry collapses repeated warnings with identical text from one call site, so two independent resources failing the same call from the same line produced a single CUDAWarning. The pw_* wrappers name their first argument, cleanup_in_context() takes the handle explicitly, and the Buffer deallocation callback names the pointer. A test releases two buffers under an injected restoration failure and expects two reports. - CUDAError and NVRTCError are importable from cuda.core; the error handling page told users to catch CUDAError but it lived in a private module. Both classes gained docstrings. - DESIGN.md and the docs no longer claim that keying the thread-local detail to its status prevents misattribution; a caller that drops the status leaves it behind for a later error with the same code. #2760 removes the thread-local state. Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com> * cuda.core: never acquire the GIL while holding a C++ lock deviceptr_import_ipc() took ipc_import_mutex and only then released the GIL, so a thread blocked on the mutex while holding the GIL deadlocked with the holder waiting to reacquire it at scope exit (#2840). It also called the pw_ wrapper for the discard path under the mutex, and the wrapper acquires the GIL to emit a CUDAWarning. Release the GIL before taking the mutex, keep lookup, import and registration under the mutex so a descriptor is never imported twice, discard with the raw driver call, and report a failed discard only after the lock is released. DESIGN.md states the rule: the GIL is the outermost lock; nothing that holds a C++ lock may acquire or reacquire it. The guard reorder is the same fix as #2848, which also adds the regression test for the deadlock; this change keeps that reorder and adds the deferred report, so whichever lands second resolves the overlapping hunk in its favor. Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com> * cuda.core docs: which reporting channel to use, and what pw_ really does Review follow-ups on #2759: - DESIGN.md gains a table that picks the reporting channel by situation (a path that can raise, an except block whose rollback failed, a deleter, a Python destructor path, a CUDA callback thread), with the thread-local mechanisms marked transitional pending #2760, and a section on p_ versus pw_: a pw_ wrapper acquires the GIL on failure and runs user Python, so it is never used while a C++ lock is held. Python exceptions raised by that code never become C++ exceptions; nothing on the report path may allocate or throw. CUDA callback threads do nothing that needs the GIL; Py_AddPendingCall is how work leaves them. - AGENTS.md gets the same two rules in short form. The header comments on the reporting functions and on WarnOnFailure say what pw_ runs. - note_or_report_cuda_error is renamed attach_rollback_failure: callers are in one situation (a rollback failed while an exception is in flight) and should not have to know the note-or-warning mechanism. The test hook follows. Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com> * cuda.core: hold ipc_import_mutex while freeing an imported IPC pointer Brandon's concurrent-import test (#2848) crashes on CUDA 12.9 once the GIL reorder lets the importers run: the IPC pointer cache's deleter did not take ipc_import_mutex, so a concurrent importer that found the entry expired while the deleter was still freeing re-imported the allocation, got a duplicate pointer to the same mapping (nvbug 5570902), and the first cuMemFreeAsync unmapped it for both. The deleter now releases the GIL, then holds the mutex across unregister and free. The cleanup report emits a CUDAWarning, which acquires the GIL and runs user code, so it must not run under the mutex: cleanup_in_context() gains an overload with an after_cleanup hook, called unconditionally once the cleanup and the context restoration are done and before anything that may run user code, and the deleter passes one that unlocks its std::unique_lock. The deallocation context is resolved before the lock for the same reason. Companion to the main-side fix pushed to #2848. Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com> --------- Co-authored-by: Claude Fable 5.1 <noreply@anthropic.com>
Description
closes #2311
Devicemethods that create resources or synchronize (create_stream,create_event,create_opaque_array,create_mipmapped_array,create_texture_object,create_surface_object,sync) previously operated on whatever context was current on the calling thread, sodev1.create_stream()could silently create a stream on device 0 while labeling itdevice_id == 1.These methods now run against the
Device's bound context and restore the caller's current context afterward, including when no context is current. Details:Device.sync()synchronizes the bound context.Device.set_current()returns the previously current context._SynchronousMemoryResourcebinds its context at construction.Checklist
🤖 Generated with Claude Code