Repository navigation
Conversation
✅ All Checks Passed — Ready for Review
📖 Need help? See the Policy FAQ for details on every check and how to fix failures. |
therock-pr-bot
Bot
added
the
Not ready to Review
|
🎉 All checks passed! This PR is ready for review. |
…overrides Move test-selection overrides into BUILD_TOPOLOGY.toml and drop the stale test_subprojects_overrides.json references from the comments. Co-Authored-By: Claude Opus 4 (1M context) <noreply@anthropic.com>
Re-adds the env-var short-circuit in configure() so downstream test workflows can force changed_projects without hitting the gh-api change detection path. Co-Authored-By: Claude Opus 4 (1M context) <noreply@anthropic.com>
Add RFC0013 proposing derived test-subproject selection from the CMake consumer graph plus BUILD_TOPOLOGY overrides, including DEFCON gating-level background, and link it in the RFC index. Co-Authored-By: Claude Opus 4 (1M context) <noreply@anthropic.com>
Adds a section framing DEFCON 4 as one policy over the consumer graph and describes how levels 1-5 map to fan-out reach, plus the test-tier dimension needed for true levels 1/2/5. Co-Authored-By: Claude Opus 4 (1M context) <noreply@anthropic.com>
937445c to
8f468a4
Compare
Pre-commit check failed⛔ pre-commit failed Please run locally:
This repo uses |
ScottTodd
left a comment
ScottTodd
left a comment
There was a problem hiding this comment.
I left some initial comments on the design.
In general I think we do need this dynamic graph analysis in some form to make CI build/test selection scale well.
| baseline derivable rather than curated, and the override keys become the knobs for | ||
| moving a component off that baseline. | ||
|
|
||
| [defcon]: https://amd.atlassian.net/wiki/spaces/MLSE/pages/1795331309/DEFCON+Enablement+Gating+Levels |
There was a problem hiding this comment.
No direct links to internal pages in open source code. You can indirect through a u.amd.com shortlink as needed, but if we want to refer to this from open source the design should be public (I also don't know if we'd want to use the term "defcon" - @astyrrian1 ?)
There was a problem hiding this comment.
I'll remove the direct amd.atlassian.net link and can add a short public summary of the gating levels so the RFC is self-contained.
On the "defcon" term itself — I'd like to wait for confirmation from others (@astyrrian1, @suchitravemuri1) before renaming. If we settle on a different public name for the levels I'll rename throughout.
| ## Background: DEFCON gating levels | ||
|
|
||
| This work exists to implement **DEFCON 4** per-PR gating across TheRock's | ||
| components. DEFCON is the ROCm enablement-gating scheme defined in | ||
| [DEFCON Enablement: Gating Levels][defcon] (MLSE space): a per-component tier | ||
| that fixes *how much* every PR must build and test, sized to the component's | ||
| downstream blast radius and regression history. | ||
|
|
||
| | Level | What every PR must build & test | Intended for | | ||
| | ------------ | ------------------------------------------------------------------- | -------------------------------------------------------- | | ||
| | **DEFCON 1** | Full QA integration + nightly suites | Components actively destabilizing releases | | ||
| | **DEFCON 2** | Frameworks built and their tests run | High downstream blast radius (e.g. framework-facing libs)| | ||
| | **DEFCON 3** | All rocm-libraries projects/tests + rest of rocm-systems (rccl etc.)| Broad-dependency components with regression history | | ||
| | **DEFCON 4** | Local + intimately related project builds/tests only | Well-tested components with contained surface | | ||
| | **DEFCON 5** | Fast unit tests only | Proven track record + strong local test coverage | |
There was a problem hiding this comment.
By this definition, isn't this between 3 and 4?
4 is just "intimately related projects", e.g.
- hip-clr edited -> test hip-tests
- rocRoller edited -> test hipBLASLt (which directly depends on rocRoller), do NOT test MIOpen (which directly depends on hipBLASLt and indirectly depends on rocRoller)
3 would be "all projects/tests" which is for "I don't trust the build/test system to compute the impact of this change for me, so just build/test everything"
What using the build graph lets us do is compute direct and indirect users of each project and decide how far along those dependency graph edges we want to walk, e.g.:
- rocRoller edited - > test hipBLASLt and MIOpen
- hip-clr edited -> test all users of it (rocRAND, rocPRIM, rocRoller, hipBLASLt, MIOpen, etc.), do NOT test projects that don't have a direct or indirect dependency on hip-clr, like rocr-debug-agent or rdc
| - **Drift and false greens.** A stale or missing entry produces a passing CI run | ||
| that never exercised the affected downstream suite: the most dangerous class of | ||
| gap because it is invisible. |
There was a problem hiding this comment.
Here's a recent example: ROCm/rocm-systems@920e5a8
A change to https://github.com/ROCm/rocm-systems/tree/develop/projects/amdsmi broke https://github.com/ROCm/rocm-systems/tree/develop/projects/rdc and the CI didn't catch it because https://github.com/ROCm/rocm-systems/blob/develop/.github/scripts/therock_matrix.py was not modeling the build dependency relationship there.
| 3. **Derive test subprojects in Python.** `determine_rocm_test_dependencies.py` | ||
| loads the graph, computes each subproject's build stage from the committed | ||
| `artifact-*.toml` descriptors + `BUILD_TOPOLOGY.toml`, and for each changed | ||
| subproject selects its **same-stage** direct consumers. | ||
| 4. **Apply explicit overrides from `BUILD_TOPOLOGY.toml`.** Couplings the graph | ||
| cannot express (test-only projects with no link edge, cross-stage couplings, | ||
| and stage-less foundational deps) are declared with `test_include` / | ||
| `test_exclude` / `test_fanout_all` keys on the relevant artifact. |
There was a problem hiding this comment.
I'm not sure I agree with the usage of BUILD_TOPOLOGY.toml stages here. "cross-stage couplings" are commonplace, e.g. many projects depend on amd-hip (the compiler), hip-clr (the runtime), and rocprofiler-sdk (the profiler).
| ### The "same-stage cut" | ||
|
|
||
| Selecting *all* transitive consumers of a foundational dependency (e.g. | ||
| `hip-clr`, `ROCR-Runtime`, the compiler) would pull in nearly the entire test | ||
| tree: every math lib, ML lib, comm lib, etc. To keep selection bounded, the tool | ||
| only selects consumers **in the same BUILD_TOPOLOGY build stage** as the changed | ||
| subproject. Cross-stage consumers are treated as "universal" and cut. | ||
|
|
||
| Where a specific cross-stage or test-only coupling genuinely must run, it is | ||
| declared explicitly (see Overrides). This makes the expensive, broad selections | ||
| opt-in and auditable rather than accidental. | ||
|
|
||
| The same-stage cut is what keeps the default at **DEFCON 4** ("local + intimately | ||
| related") rather than sliding into DEFCON 3 ("all rocm-libraries / rocm-systems"). | ||
| Selecting every transitive consumer of a foundational dep is effectively DEFCON 3 | ||
| behavior on every touch of that dep; the cut confines the automatic selection to | ||
| the directly-related, same-stage neighborhood and keeps runs inside the 10–15 min | ||
| SLA. Deliberately widening a component toward DEFCON 3/2 is then an explicit, | ||
| reviewable act (`test_fanout_all` / `test_include`) rather than an accident of the | ||
| graph's shape. |
There was a problem hiding this comment.
Relating to my other comments... well yeah, if something depends on hip-clr then its builds/tests could be affected by changes there. I don't think "build stage" is the right boundary here. Viewing this through the lens of "direct" and "indirect" deps may be more useful.
The framing here as "an accident of the graph's shape" doesn't make sense to me - if a project takes a dependency on another project that's no accident.
There was a problem hiding this comment.
Agree with all the comments related to build stages. I'll come up with a plan on how to implement direct, transitive dependencies, update the RFC for another round of review.
| ### CI integration | ||
|
|
||
| The change-detection job in `test_artifacts.yml` gains three steps before it | ||
| determines test dependencies: | ||
|
|
||
| 1. `pip install -r requirements.txt` | ||
| 2. `fetch_sources.py` — all subproject source trees must be present so every | ||
| `therock_cmake_subproject_declare()` runs and the emitted graph is complete. | ||
| 3. A configure-only `cmake -B build -DTHEROCK_ENABLE_ALL=ON ...` that emits | ||
| `build/therock_consumer_graph.json` as a side effect. | ||
|
|
||
| `determine_rocm_test_dependencies.py` then reads the freshly emitted graph. The | ||
| graph is never committed, so it cannot drift from the build definitions. |
There was a problem hiding this comment.
This is pretty expensive to run: https://github.com/ROCm/TheRock/actions/runs/30411761253/job/90461735601?pr=6941. It takes around 6 minutes on a GitHub-hosted runner (4m40s on checkout, 1m40s on configure).
We'll also want more of this information to be available prior to building, so we can ensure
- all projects we plan on testing are also built
- (if we want) only projects we plan on testing are built
Some ideas:
- Generate the graph and check it in / cache it somehow, so we don't need to recompute with this dynamic code
- Have the CI configure step that runs at the start of the pipeline compute the graph (something similar is needed for build artifact reuse too, cc @erman-gurses @ammallya )
- Have each build stage emit a subset of the graph and then merge those subgraphs back together when running tests
There was a problem hiding this comment.
Ack. My earlier idea was to generate the graph, check it in and have a CI configure step. However, I didn't want to introduce a new json file to check in and went with dynamically generating each time.
I'll explore adding the consumer_graph.json and add a CI step to validate at the start of the pipeline.
There was a problem hiding this comment.
Agreed the dynamic-every-run configure is too expensive on the per-PR hot path - I've moved off it in the RFC. The approach I'm proposing:
- Commit therock_consumer_graph.json to the repo. The per-PR change-detection job reads the checked-in file directly — no cmake, no source fetch on the hot path.
- Add a CI validity check that regenerates the graph and fails if the committed copy is stale. It runs only when dependency-related files change (**/CMakeLists.txt, the emit/registration cmake, the committed graph itself), so the ~6-minute configure is confined to that low-frequency job instead of every PR. This turns silent drift into a CI failure similar to the contract BUILD_TOPOLOGY.toml already carries.
There was a problem hiding this comment.
That works for me. The main thing I want to avoid is having a complex graph that is only manually updated, as then we can easily overlook that graph going stale or accidentally missing dependency edges that it should have. A dynamically generated graph or a static graph that is produced by a script (ideally with CI enforcing that it stays up to date) would both be okay.
| ### What the generalization adds | ||
|
|
||
| 1. **A `defcon` key** on `[artifacts.<name>]`, overridable in the | ||
| `test_overrides.<subproject>` sub-table and resolved by the existing | ||
| `get_test_overrides_for_subproject()` path. Absent means the default level 4, | ||
| which is exactly the RFC's "Phase 1 starts every component at DEFCON 4." |
There was a problem hiding this comment.
I like the idea of having a clear place to say "changes to these code paths/project should use this level of testing". I'm not convinced that the topology or artifacts are the right place for that though. Need to think on this some more.
There was a problem hiding this comment.
ACK
If topology or artifacts are not the right places, I'm leaning toward a dedicated file at the root or in testing_tools, e.g. test_policies.toml, keyed by component, carrying both the per-component level and any test_include / test_exclude. Keying by component removes the artifact-sharing issue and gives one clear place to answer "what testing does a change to this component trigger?".
Rough sketch:
[component.hipblaslt]
level = 4
test_include = ["hipsparselt"]
Let me know if you have other suggestions. Otherwise, I can document this approach in RFC and implement.
There was a problem hiding this comment.
I'm coming around to the idea of putting more code in https://github.com/ROCm/TheRock/tree/main/test_tools specifically for these sorts of details in the CI system.
Ideally I'd like to have what build systems like Bazel have, where all build/package/test/etc. targets are known to the build system and you can just run bazel test and have it understand what targets already have cached outputs/results and which need to get built/run. To that end, I've hoped we could get the CMake superproject here to support similar features as natively as possible via our CMake modules and supporting files like BUILD_TOPOLOGY.toml.
I think a reasonable middle ground between "the build system fully models all dependencies and knows how to cache" and "we manually specify every dependency relationship and testing level for each project" would be to have code in https://github.com/ROCm/TheRock/tree/main/test_tools or a similar location that is generated from knowledge about the build system and has a few escape hatches / customization points where we can say "changes to this project should by default also build/test projects 'foo' and 'bar'" or "changes to this project should by default build/test all projects that depend on it: [generated list]"
There was a problem hiding this comment.
That matches with where I've landed. Concretely I'd split it into two files:
therock_consumer_graph.json: generated, CI-monitored. The full reverse dependency graph, emitted from the build system, no hand editing. The validity check from the CI-cost thread is the "CI enforcing it stays up to date" part.
A hand-maintained overrides file (e.g. test_policies.toml): small and reviewed. Only the escape hatches, per-component level (how far to walk the generated graph) plus test_include / test_exclude for what the graph can't express.
The escape hatches you named map directly: "also build/test component a and b" -> test_include;
"ignore testing component c" -> test_exclude;
"build/test all projects that depend on it" -> level = 3 (transitive walk over the generated graph, so the list stays generated rather than pasted in).
I'd keep the generated graph and the overrides as separate files because they have opposite lifecycles: the graph is machine-generated and shouldn't be hand-edited (edits get clobbered on regen, and the whole point is it can't drift), the overrides are human intent. Splitting them lets the validity check just regenerate-and-diff the graph, and the overrides get reviewed like code, with no file that's half-generated and half-authored.
Drawbacks of the two-file split, to be upfront:
Two places to look. Answering "what does a change to X test?" means reading the graph and the overrides together instead of one entry. I'd add a determine_rocm_test_dependencies.py helper that prints the resolved selection for a component so we rarely compose by hand.
Referential integrity across files. An override can name a component that no longer exists in the graph, and the override file alone won't catch it. Needs a small validation step that checks override keys against the current graph.
Two-step edit sometimes. A genuinely new dep edge regenerates the graph (automatic) and may need an override tweak (manual), where a single hand-file would be one edit (but then it isn't generated at all).
Precedence has to be defined. With level plus include/exclude on a generated base, the resolution order (walk to level -> apply include -> apply exclude) needs to be specified and tested so results are deterministic.
I think the split is worth it, it's the only way the graph half stays CI-owned and drift-proof while the intent half stays hand-reviewed, and the drawbacks are all addressable with a resolver helper + an override validation check.
| 2. **Emit a consumer graph.** At the end of the top-level configure, | ||
| `therock_emit_consumer_graph()` serializes the registry to | ||
| `build/therock_consumer_graph.json`, a configure-time side effect analogous to | ||
| `compile_commands.json`. It is **dynamic and never committed**. |
There was a problem hiding this comment.
Can you post the current therock_consumer_graph.json somewhere we can look at it? This comment thread, a gist linked from the PR description, etc.
There was a problem hiding this comment.
I will check in therock_consumer_graph.json once I build it with direct, indirect consumers.
Add the drift-check tooling (check_consumer_graph_drift.py + test), its CI workflow, the committed consumer graph and test_policies.toml, and wire the emitter/topology changes. Rename RFC0013 to Consumer-Based-Test-Selection and refresh its references.
Set all components to level 4 (default) and add test_include couplings so the graph walk plus overrides covers the tracked downstream dependent set. rocr-runtime uses level 3 to auto-derive its broad transitive closure, pinning only the graph-unreachable couplings (amd-dbgapi, ocl-icd, rocgdb).
Drops the env-var short-circuit in configure_external_repo_ci.py (reverts dfbc289) and its RFC0013 mention. The override is retained on the ...-testing-e2e branch for end-to-end selection testing; keep it out of the mergeable change.
therock-pr-bot
Bot
removed
the
Not ready to Review
ScottTodd
left a comment
ScottTodd
left a comment
There was a problem hiding this comment.
Architecturally LGTM. A mix of comments on style, specific policies to apply to individual subprojects for correctness, and some details on how to integrate into our existing infrastructure then I'll be okay with approving.
| - name: Check committed consumer graph is up to date | ||
| # Compares the freshly emitted graph (build/) against the committed copy | ||
| # (test_tools/) after normalizing both the same way, and fails on any real | ||
| # edge change. Logic lives in a unit-tested script, not inline here. | ||
| run: python3 ./build_tools/github_actions/check_consumer_graph_drift.py --check |
There was a problem hiding this comment.
No need for verbose comments like "Logic lives in a unit-tested script, not inline here.", the code already expresses that.
| # Resolution order: walk to the component's level -> union test_include -> | ||
| # subtract test_exclude (last). Every component KEY must name a real | ||
| # consumer-graph key (it is the change trigger; a cross-file validation, task #5, | ||
| # fails the build otherwise). test_include / test_exclude VALUES need NOT be graph | ||
| # keys: their whole purpose is to name test-only targets the graph cannot express | ||
| # (e.g. ctest suites like rocgdb-cpu). There is no `test_fanout_all`: "test all | ||
| # downstream consumers" is expressed as `level = 3`. |
There was a problem hiding this comment.
What is task #5? (I've seen internal task planning state leak into code comments authored by AI agents, did that happen here?)
There was a problem hiding this comment.
My bad, I cleaned up all internal references, other internal comments but, clearly some escaped. Let me fix.
| # Components are DEFCON 4 by default (level 4 = self + direct consumers, so the | ||
| # level key is omitted). A few broad-blast-radius runtimes set level = 3 (full |
There was a problem hiding this comment.
nit: this sentence is redundant with the table above that says the default level is 4. Could trim these comments down a bit.
| # UNION (test_include) covers the dependent set from the "DEFCON Enablement: | ||
| # Gating Levels" tracking page. |
There was a problem hiding this comment.
What is the "DEFCON Enablement: Gating Levels tracking page"? (careful about notes that make sense to an agent with context spilling into open source code that needs to stand on its own)
There was a problem hiding this comment.
Internal page, will remove.
| # --- Foundational runtimes: broad downstream dependent sets -------------------- | ||
| # rocr-runtime / hip-clr / amd-llvm sit under most of the tree. Their direct | ||
| # graph consumers miss many real downstream projects (framework-facing libs, test | ||
| # harnesses), so the DEFCON 4 dependent list is carried explicitly here. |
There was a problem hiding this comment.
The framing of this comment is confusing. The amd-llvm compiler is used to compile ~all projects and it is very close to the root of the build graph, so of course the "direct graph consumers" list is short for it. Changes to the compiler should build all projects that use the compiler, direct dependency or otherwise.
Note that deps on the compiler are carried via COMPILER_TOOLCHAIN (amd-hip or amd-llvm) and not BUILD_DEPS or TEST_DEPS. For example:
TheRock/math-libs/CMakeLists.txt
Lines 60 to 73 in bfe0b55
There was a problem hiding this comment.
This makes sense, I will update to use COMPILER_TOOLCHAIN for building consumer graph for compilers.
| def get_subprojects_to_test( | ||
| changed_subprojects: list[str], |
There was a problem hiding this comment.
What is this function expecting for "changed_subprojects"? My review agent caught this:
The old stage-map mismatch was removed, but the end-to-end pipeline still uses
at least three different identifiers without an authoritative mapping:
configure_external_repo_ci.py
emits external-repository subtree paths such asprojects/clrand
shared/rocroller.determine_rocm_test_dependencies.py
strips onlyprojects/, then looks the resulting string up in a graph keyed
by CMake target names such aship-clr,rocroller, and
hipdnn_integration_tests.fetch_test_configurations.py
schedules jobs only on an exact match with test-matrix keys such as
hipdnn-integration-testsandhipdnn-samples.Concrete current-head results:
projects/clr -> warning; ["clr"] shared/rocroller -> warning; ["shared/rocroller"] projects/hipdnn-integration-tests -> warning; ["hipdnn-integration-tests"]The first two select no component job beyond sanity. The policy has the same
problem:test_policies.toml
addshipdnn_integration_testsandhipdnn_samplesfor MIOpen and
hipkernelprovider, while the real matrix keys are
hipdnn-integration-testsandhipdnn-samples.
An end-to-end local run for a MIOpen change scheduledmiopenand
miopenprovider, but silently omitted both intended hipDNN suites.The unit tests stop at the selector's returned set, so they accept names that
cannot create a real CI job.
There was a problem hiding this comment.
Earlier I was using an test override to mimic changed subprojects in rocm-systems, rocm-libraries. I removed that logic from here, created another test branch off of it for my e2e testing.
| A missing file yields empty policies (so every component uses the default | ||
| level and no include/exclude) rather than crashing — useful for clean-checkout | ||
| introspection, though the file is committed and normally present. | ||
| """ | ||
| test_deps = {} | ||
| cmake_files = list(Path(therock_dir).rglob("CMakeLists.txt")) | ||
| policies_path = _test_policies_path(therock_dir) | ||
| if not policies_path.exists(): | ||
| return {} |
There was a problem hiding this comment.
Seems like a missing file should raise an exception rather than default to empty policies: https://github.com/ROCm/TheRock/blob/main/docs/development/style_guides/python_style_guide.md#fail-fast-behavior
| The per-component level, plus test_include/test_exclude, come from the | ||
| hand-maintained `test_tools/test_policies.toml`. A component with no table there | ||
| uses the default level 4 (depth 1) with no include/exclude. See the schema | ||
| contract in docs/development/test_selection_schemas.md. |
There was a problem hiding this comment.
test_selection_schemas.md does not exist. Redirect to the RFC in docs/rfcs?
| hand-authored regions. Keeping them apart lets the graph be fully CI-owned | ||
| (regenerate-and-fail-on-diff) and the policy be fully human-owned (reviewed like | ||
| code). This mirrors ScottTodd's "generated from build-system knowledge, with a few | ||
| escape hatches" framing: the graph is the generated knowledge, the policy file is | ||
| the escape hatches. |
There was a problem hiding this comment.
nit: can drop the reference to my specific username and say instead "during review we decided..." or similar ;) the full discussion history is here on the PR
| - **Level naming in open source.** The "DEFCON" label is internal. Should the | ||
| public RFC and code use a different name for the gating levels? (Pending input | ||
| from @ScottTodd / @astyrrian1.) |
There was a problem hiding this comment.
lol, @astyrrian1 @suchitravemuri1 how you feel about us using "DEFCON" as the public name for these test tiers?
There was a problem hiding this comment.
My bad, I cleaned up all internal references, other internal comments but, clearly some escaped. Let me fix.
|
Just so I understand the impact of this change on ROCgdb CI and what we're trying to achieve here. We're throwing away TEST_SUBPROJECTS entirely and replacing it with another mechanism that does things automatically? I went through the generated consumer graph json and there might be a gap here. Take for instance ROCgdb. ROCgdb does not depend directly on rocr-runtime, but when ROCgdb runs its tests that definitely depends on rocr-runtime. In fact that's been a frequent area of uncaught regressions from rocm-systems changes. Could we please make sure we have that covered? Another point is that we have a ROCgdb testing split to optimize capacity usage. rocgdb-cpu tests only have to be exercised due to changes in the amd-llvm compiler. rocgdb-gpu needs to be exercised whenever amd-llvm or rocr-runtime/clr change etc. Do we have any provisions to cater for that split so we don't run useless tests when not required? |
| [component.rocgdb] | ||
| test_include = ["rocgdb-cpu", "rocgdb-gpu"] |
There was a problem hiding this comment.
Just so I understand the impact of this change on ROCgdb CI and what we're trying to achieve here. We're throwing away TEST_SUBPROJECTS entirely and replacing it with another mechanism that does things automatically?
I went through the generated consumer graph json and there might be a gap here. Take for instance ROCgdb. ROCgdb does not depend directly on rocr-runtime, but when ROCgdb runs its tests that definitely depends on rocr-runtime. In fact that's been a frequent area of uncaught regressions from rocm-systems changes.
Could we please make sure we have that covered?
Another point is that we have a ROCgdb testing split to optimize capacity usage. rocgdb-cpu tests only have to be exercised due to changes in the amd-llvm compiler. rocgdb-gpu needs to be exercised whenever amd-llvm or rocr-runtime/clr change etc. Do we have any provisions to cater for that split so we don't run useless tests when not required?
I would expect:
- This policy says "when rocgdb is modified, run rocgdb-cpu and rocgdb-gpu"
- A policy for rocr-runtime of
says "when rocr-runtime is changed, test every project that depends on rocr-runtime directly or indirectly"
[component.rocr-runtime] # ROCr is used by many projects so test self + transitive users level = 3
- A policy for rocr-runtime of
(using the default level) would instead say "when rocr-runtime is changed, test rocr-runtime itself and the one additional project 'rocgdb'"
[component.rocr-runtime] test_include = ["rocgdb"]
As for the cpu/gpu split, I don't see a clear spot for that in the current design. We could add some additional tiers of tests along those lines 🤔
There was a problem hiding this comment.
For cpu, gpu split, can we add explicit excludes for amd-llvm, hip-clr changes like below? I need to test, but this should work. If we need to add more specific tiers of tests then we can do that in the next iteration once this is merged.
[component.amd-llvm]
test_include = ["rocgdb-cpu"]
test_exclude = ["rocgdb-gpu"]
There was a problem hiding this comment.
Thanks for the examples. As long as we can specify these dependencies clearly (or if they get resolved automatically, even better), this should work for us. The split is more of an optimization of capacity use, not a hard requirement.
There was a problem hiding this comment.
Another option to consider for cheap/fast CPU unit tests is therock_cmake_subproject_build_test which runs during the build itself, on CPU runners. That runs in parallel to other build jobs and does not need to wait until all other subproject builds run, the GPU sanity check job is scheduled, etc.
There was a problem hiding this comment.
Yeah that may be a better fit for rocgdb-cpu tests instead of building so many branches into the existing GPU testing scripts and workflows. See also #4789
| contents: read | ||
|
|
||
| jobs: | ||
| consumer_graph_drift: |
There was a problem hiding this comment.
let's add test as if it is making sure it is not drifting
There was a problem hiding this comment.
Edge drift is caught by test_check_fails_on_edge_change in the unit tests, and by this workflow regenerating from a full configure and diffing. Let me know if you are thinking of other cases that I missed.
| # Level -> BFS walk depth over `consumers` edges. depth 0 = self only, | ||
| # depth 1 = self + direct consumers, None = unbounded (transitive closure). | ||
| # Levels 1 and 2 are the "stricter/broader" tiers on the DEFCON ladder; until a | ||
| # separate test-tier output exists they walk transitively (same depth as 3), so | ||
| # the accepted policy range [1, 5] and the engine never disagree. |
There was a problem hiding this comment.
can we do a better comment?
Maybe a
depth 0:
depth 1:
depth 2:
| stream = sys.stdout if ok else sys.stderr | ||
| for line in messages: | ||
| print(line, file=stream) | ||
| sys.exit(0 if ok else 1) |
There was a problem hiding this comment.
maybe instead of sys.exit, could we raise an Exception to indicate policy is not ok?
| else: | ||
| lines.append(" test_include: (none)") | ||
| if exclude: | ||
| lines.append(" test_exclude: " + ", ".join(sorted(exclude))) |
There was a problem hiding this comment.
could we use \t instead of spaces?
Record COMPILER_TOOLCHAIN edges in the consumer graph, rename the registry property to THEROCK_DIRECT_CONSUMERS_OF_*, regenerate the committed graph, and prune redundant policy includes. Add --build-dir to the drift checker, rename the drift workflow, move policy validation into the test suite, fail fast on a missing policy file, and simplify comments and the RFC.
| # Levels 1 and 2 walk transitively (same as 3) until a separate test-tier output | ||
| # distinguishes them. | ||
| _DEFAULT_LEVEL = 4 | ||
| _LEVEL_TO_DEPTH: dict[int, int | None] = {5: 0, 4: 1, 3: None, 2: None, 1: None} |
There was a problem hiding this comment.
As per this, levels 1 and 2 also will get same treatment as level 3 - a transitive walk.
As per RFC0013:
Level 2 = self + a named frameworks group
Level 1 = everything (projects_to_test = "*") + a full-QA / nightly workflow dispatch
There was a problem hiding this comment.
Yes, this PR is enabling only levels 3-5. So, temporarily levels 1, 2 are falling back to level 3. I'll add a TODO flag here to implement levels 1, 2 in the next iteration as improvement.
ScottTodd
left a comment
ScottTodd
left a comment
There was a problem hiding this comment.
Looks complete enough to land and iterate.
You may want to sync the PR branch and let CI run again prior to merge. There was a github actions outage yesterday that caused jobs to not trigger. The code here mostly doesn't affect CI yet though as far as I can tell so it could be safe.
| [component.hipsparse] | ||
| test_include = ["rocsparse"] |
There was a problem hiding this comment.
Want to adjust these includes as part of this PR or a follow-up?
Synced the branch, also cleaned up some reverse edges in test_policies.toml. Any remaining ones will be cleaned up in next iteration. Do I also need approval from @ROCm/rocm-product-managers to merge this PR? |
Technically yes, but admins could also merge through that requirement. The revirew request there is due to CODEOWNERS for |
I have re-triggered CI after syncing the branch. All the required checks pass. I see some non-blocking test failures, but the PR should be safe to merge. cc: @geomin12, @suchitravemuri1 @ScottTodd, will you be able to override @ROCm/rocm-product-managers approval requirement and merge the PR? |
|
Sure. If there are concerns about the RFC file, post-merge feedback is welcome. |
5 participants
Motivation
TheRock CI selects which GPU test suites to run from the set of changed projects, via a hand-maintained
TEST_SUBPROJECTSlist on eachtherock_cmake_subproject_declare(). That list is only partially populated, so a change to most subprojects selects nothing beyond itself, producing passing CI runs that never exercised the affected downstream suite (false greens).This PR makes test selection a derived property of the dependency edges the build already declares, rather than a parallel list kept in sync by hand. Selection walks the reverse-dependency (consumer) graph to a per-component level on a 1–5 gating ladder — default level 4 (the changed component + its direct consumers), with broader/narrower tiers available per component. See
RFC0013 for the design and gating background.
Issue ID: #3343
Technical Details
therock_cmake_subproject_declare()records a reverse edge perBUILD_DEPS/RUNTIME_DEPS;therock_emit_consumer_graph()serializes it at configure time. The graph is committed totest_tools/therock_consumer_graph.jsonso the per-PR change-detection job reads it directly — no fetch or configure on the hot path. Removes theTEST_SUBPROJECTSkeyword and its per-component uses.determine_rocm_test_dependencies.pyloads the committed graph and, for each changed subproject, walks consumer edges to the depth set by its policy level (5 = self only, 4 = direct consumers, 3 = transitive closure). Build stage is not consulted.test_tools/test_policies.toml). A small, hand-reviewed file layers explicit intent on the graph: per-componentlevel, plustest_include(additive) andtest_exclude(subtractive, applied last) for couplings the graph can't express, test-only targets with no link edge (e.g.rocgdb-cpu,hipinfo) and reverse edges absent from forward deps. Broad-blast-radius runtimes (e.g.rocr-runtime) uselevel = 3to auto-derive their downstream set instead of listing it by hand.consumer_graph_drift.ymlregenerates the graph from a full-tree configure and fails if the committed copy is stale, so the checked-in graph can never silently diverge from the declared dependency edges.Test Plan
test_tools/tests/determine_rocm_test_dependencies_test.pycovers the level walk (each depth),test_include/test_exclude, and policy validation against the committed graph.build_tools/github_actions/tests/check_consumer_graph_drift_test.pycovers the drift check's normalization and pass/fail behavior.Test Result
Submission Checklist