[STF] Preserve read-only imports across nested scopes - #11036
Conversation
Keep inherited read-only data frozen as read-only so independent sibling graph scopes do not acquire false ordering dependencies.
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Enterprise Run ID: 📒 Files selected for processing (1)
Included review availability: Your plan provides up to 12 included reviews per hour; 9 remain after this review. 📝 WalkthroughSummary by CodeRabbit
WalkthroughChanges
Suggested reviewers: Merge Risk: ⚪ Minimal · up to This localized change preserves read-only behavior across nested scopes and adds regression coverage; no actionable merge-blocking risk remains beyond normal checks and review. Comment |
| return result; | ||
| } | ||
|
|
||
| static void expect_independent(cudaGraph_t graph, cudaGraphNodeType type) |
There was a problem hiding this comment.
we can do better topology tests (have done in the past)
There was a problem hiding this comment.
The test now captures each graph's dependency structure once in a graph_topology helper (nodes, per-node direct dependencies, type queries) and runs all reachability checks on the cached map; independence checks also require a common transitive ancestor so they cannot pass vacuously, and conditional bodies must contain a kernel (27af4e1). If the better mechanism you had in mind is something else from past work, happy to adopt it. -- Grégoire
andralex
left a comment
andralex
left a comment
There was a problem hiding this comment.
Review by Grégoire (posting from Andrei's account). The fix is correct for the targeted path: a read access whose ancestor import is frozen read-only now inherits read instead of escalating to rw, so popping the first sibling no longer publishes a write prerequisite. The structural topology test is the right instrument; the transitive walk traverses intermediate nodes of any type, so a false edge routed through an upload or allocation node is still caught.
Inline comments below. One question on the header (the write-through-read-only-import case), one simplification, and a set of test hardenings. Each test comment is written as a self-contained spec so it can be handed to an implementation agent as-is.
- capture each graph's dependency structure once (graph_topology) instead of re-querying the CUDA API per reachability walk - expect_independent also requires a common transitive ancestor so the check cannot pass vacuously if the scopes stop sharing their input - add multi-hop coverage: read-only import propagated through an intermediate scope that never touches the data - pin the eager-rw root import behavior (two root readers serialize) so a future policy change flips a test deliberately - require non-empty conditional bodies in the while-scope case - dump a verbose DOT of the graph on any topology check failure Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
The new-import branch of validate_access now enforces the same access_mode_permits rule as the already-imported branch: a mutating request through an import frozen read-only aborts with a diagnostic instead of silently pushing rw through it. With the check in place, inherited_read_only reduces to the import freeze being read. The ancestor walk and the push-path construction were the same loop written twice; build the path while locating the deepest import. Also document why root data still imports rw on a read access (eager write capability versus sibling serialization). Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
|
/ok to test 069660b |
A failed topology check no longer writes files by default; it names the CUDASTF_DUMP_GRAPHS switch in the failure message instead, matching the dump conventions in stackable_ctx_impl. CI stays file-free. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
😬 CI Workflow Results🟥 Finished in 1h 30m: Pass: 69%/63 | Total: 20h 32m | Max: 56m 13s | Hits: 27%/64500See results here. AI failure analysis1. sibling_scope_dependencies: cudaGraphNodeGetParams is undefined · 15 jobsExplanation: The new conditional-body helper is enabled for CUDA Toolkit 12.4 and newer, but `cudaGraphNodeGetParams` is unavailable in every tested 12.9 and 13.0 configuration; NVIDIA documents the API in CUDA 13.2. citeturn1search0 All 15 GCC and Clang build jobs fail at the same call. Evidence: Copy this prompt into a coding agentJobs:
2. sibling_scope_dependencies: graph_scopes topology check aborts · 4 jobsExplanation: All four CUDA 13.3 test jobs compile successfully but abort in the first `graph_scopes` topology expectation. Because every predicate is reported through the same `check_or_dump` line and the DOT files are not among the collected artifacts, the logs cannot distinguish an unexpected node count, sibling dependency, or missing common ancestor. Evidence: Copy this prompt into a coding agentJobs: |
The previous pin pointed at an unrelated snapshot of the fork that lacks the two fixes the composed centrality experiment requires. Pin the tip of lab/cugraph-stf-sibling-fixes instead, which combines NVIDIA/cccl#11041 (stream-affine executable graph cache) and NVIDIA/cccl#11036 (preserve nested read-only imports) on upstream main, so a fresh checkout of this branch reproduces the measured composed-centrality behavior. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
…ions Two CI failure groups from run 33108185275, plus two review questions: - cudaGraphNodeGetParams only exists from CUDA 13.2 (it broke every 12.9 and 13.0 build), so expect_nonempty_conditional_bodies and its call are now guarded by _CCCL_CTK_AT_LEAST(13, 2). count_kernel_nodes_recursive only needs cudaGraphChildGraphNodeGetGraph and moves out of the guard. - The common-ancestor requirement in expect_independent aborted the 13.3 test jobs: the shared input is imported by the enclosing scope, whose transfer runs in the enclosing context's stream, so it is not a node of the inspected graph and sibling nodes correctly have no in-graph ancestor. Non-vacuity is now established by requiring real kernel work inside each sibling child graph (recursively), with the per-test value checks proving both siblings consumed the shared input. - Every topology predicate now reports a distinct message through check_or_dump; previously all failures pointed at the same EXPECT line and could not be told apart in CI logs. - Drop static from file-scope test helpers to match the convention of the other STF tests (review question on test_nested_graph_scopes). Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Both access_mode_permits checks in validate_access now report through the house EXPECT (always-on, message-carrying, throwing with source location) instead of fprintf+abort, per review; the failure becomes a catchable exception carrying the runtime mode names. Direct include of unittest.cuh added. Per review, the topology test's DOT dump is removed outright: the distinct per-predicate diagnostics move into the EXPECT message and the env-var machinery goes away. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
2 participants
Summary
Problem
Nested imports selected
rwfrom the logical data's root capability even when the parent scope had already frozen that data as read-only. Popping the first sibling therefore published a write prerequisite, producing falsechild_0 -> child_1andreset_0 -> conditional_1edges.Test plan
git diff --check