[saiyambharara]

Description

closes #3106

The event-record case failed after exactly 30s on the MCDM runner, so the launch or the first is_done query blocked until the callback timeout. Drop the blocking callback and check the record target after completion with timing-enabled events, as suggested by @jpascucci-nv.

Checklist

• New or existing tests cover these changes.
• The documentation is up to date with these changes.

[copy-pr-bot]

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.

[coderabbitai]

Important

Review skipped

Review was skipped as selected files did not have any reviewable changes.

Run configuration

• Source mode: Auto
• Repository: NVIDIA/cuda-python
• Branch: main
• File: .coderabbit.yaml
• Commit: 2e6aa4e
• Layers: Repository YAML
• Review profile: CHILL
• Plan: Enterprise
• Run ID: 28369ccc-8f38-4a76-a7a6-390e113df6dc

Commits

Reviewing files that changed from the base of the PR and between f36374a and 62db871.

You can disable this status message by setting the reviews.review_status to false in the CodeRabbit configuration file.

Use the checkbox below for a quick retry:

• 🔍 Trigger review

No actionable comments were generated in the recent review.

Recent review info

Run configuration

• Source mode: Auto
• Repository: NVIDIA/cuda-python
• Branch: main
• File: .coderabbit.yaml
• Commit: b288fc8
• Layers: Repository YAML
• Review profile: CHILL
• Plan: Enterprise
• Run ID: 0b2bd1a2-1632-4cc4-9cbd-664d319ed84e

Commits

Reviewing files that changed from the base of the PR and between 5f9bd01 and f36374a.

Files selected for processing: 1

• cuda_core/tests/graph/test_graph_node_update.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
  • Updated event-record tests to verify that elapsed-time measurements are unavailable until both events have been recorded. After graph execution and synchronization, the tests confirm that elapsed-time measurement returns a numeric result. This replaces the previous check that relied on a blocking host callback to keep an event pending.

The event-record test now uses timing-enabled events and checks the recorded target after graph launch and synchronization. It removes the blocking host callback and checks that elapsed-time subtraction fails before the graph runs and succeeds afterward.

Changes: Event-record test

Layer / File(s)	Summary
Check event-record target after execution cuda_core/tests/graph/test_graph_node_update.py	The test uses timing-enabled events and a synchronized reference event. Elapsed-time subtraction raises before graph execution and returns a float after the graph launch and synchronization.

Assessment against linked issues

Objective	Addressed	Explanation
Remove the race in the event-record test by checking the record target after graph execution [#3106]	✅

Suggested reviewers: andy-jost

Priority: Low

Change: Bug fix · Severity of issue fixed: Low

Merge Risk: Minimal · previous review f3637

The test checks both event-record targets after graph execution, without the blocking callback. No outstanding issue prevents merging.

Finishing Touches: 💡 1

🛠️ Fix failing CI checks: 💡

• Commit to this branch
• Create a new PR

• Autofix · Keep fixing CodeRabbit findings and required CI, and resolving merge conflicts

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

[jpascucci-nv]

Sol 5.6 Ultra has this to say:

I don’t think this bounded poll covers the failure we actually observed. All three failures took 30.00–30.01s (new occurrence, original attempt 1, attempt 2), exactly matching callback_release.wait(timeout=30). The passing rerun completed this case in about 6 ms.

If cuEventQuery had immediately returned True for the untouched event, the finally block would have released the callback and the test would have failed quickly. Instead, either cuGraphLaunch or the first cuEventQuery appears to have blocked/serialized until the callback timed out and the downstream event-record node completed.
Therefore _wait_until is not actually bounded here:

• If graph.launch() blocks, polling begins after the event has completed.
• If expected.is_done blocks, the helper cannot check its five-second deadline until that predicate returns roughly 30 seconds later.
  CUDA also documents that any API call may block or synchronize and undocumented behavior should not be relied upon.
  Could we remove the blocking callback and verify the recording transition after completion using timing-enabled events?

+from cuda.core import Device, DeviceMemoryResource, EventOptions, LaunchConfig, LegacyPinnedMemoryResource

 def _event_record_case(device):
-    """Keep the selected event pending to identify each exec's record target."""
-    original = device.create_event()
-    replacement = device.create_event()
-    invalid_replacement = device.create_event()
+    """Identify each exec's record target after completion."""
+    event_options = EventOptions(timing_enabled=True)
+    original = device.create_event(options=event_options)
+    replacement = device.create_event(options=event_options)
+    invalid_replacement = device.create_event(options=event_options)
     invalid_replacement.close()

-    callback_started = threading.Event()
-    callback_release = threading.Event()
-
-    def blocking_callback():
-        callback_started.set()
-        callback_release.wait(timeout=30)
-
     graph_def = GraphDefinition()
-    callback_node = graph_def.callback(blocking_callback)
-    node = callback_node.record(original)
+    node = graph_def.record(original)

     def assert_exec_uses(graph, expected):
-        callback_started.clear()
-        callback_release.clear()
         stream = device.create_stream()
+        reference = stream.record(options=event_options)
+        stream.sync()
+
+        with pytest.raises(RuntimeError, match=r"^Both Events must be recorded"):
+            expected - reference
+
         graph.launch(stream)
-        try:
-            assert callback_started.wait(timeout=5)
-            _wait_until(lambda: expected.is_done is False)
-            unexpected = replacement if expected is original else original
-            assert unexpected.is_done is True
-        finally:
-            callback_release.set()
-            stream.sync()
+        stream.sync()
+        assert isinstance(expected - reference, float)```

This uses the documented behavior that [cuEventElapsedTime fails when either event has never been recorded](https://docs.nvidia.com/cuda/cuda-driver-api/cuda_driver_api/group__CUDA__EVENT.html). The test invokes old/original first and new/replacement second, so each expected event is fresh before its corresponding launch. We therefore don’t need to inspect the “other” event during the second invocation.

[saiyambharara]

@jpascucci-nv confirmed in the logs. All three failures took 30.00 to 30.01s, so the poll couldn't help. Pushed your version in f36374a. Locally the file passes, 1000 repeats pass, and the record-target checks still catch an ignored or leaked update. I injected each fault against both the old and new versions. My objection to the elapsed_time route on the issue was wrong, since each expected event is fresh before its launch. Could you /ok to test f36374a?

[jpascucci-nv]

/ok to test f36374a

[github-actions]

Doc Preview CI
Preview removed because the pull request was closed or merged.

[jpascucci-nv]

/ok to test 62db871

[jpascucci-nv]

@saiyambharara FYI, I rebased your change because it wasn't passing CI (due to a known and fixed problem).

[Andy-Jost]

Approving. The rewrite removes the cause of the 30 s failure rather than narrowing it: the graph is a single event-record node, so nothing can block the launch or the first query, and the elapsed-time check after the sync still fails when a definition update is ignored, leaks into the old exec, or an exec-node update is ignored. CI on this head is green on every Windows and WSL row, including the MCDM runners where #3106 failed.

One follow-up, not for this PR: the event-wait case (assert_exec_waits, around line 153) keeps the blocking-callback pattern and a 100 ms negative wait, so it can still meet the same timeout and cannot tell which event the exec waits on. A device-side proof of the wait target would close that; happy to take it in the follow-up you offered.