Skip to content

Add host-callback contract tests from the stream-callback survey - #2925

Open
0z5a wants to merge 3 commits into
NVIDIA:mainfrom
0z5a:0z5a/cp311-stream-callback-regressions
Open

0z5a wants to merge 3 commits into
NVIDIA:mainfrom
0z5a:0z5a/cp311-stream-callback-regressions

Conversation

@0z5a

@0z5a 0z5a commented Sep 20, 2026 •

Copy link
Copy Markdown

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 on main — 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, a cuStreamAddCallback wrapper, 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:

  • Exception reporting. The trampoline is 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 to sys.unraisablehook, graph.launch() and stream.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.
  • Raw user_data. user_data given as an int is the caller's own pointer and reaches a ctypes callback unchanged (_resolve_host_callback copies 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.
  • Two devices. Two devices, two graphs each, each graph with its own stream, device buffer and host slot: every callback observes exactly its own value, and the set of callbacks is exact.

Evidence

  • Base SHA: 8b6e9f52db1074a580fc1678c71e7c75c42db2ea
  • Tested SHA: 7cdc6395632c181d543f819a7f75c8056625592f — the commit in this PR (cuda_core/tests/graph/test_host_callback_contracts.py, sha256 21b71759af2f3a359f70c385cc8171127390c3a8846ed49074669d811a05111a); the branch carries that same commit object.
  • CUDA Python components: cuda-core 1.2.1.dev112+g8b6e9f52d built 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/cuda
  • GPUs: 2 × NVIDIA L20 (SM89), driver 595.91.07, NVML 13.595.91.07; devices 0 and 1 of the host (GPU-836d0605-… / GPU-739e6fcb-…)
  • Deterministic tests: cuda_core/tests/graph/test_host_callback_contracts.py — 3 passed; the file was run 20 consecutive times to check for flakiness (all green)
  • Second host (8 x RTX 5090, Python 3.12.13, cuda-bindings 13.4.2, CUDA 13.4 headers), same commit: 3 passed in 1.04 s, cuda_core/tests/graph 502 passed, 33 skipped, and cuda_core/tests in full 4317 passed, 0 failed — so the three assertions hold on both a 2-GPU and an 8-GPU machine
  • Regression: cuda_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 passed
  • Native tests: all three cases execute real graphs on the two L20s; nothing is mocked, and the multi-device case covers the two GPUs that were free on the host
  • Skipped or blocked cases: BLOCKED: 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 that main does not have yet; reported as blocked rather than asserted or failed. OUT OF SCOPE — capture/replay (C13).
  • Duplicates deliberately not added: prerequisite-data visibility and post-callback ordering are already covered by tests/graph/test_graph_definition_integration.py::test_heat_diffusion and test_graph_definition.py::test_instantiate_and_execute_host_callback; callable lifetime by test_graph_definition_lifetime.py::test_python_callable_callback_survives_del and friends; signature rejection and bytes user_data by the test_host_callback_* cases; cleanup coalescing and shutdown safety by test_user_object_cleanup_is_coalesced_on_python_thread, test_pending_call_queue_saturation_preserves_cleanup and test_pending_cleanup_is_safe_during_python_shutdown.

Limits

  • No implementation change is included: the survey reproduced no defect in the existing helper, so there is nothing to fix yet. If the stream-side work later reproduces one, the fix should be a separate, narrow change.
  • Callback concurrency is not asserted, and must not be: CUDA does not define ordering between independent streams or callbacks, so the tests only check each graph's own order, count and data.
  • No fault injection was attempted (no XID/ECC/GPU-lost, no driver unbind, no context corruption) — that is not acceptable on a shared host, and the tests therefore do not claim those paths.
  • Exceptions raised inside a ctypes callback are not covered: the exception cannot cross the C ABI, so such a case would be an invalid use rather than a contract.
  • sys.unraisablehook is process-global, so the reporting test is marked thread_unsafe, following the existing precedent in tests/test_program.py.

@copy-pr-bot

copy-pr-bot Bot commented Sep 20, 2026

Copy link
Copy Markdown
Contributor

This pull request requires additional validation before any workflows can run on NVIDIA's runners.

Pull request vetters can view their responsibilities here.

Contributors can view more details about this message here.

@github-actions github-actions Bot added the cuda.core Everything related to the cuda.core module label Sep 20, 2026
@0z5a

0z5a commented Sep 20, 2026

Copy link
Copy Markdown
Author

Re-verified on a second host (8 × RTX 5090)

No code change since the first push — this is the same commit 7cdc63956 — but the three cases were re-run on a second machine to check that they are not tuned to one stack: 8 × RTX 5090 (SM120), driver 580.82.07, Python 3.12.13, cuda-bindings 13.4.2, CUDA 13.4 headers.

  • test_host_callback_contracts.py: 3 passed in 1.04s (same file, sha256 21b71759… as in the commit).
  • cuda_core/tests/graph on that host: 502 passed, 33 skipped.
  • cuda_core/tests in full: 4317 passed, 0 failed.

