Repository navigation
(feat): [AILIKFD-40] add kfdtest to TheRock CI - #6334
Conversation
✅ All Checks Passed — Ready for Review
📖 Need help? See the Policy FAQ for details on every check and how to fix failures. |
|
🎉 All checks passed! This PR is ready for review. |
a43a8be to
2d97e92
Compare
2d97e92 to
0fc9e92
Compare
5da92dc to
b8e8a86
Compare
## Summary TheRock side of test-filter standardization for **rocSHMEM**, paired with [ROCm/rocm-systems#6334](ROCm/rocm-systems#6334). rocm-systems#6334 adds native CTest integration for rocSHMEM: `projects/rocshmem/tests/test_categories.yaml` plus `apply_ctest_category_labels()` wiring that installs relocatable `bin/rocshmem/**/CTestTestfile.cmake` files (a root dispatcher that `subdirs()` into `tests/functional` and `tests/unit`) carrying the `quick`/`standard`/`comprehensive`/`full` tier labels. This PR wires TheRock so the generic `test_runner.py` can drive those tests from the installed artifact: - **`comm-libs/CMakeLists.txt`** — pass `-DBUILD_CTESTS=${THEROCK_BUILD_TESTING}` to the rocshmem subproject. The CTest registration and the install-tree `CTestTestfile.cmake` generation in #6334 are gated behind `BUILD_CTESTS` (default `OFF`), so without this the install tree would contain no `bin/rocshmem/**` and `test_runner.py` would fail with `Test directory does not exist`. - **`comm-libs/artifact-rocshmem.toml`** — populate the previously-empty `test` component: `bin/rocshmem/**` (the install-tree CTest files), `bin/rocshmem_unit_tests`, the `share/rocshmem/*` CTest wrapper scripts, and the `device_bitcode` HSACOs. The functional binary and `rocshmem_info` already live in the `run` component (fetched via `rocshmem_run`). - **`build_tools/install_rocm_from_artifacts.py`** — add a `--rocshmem` artifact selector that pulls `rocshmem_lib`/`_run`/`_test` plus OpenMPI (`mpirun` + libs) needed to launch the tests. - **`build_tools/github_actions/fetch_test_configurations.py`** — add a `rocshmem` test job using `test_runner.py` (`fetch_artifact_args = "--rocshmem --tests"`), on Linux multi-GPU runners (the functional/unit tests launch via `mpirun` with `RANKS` 2..64, same setup as `rccl`). - **`fetch_test_configurations_test.py`** — relax the assertion that assumed exactly one multi-GPU job repo-wide, now that rocshmem is a second multi-GPU job alongside rccl. `test_runner.py` needs no `COMPONENT_DIR_MAPPING` entry (job name `rocshmem` == install dir `bin/rocshmem`). ## Test plan - [x] `python3 -m pytest build_tools/github_actions/tests/fetch_test_configurations_test.py` (22 passed) - [x] `black --check` on the edited Python files - [x] `--rocshmem` registered in `install_rocm_from_artifacts.py --help` - [x] `artifact-rocshmem.toml` parses; test `include` resolves - [x] CI: build with `THEROCK_BUILD_TESTING=ON`, confirm `bin/rocshmem/CTestTestfile.cmake` + `tests/{functional,unit}/CTestTestfile.cmake` are installed and bundled in the test artifact - [x] CI: `TEST_COMPONENT=rocshmem TEST_TYPE=quick python test_runner.py` discovers labels and runs `ctest -L quick` from `THEROCK_BIN_DIR/rocshmem` ## Test Results Test Run: https://github.com/ROCm/TheRock/actions/runs/30112055659/job/89560360151?pr=5691 Made with [Cursor](https://cursor.com) --------- Co-authored-by: Cursor <cursoragent@cursor.com>
|
Requested review from @nunnikri who originally reviewed #5377. A few points:
|
b8e8a86 to
4d3b0c7
Compare
6b12604 to
9a27f90
Compare
|
@nunnikri . kfdtest is not running in the CI, it was disabled on advised by Saad and Luis. The big problem with it is that it wants exclusive control of the GPU and tend to crash when it has to share. It is designed to be selfish in that way. So the idea is just to compile and then those who want to use it (QA, customers) can just run it. I can merge here, then do an improvement PR for runtime (b/c this issue only affect stand alone build) with the above changes? |
TheRock is giving a platform to build, test and distribute ROCm components. But we cant decide on the features each components wants. It should be discussed with BU and finalize. |
Running kfdtest in TheRock ci we didnt want at this point, may be later can add. The idea was for users/QA to have an option to run these test. https://therock-ci-artifacts.s3.amazonaws.com/30953000930-linux/packages/deb/pool/main/index.html But one thing we should add before merging the PR. Every component should have a LIcense file under which the component is distributed. Usually MIT for ROCm components. Please get that added before merging. Otherwise I am good. |
bcbcc93 to
f1ce5a0
Compare
|
Thanks, License added, hoping that it will be green. FYI ROCm/rocm-systems#9716 will take care of that other issue. |
|
@nunnikri the last build is now finish, do check if the license file you need is now present (how can I check this on my own)? |
1d387b8 to
88514ad
Compare
Pre-commit check failed⛔ pre-commit failed Please run locally:
This repo uses |
02c08b9 to
10ba5be
Compare
nunnikri
left a comment
There was a problem hiding this comment.
Looks like again the PR got messed up. All the submodules got bumped.
Need to revert all the changes in compiler, rocgdb, math-libs, third-party, rocm-libraries, rocm-systems
6cb8379 to
11d5527
Compare
|
Thanks for that, I did realize that the last rebase had those sub-module changes that was pulled into mine. I have, stashed, hard reset, pop, cherry-pick, and fixed things. |
- Add kfdtest subproject to core/CMakeLists.txt with proper build dependencies - Install LICENSE.kfdtest to share/doc/kfdtest/ in stage directory - Create kfdtest_test_generic.tar.zst artifact - Add kfdtest to amdrocm-runtime-test package (DEB and RPM) - kfdtest requires amdrocm-sysdeps and amdrocm-llvm runtime dependencies Resolves: AILIKFD-40 Co-Authored-By: Claude Sonnet 4 <noreply@anthropic.com>
11d5527 to
d77730c
Compare
|
The license file is in the pkg now, please have a look and see if you like things. |
nunnikri
left a comment
There was a problem hiding this comment.
Now the package contents looks good.
/opt/rocm/core-10.1/bin/kfdtest
/opt/rocm/core-10.1/bin/run_kfdtest.sh
/opt/rocm/core-10.1/share/doc/kfdtest
/opt/rocm/core-10.1/share/doc/kfdtest/LICENSE.kfdtest
/opt/rocm/core-10.1/share/kfdtest
/opt/rocm/core-10.1/share/kfdtest/kfdtest.exclude
|
This was merged with test failures: https://github.com/ROCm/TheRock/actions/runs/31300075067/job/93211327008 See #7298 |
## Motivation kfdtest was added as a subproject in #6334, but the committed consumer graph was never regenerated. The drift check compares the whole file, so any PR touching a graph-affecting path either fails it or has to carry these lines in an unrelated diff. GitHub issue: #7298 ## Technical Details Regenerated `test_tools/therock_consumer_graph.json` with `check_consumer_graph_drift.py --write`. No other files change. The diff is the kfdtest node plus its seven reverse edges from `core/CMakeLists.txt`: `amd-llvm`, `rocr-runtime`, `therock-libdrm`, `therock-numactl`, `therock-yaml-cpp`, `therock-zlib`, `therock-zstd`. No behavior change. Selection can now emit `kfdtest`, but there is no `kfdtest` key in `fetch_test_configurations.py`, so no job is scheduled. ## Test Plan Drift check, policy validation, selection unit tests, pre-commit. ## Test Result - `--validate-policies`: OK, 18 component keys against 116 graph nodes - `determine_rocm_test_dependencies_test.py`: 49 passed - pre-commit: passed ## Submission Checklist - [x] Look over the contributing guidelines at https://github.com/ROCm/TheRock/blob/main/CONTRIBUTING.md.
Re-launching kfdtest integration into TheRock CI.
Co-Authored-By: Claude Sonnet 4.5
Signed-off-by: Andrew Martin andrew.martin@amd.com
JIRA ID: AILIKFD-40
Co-Authored-By: Claude Sonnet 4.5
Motivation
To have a copy of the
kfdtestbinary as part of TheRock release archive.This is useful for both QA and various customers who use it to test and verify KFD.
Technical Details
Test Plan
Test Result
Submission Checklist