Repository navigation
ci(hipblaslt): reland rocjitsu race detection - #9969
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. |
1db90cd to
d9f91c4
Compare
d9f91c4 to
2f3c4f4
Compare
tony-davis
left a comment
There was a problem hiding this comment.
Thanks for the careful reland. The selection-logic fixes here are solid: the tensilelite token plus the explicit matrix boolean is the right way to stop the old contains('hipblaslt') misfire onto hipblasltprovider, and the new unit tests in therock_matrix_test.py and therock_configure_ci_test.py actually cover that behavior. The rocm-systems pin and the continue-on-error burn-in framing are good instincts too.
My blocking concern is where this runs, not what it does.
Adding gfx1151 to per-PR presubmit is a big compute increase for the whole repo. The change to therock-ci.yml flips the default Linux families to gfx94X, gfx950, gfx1151, and that runs on pull_request for every PR. Because the matrix is project-rows by target-bundles, this adds a full gfx1151 Linux build to every project row on every PR, not just hipBLASLt. In this PR's own run that is 4 new gfx1151 Linux builds (rocrand/hipblaslt row ~26m, rocprim/hipcub/rocthrust ~15m, hipkernelprovider ~10m, hipfft/rocfft ~11m), and 3 of those 4 exist only to feed a hipBLASLt-only sidecar. Given our PR volume this is a large amount of extra compute, and right now it is disclosed as a one-line follow-up rather than called out as a default-behavior change.
The placement is also backwards for a burn-in check. The sidecar is advisory (continue-on-error: true, TODO(AIHPBLAS-3605) to remove after it is reliably green), which is exactly the kind of thing that belongs on nightly first, not on every contributor's PR. As written it is the opposite: it runs on every presubmit, and nightly (therock-ci-nightly.yml) never runs it at all (nightly does not pass run_rocjitsu_race_check, and its Linux families stay gfx94X, gfx950). So we pay for it everywhere except the one place where noise would be cheap, and we get zero nightly coverage of the new check.
Recommendation: target the MultiArch Nightly, not the older single-arch CI. We are moving to MultiArch CI, so new instrumentation like this should land in therock-multi-arch-ci-nightly.yml rather than editing the older therock-ci.yml / therock-ci-linux.yml path. That keeps the experimental check on nightly during burn-in, avoids adding a new runner class to per-PR CI, and puts it on the lane we are actually investing in.
If we do end up touching nightly (or the multi-arch nightly), this PR needs evidence that nightly still runs correctly. Please include a link to a manual workflow run of the affected nightly showing it green, since presubmit CI on this PR does not exercise the nightly path.
To be clear on the smaller stuff, none of that is blocking: the inline TensileLite YAMLs and the GEMM-space expansion already have TODOs, and the second-caller and test_runs_on gating checked out as non-breaking for nightly. The one thing I would want to gate merge on is moving this off per-PR single-arch CI and onto the MultiArch Nightly, with a green nightly run linked here.
BrianHarrisonAMD
left a comment
There was a problem hiding this comment.
LGTM with a minor testing nit.
I can look into either
I don't agree with this comment, I think 'burn-in' and 'runs on every PR vs nightly' are orthogonal. Is your concern the overhead (time) of running this test or the possibility that it might fail and block PRs and hold up landing? In general I have a strong aversion to any testing that does not run on every PR that has the potential to make a test fail, and think nightly testing should only be used when tests are too slow and cannot be made faster.
I don't follow this. TL;DR I like the idea of MutliArch, because that sounds aligned with running this on different architectures. I don't like the idea of it being nightly as opposed to on every PR. I'll do some investigating though. |
|
I am currently working on dropping gfx1151 from this PR altogether (will update PR when ready). That'll eliminate the extra build time being observed. I think this is reasonable: currently there is no gfx1151 testing on PRs for hipblaslt/tensilelite anyway (i.e. no numerical testing or other unit tests) on linux. So forcing through race detection testing for these architectures on linux seems premature. When additional archs are added for 'normal' testing, I can consider adding race detection to accompany. |
2f3c4f4 to
ed4b164
Compare
|
Thanks @newling , that works for me. If we're not adding a full gfx1151 build to every PR, I'm happy with this check running on all hipBLASLt PRs. The job is already gated to hipBLASLt-selected rows (the matrix boolean plus the You're also right that burn-in and per-PR-vs-nightly are orthogonal. My real concern was the per-PR compute from the new gfx1151 build class, not the check running on presubmit. Since it's Dropping gfx1151 for now also seems like the right call given there's no other gfx1151 testing on Linux for hipBLASLt/TensileLite yet. Adding race detection alongside normal testing when more archs come online makes sense. I'll re-review once you push the update, and I'll clear the changes-requested then. |
|
@tony-davis thanks for the review above. I've dropped the gfx1151 part of this, so the additional time in CI should be minimal. My understanding is (and I'm going to confirm this) that the new sidecar job runs in parallel to other jobs. As it is fast (the tests run in seconds, their is a build of a relatively small component) it won't add to the total time a PR takes to get through CI (i.e. it won't affect the maximum time over all jobs). If you still have a preference for making this nightly though, happy to discuss. |
|
Thanks Tony. Right call to drop gfx1151 for now. I double checked that this new approach (without gfx1151) should add no extra time to a PR getting through CI, an agent confirmed the complete job added in this PR runs in parallel with other normal package tests. And as the complete time for the sidecar is small ( < 4 minutes to run tests, < 1 minute to build the component need ) and does not use the limiting resource (gpu machines) it should not have any effect on total time for a PR. Commit removing gfx1151: ed4b164 An agent reports that the current run failed for unrelated reasons:
I'll relaunch the failed tests later when I think this has been resolved |
geomin12
left a comment
There was a problem hiding this comment.
left a comment. this might require re-organization or a new way to not block rocjitsu race checks
|
As a test that I'm going to look into a MI455-style separation still, as it may be useful as a future architectural improvement. I'll also address your other comments @geomin12 and ping when ready for rereview. Thanks for checking this out! |
18098b7 to
6e544a8
Compare
|
@geomin12 could you take another look at this please? No rush, I don't want to land this this week. |
6e544a8 to
fd91d7f
Compare
fd91d7f to
84c46f7
Compare
tony-davis
left a comment
There was a problem hiding this comment.
Thanks for the turnaround. Dropping gfx1151 addresses my blocking concern, so I'm clearing changes-requested.
I checked this is the same code that already ran green: all eight files are byte-identical between the current head 84c46f7 and fd91d7f, so the Aug 3 push was a pure rebase, and at fd91d7f the sidecar passed on both gfx94X and gfx950. I also ran the new unit tests and mutated the source to confirm they bite (removing the tensilelite guard fails test_retrieve_projects_runs_ci_for_workflow_paths). Nice tests.
The current run is red, but it's all "Executing the custom container implementation failed" on the self-hosted runners and develop is green, so I don't think any of it is yours. The side effect is that the hipBLASLt builds died and the race check skipped everywhere, so please re-run before merging so we see the sidecar green at the SHA we land.
Two non-blocking asks. Could you file a ticket for the HPL-init device-fill race that --initialization zero sidesteps? My worry is that "needs independent investigation" in a shell comment quietly becomes permanent. And I'd add default: false to the run_rocjitsu_race_check input so presubmit-only is stated rather than implied.
The rocjitsu race-check sidecar is gated with contains(inputs.projects_to_test, 'hipblaslt'). Because projects_to_test is a comma-joined string of test tokens and contains() is a substring match, the gate also matches rows whose token set includes a name that merely contains the 'hipblaslt' substring, such as hipDNN's 'hipblasltprovider' component. As a result the sidecar runs on hipDNN pull requests, which are not intended targets for this check. Match the 'tensilelite' token instead. It is unique to the hipBLASLt library row and is retained when the hipBLASLt-provider row merges into it, so the sidecar continues to run for hipBLASLt library changes while no longer running for hipDNN rows.
Carry an explicit matrix boolean from changed-subtree detection so rocBLAS and other BLAS-only changes do not launch the hipBLASLt race check. Retain the tensilelite token check to select the correct merged test row.
Pin rocm-systems so every target uses the same tested rocjitsu revision. Keep failures non-blocking while the new sidecar is evaluated.
Add the fetched compiler artifact host-runtime directory to LD_LIBRARY_PATH so tensilelite-client can load libomp.so.
84c46f7 to
fb9319c
Compare
| fi | ||
|
|
||
| # The fetched compiler artifact installs the host OpenMP runtime here. | ||
| LLVM_HOST_RUNTIME_DIR="${ROCM_PATH}/lib/llvm/lib/x86_64-unknown-linux-gnu" |
There was a problem hiding this comment.
This is not always true. In the near future it will be moved to lib/llvm/lib/x86_64-unknown-linux-gnu, but currently it is at lib/llvm/lib. The LLVM_ENABLE_PER_TARGET_RUNTIME_DIR patch in TheRock was reverted due to ASAN build failures.
…0522) I've been monitoring the rocjitsu race-detection sidecar that landed in [PR #9969](#9969). The first sidecars to run after the merge were triggered on [PR #9631](#9631), and both failed before reaching either detector workload: ```text OpenMP runtime not found: .../lib/llvm/lib/x86_64-unknown-linux-gnu/libomp.so ``` This looked familiar because a missing `libomp.so` was one of the reasons the original sidecar was temporarily reverted in [PR #9938](#9938). The original failure and the current failure have the same underlying dependency, but the compiler artifact layout changed: - PR #9969's green CI proof used a compiler artifact with `libomp.so` under: ```text lib/llvm/lib/x86_64-unknown-linux-gnu/libomp.so ``` - Current `develop` uses a newer TheRock revision that includes [TheRock PR #6802](ROCm/TheRock#6802), which reverted host per-target runtime directories. Its compiler artifact places the runtime at: ```text lib/llvm/lib/libomp.so ``` [TheRock PR #7082](ROCm/TheRock#7082) is now re-enabling `LLVM_ENABLE_PER_TARGET_RUNTIME_DIR`. Changing the hard-coded path to the current flat location would therefore regress when the nested layout returns. This PR instead supports both expected layouts explicitly. The sidecar script added the historical host-triple directory to `LD_LIBRARY_PATH`, but it also required `libomp.so` to exist at that exact path. With the current artifact, the parent LLVM library directory is already in `LD_LIBRARY_PATH`, but the hard-coded preflight check exits before the workload can run. This PR replaces that exact-path assumption with a deterministic ordered lookup: 1. `lib/llvm/lib/libomp.so` 2. `lib/llvm/lib/<machine>-unknown-linux-gnu/libomp.so` If neither expected path contains `libomp.so`, the script still fails early and prints both attempted locations. ## Validation Focused validation invoked the real script against temporary ROCm roots while stubbing the later compiler invocation. The observed results were: ```text flat: LIBOMP_PATH=.../lib/llvm/lib/libomp.so lib/llvm/lib appears once in LD_LIBRARY_PATH nested: LIBOMP_PATH=.../lib/llvm/lib/x86_64-unknown-linux-gnu/libomp.so both the root and host-triple directories appear in LD_LIBRARY_PATH missing: OpenMP runtime not found at either expected path: .../lib/llvm/lib/libomp.so .../lib/llvm/lib/x86_64-unknown-linux-gnu/libomp.so layout validation passed: flat, nested, missing ``` ## Issue Tracking Related: AIHPBLAS-3605
I've been monitoring the new rocjitsu race-detection CI job that landed in [PR #9969](#9969). While doing that, I noticed that the sidecar was triggered on [PR #9631](#9631), even though that PR does not change hipBLASLt or TensileLite. This happened because changes to shared TheRock CI files expand the normal build/test matrix to include every project. The race-check selection logic then saw `projects/hipblaslt` in that expanded list and treated it as though hipBLASLt had been selected directly by the PR. This PR separates those two concepts. Shared CI changes can still expand the ordinary build and test matrix without implicitly enabling the hipBLASLt race-detection sidecar. In brief, the sidecar is now expected to be triggered in exactly these cases: 1. The PR directly changes a file under `projects/hipblaslt/**`. 2. The PR has the `test:hipblaslt` label. 3. The PR changes the rocjitsu race-check workflow or its driver script. The sidecar remains presubmit-only, so pushes, nightlies, and manual all-project dispatches do not enable it. Tests cover the behavior observed on PR #9631, direct hipBLASLt changes, explicit label selection, sidecar-owned files, and non-PR events. An agent looking at all runs of the sidecar job over the last few days estimates that this PR will reduce the number of times the sidecar runs by ~20%. Not a massive saving on average. ## Issue Tracking Related: AIHPBLAS-3605
Summary
Relands the rocjitsu hipBLASLt race checks from #9363 after the temporary revert in #9938.
The original motivation, design, workload details, limitations, and follow-up work remain documented in #9363. This reland initially covers the existing Linux
gfx94X/gfx942andgfx950target builds.The reland adds the following safeguards and scope adjustments:
tensileliterow marker and an explicit changed-path marker so the sidecar runs only whenprojects/hipblaslt/**selected the final merged product row.gfx94X/gfx942andgfx950.bded72b, ensuring both targets use the same tested rocjitsu revision and exact target-specific KMD configs.continue-on-error: trueso rocjitsu failures remain visible without failing the required CI summary during burn-in.LD_LIBRARY_PATH, allowingtensilelite-clientto loadlibomp.so.Commit structure
The commits are organized for review:
tensileliterow-selection fix.gfx94X/gfx942andgfx950, simplify the pinned rocjitsu integration, and add explicit provider-row regression coverage.The first commit is a mechanical restoration; the review-specific changes are isolated in commits 2–7.
JIRA ID: AIHPBLAS-3605