The first host was 2 × L20 with Python 3.10 and cuda-bindings 12.9.8, so the three contract assertions (exception reporting through sys.unraisablehook, caller-owned int user_data delivered unchanged, two devices keeping their own callback parameters) hold on both a 2-GPU and an 8-GPU machine.

Still unchanged and still waiting on the split answer: the stream-side matrix (C01–C05, C09) remains BLOCKED: dependency until the ordinary-stream callback entry from #2058 exists, and capture/replay stays out of scope.

@0z5a
0z5a marked this pull request as ready for review September 21, 2026 08:22
rparolin

This comment was marked as low quality.

rparolin

This comment was marked as low quality.

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>
@Andy-Jost

Copy link
Copy Markdown
Contributor

/ok to test d77759e

@Andy-Jost Andy-Jost left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

  1. cuda_core/tests/graph/test_host_callback_contracts.py line 43: sys.unraisablehook is replaced before the try/finally that restores it. The calls on lines 47-49 (create_stream, allocate, and LegacyPinnedMemoryResource().allocate) run outside the try that starts at line 55. If one of them raises, the finally never runs, pytest loses its session hook, and every later unraisable report in the run is swallowed. Use monkeypatch.setattr(sys, "unraisablehook", records.append) as cuda_core/tests/test_program.py does, or move lines 42-49 inside the try.

  2. cuda_core/tests/graph/test_host_callback_contracts.py lines 37, 82, and 112: the three tests have no authorship marker. AGENTS.md asks for one on each new test: @pytest.mark.human_authored, @pytest.mark.agent_authored(model="..."), or @pytest.mark.human_reviewed. The markers are registered in cuda_core/pytest.ini, and the nearby host-callback tests in test_graph_definition.py carry them. Please add the marker that matches the origin of each test.

Nits

  1. 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_launch lands. Please trim the docstring to the three contracts in lines 10-15.

  2. Lines 48-49, 87, 117, and 130: buffers are constructed and never released. cuda_core/tests/AGENTS.md asks 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_launch exists. 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.

@github-actions

Copy link
Copy Markdown
Contributor

0z5a and others added 2 commits October 23, 2026 21:45
…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>
@Andy-Jost

Copy link
Copy Markdown
Contributor

I merged main into this branch to pick up #3188 (cuDSS 1.0 library names), which the pathfinder CI step needs now. No other change. CI runs again on the new head.

@Andy-Jost

Copy link
Copy Markdown
Contributor

/ok to test a908c7c

@coderabbitai

coderabbitai Bot commented Oct 23, 2026

Copy link
Copy Markdown

Review in Change Stack →

No actionable comments were generated in the recent review. 🎉

ℹ️ Recent review info
⚙️ Run configuration
📥 Commits

Reviewing files that changed from the base of the PR and between d5f37e0 and a908c7c.

📒 Files selected for processing: 1
  • cuda_core/tests/graph/test_host_callback_contracts.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.

📝 Summary

Summary by CodeRabbit

  • Tests
    • Added coverage for reporting Python callback exceptions while allowing graph execution and later launches to complete.
    • Added checks that ctypes callbacks receive the supplied user-data pointer unchanged and that callback parameters remain separate across devices and streams.

Adds graph host-callback contract tests for exception reporting, caller-owned user data pointers, and distinct callback parameters across devices and streams.

Changes: Graph host-callback contracts

Layer / File(s) Summary
Callback exception reporting
cuda_core/tests/graph/test_host_callback_contracts.py
Tests that callback exceptions reach sys.unraisablehook and that graph completion and a later callback still occur.
Caller-owned user data
cuda_core/tests/graph/test_host_callback_contracts.py
Tests that a ctypes callback receives an integer pointer unchanged and can read the pointed-to byte.
Callback values across devices and streams
cuda_core/tests/graph/test_host_callback_contracts.py
Tests that callbacks observe the expected label/value pairs across two devices and multiple streams.

Possibly related PRs

  • NVIDIA/cuda-python#3194: Documents the Python host-callback path and its non-propagating callback error channel, which the exception-reporting test exercises.

Suggested labels: test

Suggested reviewers: andy-jost

Priority: ⬇️ Low

Change: Other

Merge Risk: ⚪ Minimal · up to a908c

The added contract tests have no identified merge-blocking issue; merge after normal checks.

- [ ] Autofix · Keep fixing CodeRabbit findings and required CI, and resolving merge conflicts

Comment @coderabbitai help to get the list of available commands.

This branch has not been deployed

No deployments
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

cuda.core Everything related to the cuda.core module P1 Medium priority - Should do

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants