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. |
|
🎉 All checks passed! This PR is ready for review. |
Allow host-asan tests to run on pull_request triggers when the build is from an external repo that has narrowed the build graph via build_stages. This ensures only repos that have explicitly optimized for presubmit (e.g., rocm-systems with compiler-runtime + runtime-tests) receive sandbox runners on every PR. Satisfies issue #7202 requirement: "Host-ASan tests receive an eligible sandbox runner on the PR trigger."
8c5acd6 to
245fe69
Compare
dileepr1
left a comment
There was a problem hiding this comment.
The change looks good.
A nit:
Adding a unit test might be good to make sure that we land on the right runner.
There was a problem hiding this comment.
I'm not confident in "External repos that set build_stages (e.g., rocm-systems presubmit) have already scoped the build enough to make sandbox runners affordable on every PR." as we still haven't completed the work to enable multi-arch CI (non-asan) in external repositories. Let's stick that landing first before we add more jobs.
- We currently set
hip-clrtolevel = 3meaning test "self + all transitive consumers" here:TheRock/test_tools/test_policies.toml
Lines 21 to 26 in 36503ee
- I would like to set RCCL to level 2 (https://github.com/ROCm/TheRock/blob/main/docs/rfcs/RFC0013-Consumer-Based-Test-Selection.md), to build and test pytorch. That decision could be separate for asan and non-asan
I expect we'll want finer-grained control here, not just setting "every external repository runs asan all the time".
|
Stepping back a bit, what coverage are we expecting from running tests on asan CI? Do we need to run both asan tests and non-asan tests on PRs? That seems like duplicate coverage that will double our runner usage. Looking at the example workflow run https://github.com/ROCm/rocm-systems/actions/runs/33412744162/job/99564499807, that failed on the driver / GPU sanity check. Do ASan tests work? Can we look at some logs together? I looked around some more and I can't find useful asan build/test logs to analyze:
We shouldn't just enable workflows on presubmit without first validating them on manual workflow_dispatch, postsubmit, or nightly (in general presubmit should be a subset of postsubmit), and I'm not seeing enough evidence that these test are functional. Meta: these changes are quite difficult to review in a timely manner without seeing the plan written down and more complete evidence of testing. I'm looking for detailed analysis like what was recently done on ROCm/rocm-libraries#11439 (comment) for example. |
|
Still looking for any logs of asan tests actually running and working (this is a prerequisite for enabling any workflow on pull requests, unless there is explicit justification for why known-failing jobs are being added):
|
geomin12
left a comment
There was a problem hiding this comment.
also as Scott mentioned, let's make sure it passes on presubmit before we enable!
Will provide a detailed analysis on what is planned and the detailed test result |
…ring it Granting sandbox test runners on the strength of build_stages being set reads "this caller scoped its build" as "this caller wants ASAN runners on every PR". Those are separate decisions: an allowlist naming every stage narrows nothing, and a repo that scopes its build for unrelated reasons would silently start consuming scarce ASAN runner capacity. Add a presubmit input that callers set to declare themselves an automatic presubmit gate, and key the host-asan runner allocation off that. The external-repo guard is unchanged, so eligibility still requires both. Addresses review feedback on #7780. Co-authored-by: Cursor <cursoragent@cursor.com>
…ring it Granting sandbox test runners on the strength of build_stages being set reads "this caller scoped its build" as "this caller wants ASAN runners on every PR". Those are separate decisions: an allowlist naming every stage narrows nothing, and a repo that scopes its build for unrelated reasons would silently start consuming scarce ASAN runner capacity. Add a presubmit input that callers set to declare themselves an automatic presubmit gate, and key the host-asan runner allocation off that. The external-repo guard is unchanged, so eligibility still requires both. Addresses review feedback on #7780. Co-authored-by: Cursor <cursoragent@cursor.com>
Aligns the split PR with the name used in #7311, and avoids colliding with the existing meaning of "presubmit" in this file, where it already denotes a family trigger tier alongside postsubmit and nightly. Co-authored-by: Cursor <cursoragent@cursor.com>
Validate ROCm/TheRock#7780 on top of current main (which already carries ROCm/TheRock#7602 and ROCm/TheRock#7600) without the sanity-check fixes still in review as ROCm/TheRock#8100. Co-authored-by: Cursor <cursoragent@cursor.com>
Rebuilds the validation branch on top of current main, which already carries #8123's ASAN environment setup, so only #7780 remains on top. #8100 is left out while it is parked. The host-asan gate moved and was widened to startswith() on main since #7780 branched, so the presubmit escape is applied to that structure rather than the original elif. Content matches #7780. Co-authored-by: Cursor <cursoragent@cursor.com>
Resolves the conflict in configure_multi_arch_ci.py. Main restructured the ASAN
runner selection since this branch was cut: the host-asan case moved ahead of
the plain-asan case and widened from `build_variant == "host-asan"` to
`build_variant.startswith("host-asan")`, so it now also covers host-asan-debug.
Kept main's structure and re-applied this PR's change to it -- the presubmit
escape is added to main's `startswith()` branch rather than to the `elif` it was
originally written against. Net diff against main is unchanged: the
asan_presubmit input, its three plumbing points, and the widened gate.
This resolution was validated against ROCm/rocm-systems#10088, where the setup
job allocated linux-gfx942-1gpu-asan-sandbox-rocm on a pull request:
https://github.com/ROCm/rocm-systems/actions/runs/34608811109
Co-authored-by: Cursor <cursoragent@cursor.com>
There was a problem hiding this comment.
nit: the PR description is difficult to follow, please rephrase.
Specifically:
Allow host-asan tests to run on pull_request triggers when the build is from an external repo that has narrowed the build graph via build_stages. This ensures only repos that have explicitly optimized for presubmit (e.g., rocm-systems with compiler-runtime + runtime-tests) receive sandbox runners on every PR.
Satisfies issue #7202 requirement: "Host-ASan tests receive an eligible sandbox runner on the PR trigger."
ISSUE ID: #7202
"Receive sandbox runners on every PR" is not a first order goal, it's a prerequisite for these workflows to function. The goal is to run host-asan CI on pull_request events and that is what this does. Sandbox runners aren't directly related to the code changes here.
Please also use the supplied pull request template (https://github.com/ROCm/TheRock/blob/main/.github/pull_request_template.md) to make review easier.
| BUILD_PYTORCH: ${{ inputs.build_pytorch }} | ||
| BUILD_JAX: ${{ inputs.build_jax }} | ||
| BUILD_NATIVE_LINUX: ${{ inputs.build_native_linux }} | ||
| ASAN_PRESUBMIT: ${{ inputs.asan_presubmit }} |
There was a problem hiding this comment.
This may be okay for now, but I've given the feedback on multiple PRs at this point that we need to generalize options like this. We have multiple build variants:
- ci/release (default)
- asan sanitizer
- tsan sanitizer
- debug symbols
- code coverage
- canary flags
We shouldn't need to add individual options for each variant, there should be a general way to instantiate a CI pipeline and choose what triggers it should be enabled/disabled for.
There was a problem hiding this comment.
Thanks, will keep it in consideration going forward.
There was a problem hiding this comment.
@geomin12 @jayhawk-commits how do you both feel here? I keep going back and forth on "good enough for now" or "blocking" for this.
As soon as one option like this gets added others may follow, and each repository using it will need to adapt either way. It feels like we're slowly adding complexity over time to CI setup scripts.
Possible approaches:
- workflow inputs like:
# rocm-libraries with: build_variant: asan pull_request_mode: automatic # TheRock with: build_variant: asan pull_request_mode: opt-in pull_request_opt_in_label: "ci:asan"
- config yaml files for https://github.com/ROCm/TheRock/blob/main/build_tools/github_actions/configure_multi_arch_ci.py like:
used like this from the workflows:
schema_version: 1 variants: release: events: pull_request: mode: automatic opt_out_label: "ci:skip-release" push: {} workflow_dispatch: {} asan: events: pull_request: mode: opt-in opt_in_label: "ci:asan" schedule: {} workflow_dispatch: {} coverage: events: workflow_dispatch: {}
with: build_variant: asan ci_policy_path: .github/therock-ci.yml
There was a problem hiding this comment.
we should block. @raramakr @dileepr1 i understand this is the easiest way to land host-asan for external repos (and fastest as ASAN is important), but Scott is right! we should make this feature reusable, expandable and understandable to other scenarios too.
these random inputs become tribal knowledge, and it's hard to track as we add more and more "one-off" needed features
for example: i'm doing a ton of re-work to cleanup these "one-off" fixes that have accumulated in #8322, as all of us are losing track of which architecture runs where (is it submodule bump? postsubmit? presubmit?)
we're working on maturing our labelling system so engineers can specify everything they need: #7375, maybe we can start from there? where we automatically add ci:host-asan labels to rocm-libraries / rocm-systems PRs as a quick and clean solution?
and as a permanent solution, we can expand amdgpu_family_matrix.py (or somewhere) to make it very clear that: "on presubmit, we run host-asan" or brainstorm to make it clear for everyone (and when we add postsubmit, later features, it'll be easy to add then rather than more inputs)
Replace the rationale-heavy comments on the host-asan gate and the asan_presubmit input with a trigger table and a one-line description, per review feedback. The table keeps the external_repo qualifier so it matches the condition below it. Add gate coverage: the opt-in enables tests for an external repo, its absence leaves them unscheduled, and the opt-in alone does not enable them without an external repo. Co-authored-by: Cursor <cursoragent@cursor.com>
Co-authored-by: Cursor <cursoragent@cursor.com> # Conflicts: # build_tools/github_actions/tests/configure_multi_arch_ci_test.py
## Motivation ### In Short: #8268 fixes two independent bugs that together made every host-ASan hip-tests shard fail without running a single test. First, is_asan() matched BUILD_VARIANT exactly against "asan", so it was false for host-asan and everything gated on it silently did nothing Second, configure_asan_env.py pinned the runtime filename to libclang_rt.asan.so, which #8077 renamed to libclang_rt.asan-<arch>.so, so the ASan runtime hadn't resolved at all since that landed. ## Details GitHub issue: #7202 Found while looking at the first host-asan presubmit run that got as far as the component test matrix (ROCm/rocm-systems#10088, [run 35004915528](https://github.com/ROCm/rocm-systems/actions/runs/35004915528)). All four `hip-tests` shards failed **without running a single test**, and with zero AddressSanitizer findings: ``` catch_tests/RTC: error while loading shared libraries: libclang_rt.asan-x86_64.so: cannot open shared object file CMake Error at CatchAddTests.cmake:82: Error listing tests from executable '.../RTC': Result: 127 ctest ... returned non-zero exit status 8 ``` ctest runs `RTC --list-tests` to enumerate Catch2 tests; it cannot load the instrumented binary, CMake aborts the listing, and the shard reports failure. Two separate defects combine to produce this. ### 1. `is_asan()` excluded host-asan ```python return BUILD_VARIANT == "asan" ``` An exact match, so `host-asan` (and `asan-debug`, `host-asan-debug`) evaluated false and everything gated on it silently did nothing: | Caller | What was skipped | | --- | --- | | `test_hiptests.py` | `LD_PRELOAD` of the runtime, `HSA_XNACK`, extended timeout | | `test_rocprofiler_sdk.py` | clang resource dir on the loader path, preload, ASAN test exclusions | | `test_hipfile.py` | preload | | `test_rocblas.py`, `test_hipblas.py`, `test_hipblaslt.py` | `HSA_XNACK` | | `tests/test_rocm_sanity.py` | the #3312 rocminfo skip | The `test_hiptests.py` preload is the one that breaks discovery. Notably the machinery was already correct — `get_asan_lib_path()` there asks clang for `libclang_rt.asan-<arch>.so`, exactly the name `RTC` needs — it just never ran. The rest of the codebase already treats these as a family: `fetch_test_configurations.py` uses `"asan" in build_variant` with a comment listing all four variants, `configure_multi_arch_ci.py` uses `suffix in ("asan", "host-asan")`, and `publish_rocm_to_release_buckets.py` has `_is_asan_variant()`. `is_asan()` was the odd one out. ### 2. `configure_asan_env.py` pinned one runtime filename `ASAN_RUNTIME_LIB = "libclang_rt.asan.so"` is the per-target-runtime-dir name. #8077 (`cd8ed0c3f`) set `LLVM_ENABLE_PER_TARGET_RUNTIME_DIR=OFF`, moving the runtime to `lib/linux/libclang_rt.asan-<arch>.so`. Resolution has failed ever since, and every ASAN test job has been logging `##[warning]ASAN runtime not found`. The regression is visible across runs: | Run | TheRock base | Result | | --- | --- | --- | | [34549006044](https://github.com/ROCm/rocm-systems/actions/runs/34549006044) | `b85dc2119` (pre-#8077) | resolved `.../x86_64-unknown-linux-gnu/libclang_rt.asan.so` | | [34613837319](https://github.com/ROCm/rocm-systems/actions/runs/34613837319) | `24cc5c7d6` | not found | | [35004915528](https://github.com/ROCm/rocm-systems/actions/runs/35004915528) | `24cc5c7d6` | not found | ## Technical Details `is_asan()` now matches any ASAN-family variant via `"asan" in BUILD_VARIANT`, mirroring `fetch_test_configurations.py`. `_resolve_asan_runtime()` tries both filenames in turn, per-target first then the legacy arch-suffixed one, relying on the fact that clang echoes the requested name back unchanged when it cannot locate the file. The warning now names both candidates. TheRock has flipped between these layouts, so pinning either one is fragile. ## Test Plan - `python -m pytest build_tools/ tests/ -q --continue-on-collection-errors` (against baseline) - `pre-commit run --files build_tools/github_actions/amdgpu_family_matrix.py build_tools/github_actions/configure_asan_env.py build_tools/github_actions/tests/configure_asan_env_test.py` ## Test Result 2432 passed / 16 failed / 6 collection errors, against a baseline of 2431 passed / 16 failed / 6 collection errors on `origin/main`. The failures and collection errors are identical on both sides and are pre-existing local dependency gaps (`zlib_ng`, `pkginfo`, `_rocm_sdk_core_DEFAULT`); the delta is the one test added here. Targeted pre-commit passed. ## Risk Assessment Risk 3. Widening `is_asan()` activates ASAN handling for host-asan across several test scripts that have never had it, which is the intent but does change their behavior. The specific known side effect is that the TheRock#3312 rocminfo skip now applies to host-asan. That `skipif` is parametrized over three search patterns, so three sanity tests that were passing under host-asan are now skipped, taking the sanity tally from `1 failed, 4 passed, 3 skipped` to `1 failed, 1 passed, 6 skipped`: ``` test_rocm_output[rocminfo - GPU Device Type Search] SKIPPED test_rocm_output[rocminfo - GFX Name Search] SKIPPED test_rocm_output[rocminfo - AMD Vendor Name Search] SKIPPED ``` That coverage loss is accepted here in exchange for the variants behaving consistently. Worth a follow-up: #3312 was filed against full ASAN, and these tests demonstrably pass under host-asan, so narrowing that `skipif` to the full-ASAN variant may be the better long-term answer. The runtime-name change is additive: it only adds a second candidate after the existing one fails. ## Related - Work tracking: #7202 - Caused the rename: #8077 - Depends on nothing; independent of #7780 and #8100 ## Device / Architecture Coverage Validated on gfx942 via the rocm-systems host-asan presubmit. No device code changes. ## Testing Summary Added `test_resolves_the_legacy_arch_suffixed_runtime`, which stubs a clang that resolves only the arch-suffixed name and echoes the request back otherwise, matching real clang behavior. Existing coverage for the per-target name, missing clang, missing runtime, missing symbolizer and the GITHUB_ENV format is unchanged. ## Testing Checklist - [x] Full unit suite vs baseline - Status: Passed, no regressions - [x] Targeted pre-commit - Status: Passed - [x] host-asan presubmit against ROCm/rocm-systems#10088 - [run 35140846158](https://github.com/ROCm/rocm-systems/actions/runs/35140846158) - Status: Passed. hip-tests went from 0 tests discovered to ~4,900 run across 4 shards (~2,050 passing). Every remaining failure is the pre-existing `hsa_amd_memory_pool_free` bad-free. The runtime now resolves to `.../lib/clang/24/lib/linux/libclang_rt.asan-x86_64.so`, confirming the #8077 layout. - [ ] PR CI - Status: Pending ## Flags / Guardrails None. ## Adjacent Tests Considered `rocprofiler_sdk_dashboard_test.py` patches `is_asan` directly rather than driving it through `BUILD_VARIANT`, so it is unaffected by the widening and needed no change. ## Risk Acceptance / Waivers None. ## Technical Changes - `is_asan()` matches any ASAN-family `BUILD_VARIANT` instead of exactly `"asan"`. - `configure_asan_env.py` resolves the ASAN runtime under either the per-target or the legacy arch-suffixed filename. - Add a unit test for the legacy name. ## Submission Checklist - [x] Look over the contributing guidelines at https://github.com/ROCm/TheRock/blob/main/CONTRIBUTING.md. Co-authored-by: Cursor <cursoragent@cursor.com>
ScottTodd
left a comment
There was a problem hiding this comment.
(marking as 'reviewed' pending latest comments, I want others to weigh in before I take another look)
There was a problem hiding this comment.
comment regarding this feature. let's re-work this so it's expandable for later use-cases (using asan for postsubmit should not require another input)
as mentioned, all these little fast easy inputs makes it hard to track. will we remember this input 6 months later? (because for me, i definitely will not)
| BUILD_PYTORCH: ${{ inputs.build_pytorch }} | ||
| BUILD_JAX: ${{ inputs.build_jax }} | ||
| BUILD_NATIVE_LINUX: ${{ inputs.build_native_linux }} | ||
| ASAN_PRESUBMIT: ${{ inputs.asan_presubmit }} |
There was a problem hiding this comment.
we should block. @raramakr @dileepr1 i understand this is the easiest way to land host-asan for external repos (and fastest as ASAN is important), but Scott is right! we should make this feature reusable, expandable and understandable to other scenarios too.
these random inputs become tribal knowledge, and it's hard to track as we add more and more "one-off" needed features
for example: i'm doing a ton of re-work to cleanup these "one-off" fixes that have accumulated in #8322, as all of us are losing track of which architecture runs where (is it submodule bump? postsubmit? presubmit?)
we're working on maturing our labelling system so engineers can specify everything they need: #7375, maybe we can start from there? where we automatically add ci:host-asan labels to rocm-libraries / rocm-systems PRs as a quick and clean solution?
and as a permanent solution, we can expand amdgpu_family_matrix.py (or somewhere) to make it very clear that: "on presubmit, we run host-asan" or brainstorm to make it clear for everyone (and when we add postsubmit, later features, it'll be easy to add then rather than more inputs)
@geomin12 @ScottTodd based on the above comments, I've got an prototype in #8455 - could you check if that would be good? |
ScottTodd asked for full URLs rather than bare issue numbers. Co-authored-by: Cursor <cursoragent@cursor.com>
ScottTodd
left a comment
There was a problem hiding this comment.
Going through my review backlog... this still has the asan_presubmit input. Needs to be generalized. Please also move PRs to draft or close if they are abandoned/discarded.
|
Closing in favor of #8455 |
Motivation
Host-asan CI currently only runs on
scheduleandworkflow_dispatch. This enables it onpull_requestevents for callers that opt in, so that ASAN regressions are caught on the PR that introduces them rather than overnight.GitHub issue: #7202
Technical Details
Adds an
asan_presubmitboolean input tosetup_multi_arch.yml, plumbed through toconfigure_multi_arch_ci.pyasCIInputs.asan_presubmit.The host-asan gate now enables tests on these triggers:
scheduleworkflow_dispatchpushpull_requestasan_presubmitopt-inThe opt-in is declared explicitly by the caller rather than inferred from
build_stages, so narrowing a build graph for unrelated reasons does not start consuming ASAN runner capacity.Prerequisites, both already merged:
Test Plan
Unit tests for the gate in
configure_multi_arch_ci_test.py: the opt-in enables tests for an external repo, its absence leaves them unscheduled, and the opt-in alone does not enable them without an external repo.End-to-end validation was run from rocm-systems against a TheRock branch carrying this change, driven incrementally so each failure could be attributed to a specific cause.
Test Result
Unit tests: 953 passed, 2 skipped (950 before this change; the 3 added are the delta).
End-to-end, https://github.com/ROCm/rocm-systems/actions/runs/35140846158 confirms the pipeline reaches real ASAN results rather than failing on infrastructure:
hip_checkto completion and reports a symbolizedAddressSanitizer: bad-freeinlibamdhip64.so.7athsa_amd_memory_pool_free, with both the allocation and free stacks resolved. This is a genuine finding, not an instrumentation artifact: ASAN's interceptors are installed and it tracked the 4 MB region across its lifetime.hip-testsdiscovers and runs roughly 4,900 tests, of which about 2,050 pass; the remaining failures are the same pre-existingbad-free.Earlier runs in the sequence failed on infrastructure rather than test content, and each was fixed separately:
build_stagesnarrows the graph — fix(ci): skip the libraries baseline gate instead of failing it #8161Submission Checklist