Repository navigation
feat(ci): derive test-subproject selection from the consumer graph #6941
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Changes from 8 commits
dd17fa4
dfbc289
92160ae
8f468a4
2dd805f
a3f3e4e
97044aa
727e442
a1d3653
e61b89d
4c07efe
5c00ba4
4617f1a
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change | ||||||||||||||||||||||
|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
| @@ -0,0 +1,65 @@ | ||||||||||||||||||||||||
| # Copyright Advanced Micro Devices, Inc. | ||||||||||||||||||||||||
| # SPDX-License-Identifier: MIT | ||||||||||||||||||||||||
|
|
||||||||||||||||||||||||
| # Drift check for the committed consumer graph | ||||||||||||||||||||||||
| # (test_tools/therock_consumer_graph.json). | ||||||||||||||||||||||||
| # | ||||||||||||||||||||||||
| # The graph is emitted at CMake configure time by therock_emit_consumer_graph() | ||||||||||||||||||||||||
| # and committed so the per-PR change-detection job can read it without a fetch or | ||||||||||||||||||||||||
| # configure (see .github/workflows/test_artifacts.yml). This low-frequency job | ||||||||||||||||||||||||
| # regenerates the graph from a full-tree configure, normalizes it the SAME way as | ||||||||||||||||||||||||
| # the committed copy, and FAILS if they differ — prompting the contributor to | ||||||||||||||||||||||||
| # regenerate and re-commit. This confines the ~6-minute configure cost to a | ||||||||||||||||||||||||
| # graph-affecting change instead of every PR (see RFC0013 "CI integration"). | ||||||||||||||||||||||||
| # | ||||||||||||||||||||||||
| # Runs only when files that can change the graph are touched. | ||||||||||||||||||||||||
|
|
||||||||||||||||||||||||
| name: Consumer Graph Drift Check | ||||||||||||||||||||||||
|
|
||||||||||||||||||||||||
| on: | ||||||||||||||||||||||||
| pull_request: | ||||||||||||||||||||||||
| paths: | ||||||||||||||||||||||||
| - "**/CMakeLists.txt" | ||||||||||||||||||||||||
| - "cmake/therock_emit_consumer_graph.cmake" | ||||||||||||||||||||||||
| - "cmake/therock_subproject.cmake" | ||||||||||||||||||||||||
| - "test_tools/therock_consumer_graph.json" | ||||||||||||||||||||||||
| - ".github/workflows/consumer_graph_drift.yml" | ||||||||||||||||||||||||
|
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I'm not thrilled about introducing a new workflow with its own triggering for this check given how many workflows we already have in https://github.com/ROCm/TheRock/tree/main/.github/workflows. I'd prefer to extend an existing workflow instead, but there aren't many great candidates for a job that only sometimes needs to run, needs to fetch all sources, needs to configure CMake but not build, and wants to report a quick status. Existing entry point options would be:
So... I guess this is fine, but let's keep an eye on it. |
||||||||||||||||||||||||
|
|
||||||||||||||||||||||||
| permissions: | ||||||||||||||||||||||||
| contents: read | ||||||||||||||||||||||||
|
|
||||||||||||||||||||||||
| jobs: | ||||||||||||||||||||||||
| consumer_graph_drift: | ||||||||||||||||||||||||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. let's add test as if it is making sure it is not drifting
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 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. |
||||||||||||||||||||||||
| name: "Consumer graph drift check" | ||||||||||||||||||||||||
| runs-on: ubuntu-24.04 | ||||||||||||||||||||||||
| steps: | ||||||||||||||||||||||||
| - name: Checking out repository | ||||||||||||||||||||||||
| uses: actions/checkout@3d3c42e5aac5ba805825da76410c181273ba90b1 # v7.0.1 | ||||||||||||||||||||||||
|
|
||||||||||||||||||||||||
| - name: Setting up Python | ||||||||||||||||||||||||
| uses: actions/setup-python@5fda3b95a4ea91299a34e894583c3862153e4b97 # v7.0.0 | ||||||||||||||||||||||||
| with: | ||||||||||||||||||||||||
| python-version: 3.12 | ||||||||||||||||||||||||
|
|
||||||||||||||||||||||||
| - name: Install Python deps | ||||||||||||||||||||||||
| run: pip install -r requirements.txt | ||||||||||||||||||||||||
|
|
||||||||||||||||||||||||
| - name: Fetch sources | ||||||||||||||||||||||||
| # The consumer graph is populated by every therock_cmake_subproject_declare() | ||||||||||||||||||||||||
| # call, so all subproject source trees must be present for a complete graph. | ||||||||||||||||||||||||
| run: python3 ./build_tools/fetch_sources.py --jobs 12 --depth 1 | ||||||||||||||||||||||||
|
|
||||||||||||||||||||||||
| - name: Configure (emits the consumer graph) | ||||||||||||||||||||||||
| # Configure only, no build. This emits build/therock_consumer_graph.json as a | ||||||||||||||||||||||||
| # configure-time side effect. THEROCK_ENABLE_ALL=ON ensures every subproject | ||||||||||||||||||||||||
| # is declared so the emitted graph matches the committed full-tree copy. | ||||||||||||||||||||||||
| run: | | ||||||||||||||||||||||||
| cmake -B build -S . -GNinja \ | ||||||||||||||||||||||||
| -DTHEROCK_ENABLE_ALL=ON \ | ||||||||||||||||||||||||
| -DTHEROCK_AMDGPU_FAMILIES=gfx110X-all | ||||||||||||||||||||||||
|
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. If any projects are enabled/disabled based on the GPU targets/families then this configure will not model those dependencies. At least for gfx110X, there are exclusions for TheRock/cmake/therock_amdgpu_targets.cmake Lines 193 to 198 in 820c35c
How about switching to TheRock/cmake/therock_amdgpu_targets.cmake Lines 92 to 96 in 820c35c
Unfortunately the only targets that include support for all subprojects are Linux-only, so Windows developers (like meeee) may find it hard to work with this so long as it requires actually running cmake configure to completion. Maybe we could add a simulation mode that does a faster walk through the dep graph without actually needing the projects sources to exist or to be buildable on the current platform. |
||||||||||||||||||||||||
|
|
||||||||||||||||||||||||
| - 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 | ||||||||||||||||||||||||
|
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. No need for verbose comments like "Logic lives in a unit-tested script, not inline here.", the code already expresses that. |
||||||||||||||||||||||||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -37,6 +37,14 @@ jobs: | |
| - name: Install python deps | ||
| run: pip install -r requirements-test.txt | ||
|
|
||
| # Referential-integrity check for test_tools/test_policies.toml against the | ||
| # committed consumer graph. This runs on EVERY PR (unlike the path-gated | ||
| # consumer_graph_drift.yml) because a stale policy key silently disables test | ||
| # selection for that component. It needs only the two committed files — no | ||
| # source fetch, no cmake configure. See RFC0013 "CI integration". | ||
| - name: Validate test policies against consumer graph | ||
| run: python test_tools/determine_rocm_test_dependencies.py --validate-policies | ||
|
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I don't want special cases in this file. Running all project unit tests should be as simple as possible, with a common test runner. Let's have https://github.com/ROCm/TheRock/blob/main/test_tools/tests/determine_rocm_test_dependencies_test.py check what this
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Sure, will remove it from unit_tests.yml, add it to determine_rocm_test_dependencies_test.py |
||
|
|
||
| # Note: build_tools/github_actions/ contains complicated tests in some | ||
| # subdirectories that do not support pytest: | ||
| # build_tools/github_actions/benchmarks/scripts/ | ||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,124 @@ | ||
| #!/usr/bin/env python3 | ||
| # Copyright Advanced Micro Devices, Inc. | ||
| # SPDX-License-Identifier: MIT | ||
| """Drift check for the committed consumer graph. | ||
|
|
||
| The consumer graph is emitted at CMake configure time by | ||
| therock_emit_consumer_graph() and committed to | ||
| test_tools/therock_consumer_graph.json so the per-PR change-detection job can | ||
| read it without a fetch or configure. This script keeps that committed copy | ||
| honest: `--check` regenerates nothing itself but compares the freshly emitted | ||
| graph (written to build/ by a configure) against the committed copy after | ||
| normalizing BOTH the same way, and fails on any real edge change. `--write` | ||
| copies the emitted graph over the committed copy in normalized form, so the | ||
| fix for a drift failure is a single command rather than a re-implemented | ||
| normalize one-liner. | ||
|
|
||
| Normalization (sorted keys, sorted consumer lists, 2-space indent, trailing | ||
| newline) makes the check insensitive to the emitter's registration-order output | ||
| so it only fails on a genuine dependency-edge change. | ||
| """ | ||
|
|
||
| import argparse | ||
| import difflib | ||
| import json | ||
| import sys | ||
| from pathlib import Path | ||
|
|
||
| _REPO_ROOT = Path(__file__).resolve().parents[2] | ||
| _COMMITTED_PATH = _REPO_ROOT / "test_tools" / "therock_consumer_graph.json" | ||
| _EMITTED_PATH = _REPO_ROOT / "build" / "therock_consumer_graph.json" | ||
|
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. There is no hard requirement that local builds use this path or that there is only one build directory, so let's make this build directory configurable with a
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Ack |
||
|
|
||
|
|
||
| def normalize(path: Path) -> str: | ||
| """Return the graph at `path` as canonical JSON text. | ||
|
|
||
| Sorted keys, sorted consumer lists, 2-space indent, trailing newline. This | ||
| is the single source of truth for the committed graph's on-disk form: both | ||
| the drift comparison and `--write` use it, so the committed copy and the | ||
| check can never disagree on formatting. | ||
| """ | ||
| graph = json.loads(path.read_text()) | ||
| norm = { | ||
| key: {"consumers": sorted(graph[key].get("consumers", []))} | ||
| for key in sorted(graph) | ||
| } | ||
| return json.dumps(norm, indent=2, sort_keys=True) + "\n" | ||
|
|
||
|
|
||
| def check(committed_path: Path, emitted_path: Path) -> int: | ||
| """Fail (return 1) if the committed graph differs from the emitted one.""" | ||
| committed = normalize(committed_path) | ||
| emitted = normalize(emitted_path) | ||
|
|
||
| if committed == emitted: | ||
| print("Consumer graph is up to date.") | ||
| return 0 | ||
|
|
||
| print("::error::test_tools/therock_consumer_graph.json is out of date.") | ||
| print( | ||
| "A dependency edge changed but the committed consumer graph was not " | ||
| "regenerated." | ||
| ) | ||
| print("Fix: run a full-tree configure, then regenerate and re-commit:") | ||
| print(" python3 ./build_tools/fetch_sources.py") | ||
| print( | ||
| " cmake -B build -GNinja -DTHEROCK_ENABLE_ALL=ON " | ||
| "-DTHEROCK_AMDGPU_FAMILIES=gfx110X-all" | ||
| ) | ||
|
Comment on lines
+54
to
+57
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Also update the recommended family to gfx94X-dcgpu here if addressing my related comment |
||
| print( | ||
| " python3 ./build_tools/github_actions/check_consumer_graph_drift.py --write" | ||
| ) | ||
| print(" git add test_tools/therock_consumer_graph.json") | ||
| sys.stdout.writelines( | ||
| difflib.unified_diff( | ||
| committed.splitlines(keepends=True), | ||
| emitted.splitlines(keepends=True), | ||
| fromfile="committed", | ||
| tofile="regenerated", | ||
| ) | ||
| ) | ||
| return 1 | ||
|
|
||
|
|
||
| def write(committed_path: Path, emitted_path: Path) -> int: | ||
| """Overwrite the committed graph with the normalized emitted graph.""" | ||
| committed_path.write_text(normalize(emitted_path)) | ||
| print(f"Wrote normalized consumer graph to {committed_path}.") | ||
| return 0 | ||
|
|
||
|
|
||
| def main(argv: list[str]) -> int: | ||
| parser = argparse.ArgumentParser(description=__doc__) | ||
| mode = parser.add_mutually_exclusive_group(required=True) | ||
| mode.add_argument( | ||
| "--check", | ||
| action="store_true", | ||
| help="Fail if the committed graph differs from build/'s emitted graph.", | ||
| ) | ||
| mode.add_argument( | ||
| "--write", | ||
| action="store_true", | ||
| help="Normalize build/'s emitted graph over the committed copy.", | ||
| ) | ||
| parser.add_argument( | ||
| "--committed", | ||
| type=Path, | ||
| default=_COMMITTED_PATH, | ||
| help="Path to the committed consumer graph (default: test_tools/).", | ||
| ) | ||
| parser.add_argument( | ||
| "--emitted", | ||
| type=Path, | ||
| default=_EMITTED_PATH, | ||
| help="Path to the freshly emitted consumer graph (default: build/).", | ||
| ) | ||
| args = parser.parse_args(argv) | ||
|
|
||
| if args.write: | ||
| return write(args.committed, args.emitted) | ||
| return check(args.committed, args.emitted) | ||
|
|
||
|
|
||
| if __name__ == "__main__": | ||
| sys.exit(main(sys.argv[1:])) | ||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,74 @@ | ||
| #!/usr/bin/env python3 | ||
| # Copyright Advanced Micro Devices, Inc. | ||
| # SPDX-License-Identifier: MIT | ||
| """Unit tests for check_consumer_graph_drift.py""" | ||
|
|
||
| import json | ||
| import sys | ||
| import tempfile | ||
| import unittest | ||
| from pathlib import Path | ||
|
|
||
| sys.path.insert(0, str(Path(__file__).parent.parent)) | ||
|
|
||
| from check_consumer_graph_drift import check, normalize, write | ||
|
|
||
|
|
||
| class TestNormalize(unittest.TestCase): | ||
| def _write(self, obj) -> Path: | ||
| path = Path(tempfile.mkdtemp()) / "graph.json" | ||
| path.write_text(json.dumps(obj)) | ||
| return path | ||
|
|
||
| def test_normalize_is_order_insensitive(self) -> None: | ||
| # Same edges, different key/consumer order -> identical normalized text. | ||
| a = self._write({"b": {"consumers": ["z", "a"]}, "a": {"consumers": []}}) | ||
| b = self._write({"a": {"consumers": []}, "b": {"consumers": ["a", "z"]}}) | ||
| self.assertEqual(normalize(a), normalize(b)) | ||
|
|
||
| def test_normalize_has_trailing_newline_and_indent(self) -> None: | ||
| text = normalize(self._write({"a": {"consumers": ["b"]}})) | ||
| self.assertTrue(text.endswith("\n")) | ||
| self.assertIn(' "a"', text) # 2-space indent | ||
|
|
||
| def test_normalize_tolerates_missing_consumers_key(self) -> None: | ||
| # A node with no "consumers" key normalizes to an empty list, not a crash. | ||
| text = normalize(self._write({"a": {}})) | ||
| self.assertIn('"consumers": []', text) | ||
|
|
||
|
|
||
| class TestCheck(unittest.TestCase): | ||
| def _write(self, obj) -> Path: | ||
| path = Path(tempfile.mkdtemp()) / "graph.json" | ||
| path.write_text(json.dumps(obj)) | ||
| return path | ||
|
|
||
| def test_check_passes_when_equal_modulo_order(self) -> None: | ||
| committed = self._write({"a": {"consumers": ["b", "c"]}}) | ||
| emitted = self._write({"a": {"consumers": ["c", "b"]}}) | ||
| self.assertEqual(check(committed, emitted), 0) | ||
|
|
||
| def test_check_fails_on_edge_change(self) -> None: | ||
| committed = self._write({"a": {"consumers": ["b"]}}) | ||
| emitted = self._write({"a": {"consumers": ["b", "c"]}}) | ||
| self.assertEqual(check(committed, emitted), 1) | ||
|
|
||
|
|
||
| class TestWrite(unittest.TestCase): | ||
| def test_write_makes_check_pass(self) -> None: | ||
| root = Path(tempfile.mkdtemp()) | ||
| committed = root / "committed.json" | ||
| emitted = root / "emitted.json" | ||
| # Stale committed copy, differently-ordered emitted graph. | ||
| committed.write_text(json.dumps({"a": {"consumers": ["stale"]}})) | ||
| emitted.write_text(json.dumps({"a": {"consumers": ["c", "b"]}})) | ||
|
|
||
| self.assertEqual(write(committed, emitted), 0) | ||
| # After write, committed matches emitted and check passes. | ||
| self.assertEqual(check(committed, emitted), 0) | ||
| # And it was written in normalized form (sorted, trailing newline). | ||
| self.assertEqual(committed.read_text(), normalize(emitted)) | ||
|
|
||
|
|
||
| if __name__ == "__main__": | ||
| unittest.main() |
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,75 @@ | ||
| # Copyright Advanced Micro Devices, Inc. | ||
| # SPDX-License-Identifier: MIT | ||
|
|
||
| # therock_emit_consumer_graph(output_file) | ||
| # | ||
| # Serializes the THEROCK_ALL_SUBPROJECTS / THEROCK_CONSUMERS_OF_* global | ||
| # property registry (populated by therock_cmake_subproject_declare) to a | ||
| # JSON file at <output_file>. | ||
| # | ||
| # Must be called AFTER all therock_cmake_subproject_declare() calls have run | ||
| # (i.e. at the end of the top-level CMakeLists.txt). The output is a | ||
| # configure-time side-effect (not a build target), analogous to | ||
| # compile_commands.json. | ||
| # | ||
| # JSON schema: | ||
| # { | ||
| # "<subproject-lowercase>": { | ||
| # "consumers": ["<consumer-lowercase>", ...] | ||
| # }, | ||
| # ... | ||
| # } | ||
| # | ||
| # The graph carries only consumer (reverse-dependency) edges. No build-stage, | ||
| # artifact, or other facts are duplicated here — each keeps its own single source | ||
| # of truth. Test selection walks these consumer edges by hop distance | ||
| # (see test_tools/determine_rocm_test_dependencies.py). | ||
| # | ||
| # Lifecycle: this configure-time emit writes build/therock_consumer_graph.json, | ||
| # but that copy is a GENERATED artifact used to refresh the COMMITTED graph at | ||
| # test_tools/therock_consumer_graph.json. The committed copy is what the per-PR | ||
| # change-detection job reads directly — no fetch, no configure on the hot path. | ||
| # A low-frequency drift-check job (.github/workflows/consumer_graph_drift.yml) | ||
| # regenerates the graph from a configure, normalizes it, and fails if it differs | ||
| # from the committed copy, prompting the contributor to regenerate and re-commit. | ||
| # The committed graph is generated-only and must NEVER be hand-edited. | ||
| function(therock_emit_consumer_graph output_file) | ||
| get_property(_all GLOBAL PROPERTY THEROCK_ALL_SUBPROJECTS) | ||
|
|
||
| set(_json "{\n") | ||
| set(_first_proj TRUE) | ||
|
|
||
| foreach(_proj IN LISTS _all) | ||
| get_property(_consumers GLOBAL PROPERTY "THEROCK_CONSUMERS_OF_${_proj}") | ||
| if(_consumers) | ||
| list(REMOVE_DUPLICATES _consumers) | ||
| endif() | ||
|
|
||
| if(NOT _first_proj) | ||
| string(APPEND _json ",\n") | ||
| endif() | ||
| set(_first_proj FALSE) | ||
|
|
||
| string(TOLOWER "${_proj}" _key) | ||
|
|
||
| # Build JSON array of consumers. | ||
| set(_arr "") | ||
| set(_first_c TRUE) | ||
| foreach(_c IN LISTS _consumers) | ||
| string(TOLOWER "${_c}" _cv) | ||
| if(NOT _first_c) | ||
| string(APPEND _arr ", ") | ||
| endif() | ||
| set(_first_c FALSE) | ||
| string(APPEND _arr "\"${_cv}\"") | ||
| endforeach() | ||
|
|
||
| string(APPEND _json " \"${_key}\": {\n") | ||
| string(APPEND _json " \"consumers\": [${_arr}]\n") | ||
| string(APPEND _json " }") | ||
| endforeach() | ||
|
|
||
| string(APPEND _json "\n}\n") | ||
| file(WRITE "${output_file}" "${_json}") | ||
| message(STATUS "Wrote consumer graph to ${output_file}") | ||
| endfunction() |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We could at least name this file
test_consumer_graph_draft.ymlso it sorts alphabetically next to some other workflows that "test" different properties of the project in https://github.com/ROCm/TheRock/tree/main/.github/workflowsThere was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Agree, I'll rename it.