Repository navigation
Conversation
Re-verified on a second host (8 × RTX 5090)No code change since the first push — this is the same commit
The first host was 2 × L20 with Python 3.10 and Still unchanged and still waiting on the split answer: the stream-side matrix (C01–C05, C09) remains |
Three behaviours of the existing host-callback helper were not covered: * a Python exception raised inside a callback is reported through sys.unraisablehook (the trampoline is noexcept with gil), the graph launch and the stream sync still return, and the node after the callback still runs; * user_data given as an int is the caller\x27s own pointer, delivered to a ctypes callback unchanged; * two devices running their own graphs each keep their own callback parameters and results. No API is added and no implementation code changes: the ordinary-stream callback entry point does not exist yet, so the stream-side ordering and lifetime matrix for NVIDIA#311 stays blocked on that dependency (reported in the survey that accompanies this change). Signed-off-by: 0z5a <Dezhen.lu@student.uni-tuebingen.de>
7cdc639 to
d77759e
Compare
|
/ok to test d77759e |
Andy-Jost
left a comment
There was a problem hiding this comment.
Thank you for this PR, and sorry it waited a month for a review. The three tests pin useful contracts, and they still fit main as it stands today. I checked that _host_callback.pyx and every API and fixture the tests use are unchanged since your base. Two changes are needed before merge, plus two small cleanups.
Should fix
-
cuda_core/tests/graph/test_host_callback_contracts.pyline 43:sys.unraisablehookis replaced before thetry/finallythat restores it. The calls on lines 47-49 (create_stream,allocate, andLegacyPinnedMemoryResource().allocate) run outside thetrythat starts at line 55. If one of them raises, thefinallynever runs, pytest loses its session hook, and every later unraisable report in the run is swallowed. Usemonkeypatch.setattr(sys, "unraisablehook", records.append)ascuda_core/tests/test_program.pydoes, or move lines 42-49 inside thetry. -
cuda_core/tests/graph/test_host_callback_contracts.pylines 37, 82, and 112: the three tests have no authorship marker.AGENTS.mdasks for one on each new test:@pytest.mark.human_authored,@pytest.mark.agent_authored(model="..."), or@pytest.mark.human_reviewed. The markers are registered incuda_core/pytest.ini, and the nearby host-callback tests intest_graph_definition.pycarry them. Please add the marker that matches the origin of each test.
Nits
-
Lines 5 and 17-19: the module docstring describes PR-process state (the "#311 regression survey" and a blocked stream-side matrix) rather than what the file tests. Neither exists in the repo, and the "does not exist yet" sentence goes stale once
host_launchlands. Please trim the docstring to the three contracts in lines 10-15. -
Lines 48-49, 87, 117, and 130: buffers are constructed and never released.
cuda_core/tests/AGENTS.mdasks tests to release what they construct, with context managers for single-scope lifetimes. Harmless today because the graph retains its operands, but it is the convention the newer tests follow.
Your open questions
- Scope: I answered on #2058 on 2026-10-05. There is no independent #311 scope. This PR tests the behaviour main has today, and I review it on its own.
- Stream-side cases: yes, they stay blocked until
host_launchexists. The matrix is written with that entry point; do not start them as an implementation. - Please update the PR body to reflect both answers and drop the "Scope approved in: not yet confirmed" line.
Next steps
I am starting CI now with /ok to test, so the suite runs on the NVIDIA runners for the first time, and I am fixing the labels and milestone. When items 1 and 2 are in and CI is green, I merge.
|
…fers Install the unraisable hook with monkeypatch so it is restored even if stream or buffer setup raises. Mark the three tests agent_authored, trim the module docstring to the contracts under test, and release every buffer the tests construct through context managers. Signed-off-by: 0z5a <Dezhen.lu@student.uni-tuebingen.de> Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
…DIA#3188 (cuDSS 1.0 library names) Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>
|
I merged |
|
/ok to test a908c7c |
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info
📝 Summary
Priority: ⬇️ Low Change: Other Merge Risk: ⚪ Minimal · up to
|
Incremental review: Files changed against
main.Scope
Related to #311. This does not close it.
Scope approved in: not yet confirmed. #311 asks for stream callbacks, and its concrete design overlaps #2058 (
host_launch, assigned to @Andy-Jost), so I asked which slice is independent before writing anything: #311 (comment) and #2058 (comment). Pending that answer, this PR adds only contract tests for the host-callback helper that already exists onmain— no public API, no implementation change.This PR changes: one new test file,
cuda_core/tests/graph/test_host_callback_contracts.py— three behaviours of_resolve_host_callback/ the Python trampoline that the existing suite did not pin down.Out of scope: an ordinary-stream callback entry point (
Stream.add_callback, acuStreamAddCallbackwrapper, or another trampoline) — that is #2058's design; the stream-side ordering/lifetime matrix of #311, which cannot be asserted until that entry exists; capture/replay coverage.Contract
The three cases, and what they fix in place:
cdef void _py_host_trampoline(void* data) noexcept with gil, so a Python exception raised inside a callback cannot cross back into the driver. Observed on this commit: the exception is delivered tosys.unraisablehook,graph.launch()andstream.sync()return normally (no CUDA error), the node after the callback still executes, and a later launch still runs its callback. The test asserts exactly that, rather than promising a mechanism the API does not document.user_data.user_datagiven as anintis the caller's own pointer and reaches a ctypes callback unchanged (_resolve_host_callbackcopies only the bytes form). The test asserts pointer identity and the byte read through it; the caller keeps owning the storage, and there is deliberately no "freed early but still expected to work" case.Evidence
8b6e9f52db1074a580fc1678c71e7c75c42db2ea7cdc6395632c181d543f819a7f75c8056625592f— the commit in this PR (cuda_core/tests/graph/test_host_callback_contracts.py, sha25621b71759af2f3a359f70c385cc8171127390c3a8846ed49074669d811a05111a); the branch carries that same commit object.cuda-core 1.2.1.dev112+g8b6e9f52dbuilt from this tree (pip install --no-build-isolation -e cuda_core),cuda-bindings 12.9.8, Cython 3.2.9, Python 3.10.12, CUDA toolkit/usr/local/cudaGPU-836d0605-…/GPU-739e6fcb-…)cuda_core/tests/graph/test_host_callback_contracts.py— 3 passed; the file was run 20 consecutive times to check for flakiness (all green)cuda-bindings13.4.2, CUDA 13.4 headers), same commit: 3 passed in 1.04 s,cuda_core/tests/graph502 passed, 33 skipped, andcuda_core/testsin full 4317 passed, 0 failed — so the three assertions hold on both a 2-GPU and an 8-GPU machinecuda_core/tests/graph— 502 passed, 33 skipped (three consecutive runs);cuda_core/tests/system+cuda_core/tests/test_stream.py— 124 passed, 130 skipped, 8 xfailed, 984 subtests passedBLOCKED: dependency— C01–C05 and C09 of the Add stream callback support #311 matrix (single-stream callback, repeated callbacks, two streams per GPU, two GPUs × two streams, two host threads, owning vs borrowed stream) all need the ordinary-stream entry point thatmaindoes not have yet; reported as blocked rather than asserted or failed.OUT OF SCOPE— capture/replay (C13).tests/graph/test_graph_definition_integration.py::test_heat_diffusionandtest_graph_definition.py::test_instantiate_and_execute_host_callback; callable lifetime bytest_graph_definition_lifetime.py::test_python_callable_callback_survives_deland friends; signature rejection and bytesuser_databy thetest_host_callback_*cases; cleanup coalescing and shutdown safety bytest_user_object_cleanup_is_coalesced_on_python_thread,test_pending_call_queue_saturation_preserves_cleanupandtest_pending_cleanup_is_safe_during_python_shutdown.Limits
sys.unraisablehookis process-global, so the reporting test is markedthread_unsafe, following the existing precedent intests/test_program.py.