[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.
| _CCCL_ASSERT(imported_offset >= 0, ""); | ||
| } | ||
| const int imported_parent = sctx_ref.get_parent_offset(imported_offset); | ||
| const bool inherited_read_only = m == access_mode::read && imported_parent >= 0 && data().is_frozen(imported_parent) |
There was a problem hiding this comment.
Question, the one item I would settle before merge: in this new-import branch, a request with m = rw/write while the import at imported_offset is frozen read-only in imported_parent makes inherited_read_only false and pushes rw, freezing at imported_offset a data whose own import was read-only. The already-imported branch above (line 325) aborts through access_mode_permits for exactly this transition, one level up.
If the case is unreachable, an _CCCL_ASSERT here documenting why would close the question. If it is reachable, it needs the same access_mode_permits check and abort as line 325.
Agent guidance: after computing imported_parent, when imported_parent >= 0 && data().is_frozen(imported_parent), check access_mode_permits(data().get_frozen_mode(imported_parent), m) and abort with the same message format as the block at lines 325-332; then inherited_read_only reduces to m == access_mode::read under that same frozen-read condition.
There was a problem hiding this comment.
Implemented in 069660b: the new-import branch now applies access_mode_permits against the freeze at the deepest import and aborts on a mutating request through a read-only import, mirroring the already-imported branch; inherited_read_only reduces to the freeze being read.
| return true; | ||
| } | ||
|
|
||
| int imported_offset = ctx_offset; |
There was a problem hiding this comment.
This walk duplicates the path-building loop at line 354; both traverse from ctx_offset to the first was_imported level. Building path once and taking imported_offset as that loop's terminal current removes the duplicated termination logic.
Agent guidance: hoist the loop at 354 above the push_mode computation, record its final current as imported_offset, and delete this while.
There was a problem hiding this comment.
Implemented in 069660b: single walk builds the path and locates the deepest import.
| const int imported_parent = sctx_ref.get_parent_offset(imported_offset); | ||
| const bool inherited_read_only = m == access_mode::read && imported_parent >= 0 && data().is_frozen(imported_parent) | ||
| && data().get_frozen_mode(imported_parent) == access_mode::read; | ||
| const access_mode push_mode = |
There was a problem hiding this comment.
Scope note worth a comment in the code: inherited_read_only requires imported_parent >= 0, so two siblings reading a root-created logical data with no explicit outer push still import rw and serialize, the original bug shape one level shallower. If the eager-rw choice at the root is the intended tradeoff (avoid a re-push when a later scope writes, per the pop_before_finalize warning), one sentence here saying so spares the next reader the derivation.
There was a problem hiding this comment.
Implemented in 069660b: comment above push_mode documents the eager-rw root import tradeoff.
| using namespace cuda::experimental::stf; | ||
|
|
||
| static ::std::unordered_set<cudaGraphNode_t> transitive_dependencies(cudaGraphNode_t root) | ||
| { |
There was a problem hiding this comment.
Diagnostics: a topology failure in CI is much easier to read from a DOT dump than from an EXPECT line. Since this test exists because of a CI-only flake class, three lines buy a one-look diagnosis.
Agent guidance: in expect_independent/expect_ordered, on assertion failure (or unconditionally when an env var like STF_DUMP_GRAPH is set), call cudaGraphDebugDotPrint(graph, path, cudaGraphDebugDotFlagsVerbose) with a per-test filename and print the path to stderr.
There was a problem hiding this comment.
But we don't want to generate files in the CI, that's probably error prone ? (Or we need to use proper mechanisms to generate temporary file names and erase ....)
There was a problem hiding this comment.
Agreed, and fixed in b27bd9b: the dump is now gated behind the existing CUDASTF_DUMP_GRAPHS / CUDASTF_DEBUG_STACKABLE_DOT variables (same convention as stackable_ctx_impl), so a plain run and CI create no files; the failure message names the switch for local reproduction. -- Grégoire
There was a problem hiding this comment.
We can even further simplify and remove that dump, we will not run the tests in isolation with such env set...
There was a problem hiding this comment.
Removed outright in ad49b91: the DOT dump, the env-var gating, and the dependency are gone. The per-predicate diagnostics survive inside the EXPECT message (check_topology folds label and predicate into the exception text), so a CI failure still names which topology check fired. -- Grégoire
- 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>
| _CCCL_ASSERT(current >= 0, ""); | ||
| path.push(current); | ||
| const access_mode imported_mode = data().get_frozen_mode(imported_parent); | ||
| if (!access_mode_permits(imported_mode, m)) |
There was a problem hiding this comment.
There is no CCCL_EXPECT macro in the repo (EXPECT exists only in the test unittest header), and _CCCL_ASSERT compiles out in release builds, whereas this is a user-error diagnostic that should always fire and wants the runtime access-mode names in the message. The fprintf+abort mirrors the existing already-imported branch just above (and the freeze-mode check later in this file). If you'd rather have a dedicated always-on error macro for these, I'd suggest converting all three sites together in a follow-up. -- Grégoire
There was a problem hiding this comment.
Maybe this is worth using the EXPECT macro here for all of these ? (even if not strictly related to the PR)
There was a problem hiding this comment.
Done in ad49b91 — and you were right on both counts: EXPECT is not test-only (my earlier reply was wrong; it is used throughout the library headers as the house always-on check: stream_ctx, graph_ctx, logical_data, localized_array). Both access_mode_permits checks in validate_access now report through EXPECT with the runtime mode names in the message, turning the abort into a catchable exception with source location. Direct unittest.cuh include added. -- Grégoire
…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