[CI] Run gfx950 validation on the MI355 and MI35x pools - #1084
Closed
coderfeli wants to merge 7 commits into
Closed
Conversation
Route each gfx950 validation through one shared label so the first available pool runs it, while preserving MI355-only dashboard benchmarks. Co-authored-by: Cursor <cursoragent@cursor.com>
…lling The shared gfx950 label can land on MI355-1, MI35x-1, or MI35x-8, and dashboard performance history stays MI355-only, so benchmarks run on MI355-1 alone. That skip was silent: a run that produced no benchmark numbers and no comparison against main or the latest tag looked identical to a clean perf pass. Resolve eligibility once in a dedicated step, export RUN_BENCH, and record the outcome as a job annotation plus a step-summary line. The three benchmark steps now share that single condition instead of repeating the predicate. The predicate also disagreed with the dashboard: startsWith() accepted linux-flydsl-mi355-10-*, which ingest.py's runner_of() rejects, so such a run would benchmark but never be ingested. Match exactly the pool label or the label plus an ARC suffix, as runner_of() does. Also fix two labelling slips: test-skip echoed matrix.runners (linux-flydsl-gfx950) while reporting the check named after matrix.check, and runner-config.yml used gpu_arch: gfx950, which runner_label_sort_key_with_inventory() cannot match against its (mi\d+[a-z0-9x]*) family regex, sorting the entry to the bottom of the runner report instead of beside the MI35x pools it aliases. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
runner_of() resolved a job purely from runner_name when the API supplied one, discarding the job-name check that previously gated it. prepare-mlir has runs-on: linux-flydsl-mi325-1, so it started resolving to that box: its LLVM build log was downloaded and fed to parse_bench.parse_log() on every ingest, and its status was appended as a second job_status entry for the runner, where app.js's `byRunner[j.arch] = j` lets the last one win and decide the board chip. Restore the gate: a job qualifies only if its name carries a "(<label>)" suffix, marking it a per-runner matrix job. runner_name still wins for those, which is what the shared gfx950 entry needs. Multi-GPU jobs and historical runs resolve exactly as before. Also move "Resolve benchmark eligibility" ahead of "Run tests". It had no `if:`, so a test failure skipped it and left RUN_BENCH unset — the annotation naming the pool went missing precisely when a run failed, and "Show benchmarks logs" stopped firing on failure as it used to. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
A job holds its entire runner for its full duration, and HIP_VISIBLE_DEVICES only masks devices inside the container — it releases nothing back to the scheduler. Putting the shared label on an 8-GPU pool would therefore park 7 idle GPUs and block multi-GPU work for the whole build+test window, with no way for the job to ask for less. Assign it to MI355-1 and MI35x-1 only, and say why in both the workflow and the runner inventory. Keep the device-0 pin: it makes run_tests.sh deterministic instead of selecting a GPU by free VRAM, and bounds the damage if the label is ever assigned to a multi-GPU pool anyway. Also report the real box in the wheel-test summary. SUMMARY_RUNNER used matrix.runners, which for the shared entry is the hardware-agnostic label, so the summary heading no longer said which GPU ran the wheel. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
The MI35x validation entries added in #1075 and the shared `linux-flydsl-gfx950` label built on top of them in this branch both target runner pools that no runner ever claims. Jobs requesting them sit in `queued` indefinitely: run 33401100150 (main) test (linux-flydsl-mi35x-1) queued, runner="" run 33397592944 (PR) test (linux-flydsl-mi355-1) queued, runner="" labels=[linux-flydsl-gfx950] Because the shared entry replaced the `linux-flydsl-mi355-1` entry, this branch also lost the gfx950 coverage that still works on main. A queued job cannot be worked around from the workflow side: `continue-on-error` and `fail-fast` only apply once a job starts, so a label with no runner behind it blocks the run and its required check for as long as the run lives. Restore the matrix to labels with confirmed live runners (`linux-flydsl-mi325-1`, `linux-flydsl-mi355-1`, `linux-flydsl-navi-2`, and `linux-flydsl-mi325-8` / `linux-flydsl-mi355-8` for the label-gated multi-GPU job). This reverts #1075 and drops the shared-label scheme; the net change against the merge base is exactly the inverse of #1075. Adding gfx950 capacity needs a runner scale set that actually exists — one named `linux-flydsl-gfx950` whose node selector spans the MI350 and MI355 nodes. It cannot be done by labelling: these are ARC scale sets (runner names look like `linux-flydsl-mi355-1-g2mdh-runner-xf25l`), and `runs-on` matches a scale set by its single unique name, so several pools cannot share one label.
coderfeli
changed the title
Give each gfx950 pool its own entry in the single-GPU test matrix, so a change is validated on whichever of MI355-1 / MI35x-1 / MI35x-8 has capacity, and add MI35x-8 to the multi-GPU matrix. Actions has no "whichever label is free" primitive: multiple labels in runs-on mean AND, not OR, and a matrix is a fan-out rather than a hand-off. So the entries are additive coverage, not substitutes. Keep the two required contexts, test (linux-flydsl-mi325-1) and test (linux-flydsl-mi355-1), pinned to their own pools. Pointing a required context at a pool that is out of capacity leaves it pending and blocks every merge, which is what the earlier shared-label attempt on this branch did. The MI35x entries are not required checks. MI35x-8 runs the single-GPU suite, so pin it to device 0 instead of letting run_tests.sh choose by free VRAM. Restore the MI35x rows in runner-config.yml and the MI35x-1 entry in test-whl.yaml, both dropped by the preceding revert.
coderfeli
changed the title
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
Run the gfx950 validation on all three gfx950 pools --
linux-flydsl-mi355-1,linux-flydsl-mi35x-1,linux-flydsl-mi35x-8-- so a change is validated on whichever of them has capacity.linux-flydsl-mi35x-8is also added to the multi-GPU matrix.Why a matrix and not a shared label
The first attempt on this branch collapsed the gfx950 entries behind one shared
linux-flydsl-gfx950label. That does not work, for two reasons:runs-onmean AND (a runner carrying all of them), not OR. A matrix is a fan-out, not a hand-off. So the only way to express "either pool" is a single label that every runner in every pool carries -- which has to be assigned on the infrastructure side, not in this repo.test (linux-flydsl-mi355-1)andtest (linux-flydsl-mi325-1)are the two required contexts onmain(ruleset11400238). The shared entry kept the check name vianame:but moved itsruns-onto the new label; with no runner behind it the check stayed pending for 24h and was then cancelled. Merged, that would have blocked every PR onmain.So this PR keeps the two required contexts pinned to their own pools, where they always get a machine, and adds the MI35x pools as additive, non-required coverage.
Changes
testandtest-skipmatrices: addlinux-flydsl-mi35x-1andlinux-flydsl-mi35x-8multi-gpumatrix: addlinux-flydsl-mi35x-8CI_HIP_VISIBLE_DEVICES: pinlinux-flydsl-mi35x-8to device 0, since it runs the single-GPU suite andrun_tests.shwould otherwise pick by free VRAMrunner-config.yml/test-whl.yaml: restore the MI35x entries dropped by the preceding revert commitKnown caveats
linux-flydsl-mi35x-1was requested 12 times andrunner_namewas empty every time -- the jobs sat queued until they were cancelled. If that has not changed, these entries queue rather than run. They are not required checks so they do not block merges, but note thatmulti-gpuhasneeds: [test, ...], so a queued MI35x entry does delay it.linux-flydsl-mi35x-8holds a whole 8-GPU box for the duration of a single-GPU job (60 min timeout), leaving 7 GPUs idle and blocking multi-GPU work meanwhile. Deliberate per the request; drop that one entry if the trade is not worth it.linux-flydsl-mi35x-{1,8}toparse_bench.RUNNER_ARCHasgfx950, so their numbers land in the samegfx950dashboard series as MI355. MI350 and MI355 are not the same silicon, so mixing them in one series may show up as phantom regressions -- worth a look, but out of scope here.parse_bench.py.# The MI35x validation runners are intentionally omitted from dashboard ingestion.directly contradicts the dict below it since Ci/change image to speedup ci #1076. Left alone; flagging it.Test plan
python3 -m pytest -q .github/dashboard/ingest/(21 passed)flydsl.yaml,test-whl.yaml,runner-config.ymlwith PyYAML and assert the resulting matrices and theCI_HIP_VISIBLE_DEVICESexpression11400238that the required contexts are exactlytest (linux-flydsl-mi325-1)andtest (linux-flydsl-mi355-1), and that both keep their own pool🤖 Generated with Claude Code