[AMD] Let the ROCm suite honour the ci-sglang-pr and ci-megatron-pr directives

[fzyzcjy]

Summary

Let ROCm CI honor dependency-ref overrides while preserving unnamed baked dependencies.

Motivation

The ROCm suite parsed ci-sglang-pr: and ci-megatron-pr: directives but always kept dependency installation disabled, so a requested ref never reached an MI350 job.

Usage

Add a dependency ref to a PR body or supply the matching manual-dispatch input:

The named dependency is fetched and installed over its image copy. An unnamed dependency stays baked into the image, and runs with no override retain the existing skip-install path.

Design Notes

• Mental model: The PR body or dispatch inputs flow into resolve-ci-deps; its decision gates the MI350 stage, reaches _run-ci-rocm.yml as a boolean, then makes the reusable workflow selectively replace named dependencies while preserving unnamed baked dependencies; CUDA/CPU selection plus ROCm image selection remain outside this boundary.
• resolve-ci-deps makes the stage wait for a successful dependency decision before allocating the reusable ROCm job.
• _run-ci-rocm.yml leaves unnamed refs empty instead of substituting moving default branches, so a single override cannot replace the other baked dependency.

Verification

• TestRocmWorkflowScopeSeam::test_stage_consumes_policy_and_preserves_manual_full_scope: 1 passed; proves the stage requires resolve-ci-deps and forwards its output.
• tests/ci/run_suite.py --hw cpu --suite stage-a-cpu --auto-partition-id 3 --auto-partition-size 4 --cadence regular --labels: 2189 passed, 9 skipped; proves the corrected workflow seam passes the failed CPU shard.
• pre-commit run --files tests/ci/test/test_run_suite.py: all hooks passed.

Review Focus

• .github/workflows/pr-test-rocm.yml: directive/input detection must gate the MI350 stage without changing image or policy resolution.
• .github/workflows/_run-ci-rocm.yml: overriding one dependency must preserve the unnamed image-baked dependency.
• tests/ci/test/test_run_suite.py: the seam assertion must track the full resolver dependency and forwarding contract.

[claude]

Claude Code Review

This repository is configured for manual code reviews. Comment @claude review for a one-time review, or @claude review always to subscribe this PR to a review on every future push.

_{Tip: disable this comment in your organization's Code Review settings.}

[fzyzcjy]

Verification

I checked the claim statically against main, then dispatched one live ROCm run to prove the flag actually reaches the install step.

The gate is real. On main, _run-ci-rocm.yml declares skip_dependency_install with default: true, and both directive-consuming steps are gated on it:

      - name: Resolve dependency refs
        if: ${{ !inputs.skip_dependency_install }}
      - name: Install dependencies
        if: ${{ !inputs.skip_dependency_install }}

pr-test-rocm.yml is the only caller (git grep -n _run-ci-rocm origin/main -- .github/), and its with: block on main never passes skip_dependency_install. So the parsing was unreachable, as the PR body says.

The load-bearing assumption holds. github.event.pull_request.body is readable inside a reusable workflow invoked via uses: — the CUDA path already relies on the identical expression and works in production. From PR #2542's stage-b-2-gpu-h200 job, inside _run-ci.yml:

  PR_BODY: ci-sglang-pr: tom/refactor-miles-repo-sglang/deliver-2
Resolved: megatron=miles-main -> fetch=miles-main, sglang=tom/refactor-miles-repo-sglang/deliver-2 -> fetch=...

Positive control: live ROCm run

Dispatched on this branch (a dispatch runs the branch's own workflow definition, so the new ci_sglang_pr input exists):

gh workflow run pr-test-rocm.yml --ref tom/rocm-honors-ci-sglang-pr \
  -f ci_sglang_pr=c447264eba460045e6e961fd7cf1bfbaf038db6b

c447264eb is the parent of the sglang-miles head the image bakes — the smallest ref that is still visibly different.

Run 32332460398, green, 2 min.

• resolve-ci-deps:

    INPUT_SGLANG_PR: c447264eba460045e6e961fd7cf1bfbaf038db6b
  Dependency refs named by the dispatch inputs: installing them over the image's baked copies
  

• stage-c-4-gpu-mi350 (1) — the previously unreachable steps ran on the MI350 runner:

  Resolved: megatron=miles-main -> fetch=miles-main, sglang=c447264eba46... -> fetch=c447264eba46...
  From https://github.com/sgl-project/sglang
   * branch                  c447264eba460045e6e961fd7cf1bfbaf038db6b -> FETCH_HEAD
  Previous HEAD position was cb05a44f35 [sglang-miles] Nemotron support on sglang-miles (#27110)
  HEAD is now at c447264eba Put the DSA cuda-graph page table in the pausable memory region (#33479)
  

  and the installed package moved off the baked one:

      Uninstalling sglang-0.5.17.dev36+gcb05a44f3:
  Successfully installed sglang-0.5.17.dev35+gc447264eb
  

Shard 0 (job 96315638494) shows the same lines.

Negative control

Naming nothing must keep the image, or this change would be a regression for every ordinary ROCm run.

Level	Evidence	Result
Resolver, this branch	This PR's own ROCm run (PR body is empty)	PR_BODY: / INPUT_*: all empty → No dependency ref named: keeping the copies baked into the image, output skip_dependency_install=true
GPU job, main's definition	Run 32330568251, stage-c-4-gpu-mi350 (0)	Step list goes straight from Wait for GPU ready to Install (miles only); no Resolve dependency refs and no Install dependencies group anywhere; image keeps sglang 0.5.17.dev36+gcb05a44f3

Release CI is also unaffected: release-branch-cut.yml calls pr-test-rocm.yml via workflow_call with no PR body and no ci_*_pr dispatch inputs of its own, so resolve-ci-deps returns true there.

What this does not prove

• No ROCm test executed with the swapped SGLang. A workflow_dispatch resolves to cadence=regular with no labels, and every ROCm registration needs run-ci-amd, so the suite selected nothing: No tests found for hw=ROCM, suite=stage-c-4-gpu-mi350, cadence=regular. That is why the run was cheap, and it is also the limit of the evidence — the install path is proven, the suite passing against an overridden SGLang is not.

• Installing a CUDA-developed SGLang tree into the ROCm image is not guaranteed to work even when the fetch succeeds. This run used a one-commit-older ancestor of what the image already bakes, i.e. the mildest possible case, and ran no test on it. A real ci-sglang-pr: pointing at an unrelated CUDA branch can still fail at import or at runtime.

• Refs resolve against the image's own remotes, not against a contributor's fork. The run shows them: From https://github.com/sgl-project/sglang and From https://github.com/radixark/Megatron-LM. A branch that lives only on a personal fork will not resolve on ROCm. The PR documents this in docs/developer/contributor-guide.md.

• Naming one ref moves the other one too. With only ci_sglang_pr set, _run-ci-rocm.yml fills the unnamed side with its default miles-main rather than leaving the baked copy alone, and the run duly replaced Megatron-LM as well:

  HEAD is now at 37f91617b Support inline base64 payloads for te-precision-config-file (#72)
   * branch                miles-main -> FETCH_HEAD
  HEAD is now at 2e64d788b [optimizer] Fix LayerWise expert ownership (#82)
  

  This is pre-existing behaviour of that step, shared with the CUDA path, but it only becomes reachable on ROCm with this PR. Worth a reviewer's opinion on whether the ROCm entry point should keep the baked copy for the side that was not named.

• No GPU-level negative control on this branch. This PR touches only workflows and docs, so resolve-ci-policy put stage-c-4-gpu-mi350 in skipped_stages and the GPU job never started. The negative half above is the resolver output on this branch plus the GPU-level baseline under main's definition. To close that gap for one more minute of MI350 time, dispatch this branch with no inputs and confirm the job runs Install (miles only) and no Install dependencies:

  gh workflow run pr-test-rocm.yml --ref tom/rocm-honors-ci-sglang-pr
  

[fzyzcjy]

Round two: the PR-description path, on this PR

The first comment proved the workflow_dispatch input path. This round proves the path contributors actually use — a ci-sglang-pr: line in the PR description — and it does so on PR 2674 itself, with the MI350 stage actually executing.

Setup, all of it temporary and all of it since removed (see Cleanup at the end):

• PR description carried ci-sglang-pr: c447264eba460045e6e961fd7cf1bfbaf038db6b — the parent of the sglang-miles head the image bakes, so the checkout is visibly different but one commit away.
• A scratch commit 243678d050 re-enabled one MI350 case and added numba==0.65.1 to the install lines. Why that was necessary is the important caveat, and it has its own section below.
• The run-ci-amd label, needed to make the stage selectable at all.

Run 32334431157.

The directive is read from the body

resolve-ci-deps — both dispatch inputs empty, decision taken from PR_BODY alone:

  PR_BODY: ci-sglang-pr: c447264eba460045e6e961fd7cf1bfbaf038db6b
  INPUT_MEGATRON_PR: 
  INPUT_SGLANG_PR: 
Dependency refs named by the pull request body: installing them over the image's baked copies

The named ref reaches the MI350 runner

From stage-c-4-gpu-mi350 (1), inside the container:

Resolved: megatron= -> fetch=, sglang=c447264eba460045e6e961fd7cf1bfbaf038db6b -> fetch=c447264eba460045e6e961fd7cf1bfbaf038db6b

HEAD is now at cb05a44f35 [sglang-miles] Nemotron support on sglang-miles (#27110)
From https://github.com/sgl-project/sglang
 * branch                  c447264eba460045e6e961fd7cf1bfbaf038db6b -> FETCH_HEAD
HEAD is now at c447264eba Put the DSA cuda-graph page table in the pausable memory region (#33479)

    Uninstalling sglang-0.5.17.dev36+gcb05a44f3:
Successfully installed sglang-0.5.17.dev35+gc447264eb

The unnamed side is left alone — observed, not inferred

Only ci-sglang-pr: was named, and megatron= resolved empty rather than falling back to miles-main. Searching the whole job log for Megatron-LM returns exactly one hit, and it is the echoed shell source of the guarded block, not output from it:

  cd /root/Megatron-LM && git reset --hard HEAD && git clean -fd && git fetch origin "$MEGATRON_PR" && ...

No From https://github.com/radixark/Megatron-LM, no HEAD is now at, no pip install -e . for it. The image's Megatron-LM was never fetched, never reset, never reinstalled. For contrast, the run in the first comment — before the unnamed-side fix — did move it from 37f91617b to 2e64d788b while naming only SGLang.

The resulting tree runs

stage-c-4-gpu-mi350 (0), first attempt, no retry:

[1/1] tests/e2e/megatron/test_qwen3_4B_offload_disk_stream.py  PASS  elapsed=397s
Test Summary: 1/1 passed

Both shards green, whole run green. Two claims this run supports to different degrees, worth keeping apart:

• The directive installed what it named. Proven directly by the fetch, the checkout and the Successfully installed line above.
• The resulting tree runs. Supported, but by exactly one 4-GPU e2e case against an SGLang one commit behind the baked one. That is the mildest possible override, not evidence that an arbitrary ci-sglang-pr: will work on ROCm.

The job log also carries Health check failed. Server couldn't get a response from detokenizer for last 20 seconds and a clang++ --offload-arch=gfx950 ... failed to execute line from the SGLang engine. The test passed on its first attempt regardless, and I did not investigate whether either is normal for this image.

What the shape of this evidence is

This matters more than the green tick.

All 8 suite="stage-c-4-gpu-mi350" registrations on main carry disabled="FIXME: re-enable once this case passes on the MI350 runners." The MI350 lane runs nothing at all in PR CI today. select_skipped_gpu_stages filters on registration.disabled is None, so runnable is empty and the stage is skipped whatever label is applied. run-ci-amd alone does not change that — confirmed on run 32333645708, which still skipped the stage.

Two consequences:

• The green above comes from a test re-enabled by hand in a scratch commit, purely so the install step could be observed on a real MI350 runner. It is a statement about the directive reaching the install step and about that one case, not about the health of the ROCm suite.
• This PR cannot regress any running ROCm test today, because there are none. Merge risk is close to zero, and the end-to-end value only lands once that lane is live again — for instance when [AMD] Enable four CI tests on ROCm #2671, which re-enables four of these cases, merges.

The image is missing numba, and this feature makes that reachable

The scratch run needed numba==0.65.1 on the install lines, and the log shows it being downloaded at run time rather than found in the image:

Collecting numba==0.65.1
Collecting llvmlite<0.48,>=0.47.0dev0 (from numba==0.65.1)

So the published rocm/sgl-dev:miles-rocm720-mi35x genuinely lacks it. #2671 adds the same package to both install lines and to docker/Dockerfile.rocm, with the comment "SGLang is installed without dependencies, but its Inkling multimodal processor imports numba", and this observation corroborates that hunk rather than merely being consistent with it.

The part that concerns this PR: the install step reinstalls SGLang with --no-deps, so anyone who uses ci-sglang-pr: on ROCm today gets a newly built SGLang inside an image that has no numba. Until #2671 or an image rebuild lands, that is a live sharp edge for the first real user of this feature.

What this still does not prove

• A genuinely CUDA-only ref. The SHA used here is an ancestor of what the image already bakes. An unrelated CUDA branch can still fail at import or at runtime inside a ROCm image, and nothing here speaks to that.
• Refs on a contributor's fork. Fetches resolve against the image's own remotes — From https://github.com/sgl-project/sglang in this run — so a branch living only on a personal fork will not resolve. Documented in docs/developer/contributor-guide.md.
• ci-megatron-pr: on ROCm. Only the SGLang half was exercised end to end. The Megatron half was proven to be correctly skipped, not proven to install.
• The suite at large. One case out of eight; the other seven remain disabled.

Cleanup

Three things existed only to produce the run above, and all three have been removed:

• The run-ci-amd label is off the PR; it now carries no labels.
• The ci-sglang-pr: line is out of the description.
• The scratch commit 243678d050 was reverted by 436c1f4d96. test_qwen3_4B_offload_disk_stream.py has its disabled= line back and neither pip line carries numba==0.65.1 any more.

The net diff against main is now exactly the three intended files — .github/workflows/_run-ci-rocm.yml, .github/workflows/pr-test-rocm.yml, docs/ci/00-stage.md. Nothing from the scratch commit survives in the tree.

[guapisolo]

ci-sglang-pr: #1234

This cause cpu ci to be failed, I removed it in pr description

[guapisolo]

This publication will advance the PR head to 04c1c55049d3da09e03aeac66537963c31247253 without source-tree changes so GitHub Actions receives a fresh pull_request event after the PR body was corrected. It will carry the exact tree from 24f10a6fbadd362751fd9a438d7ea64aa0e700f1, verified by an empty git diff-tree and a clean Git-visible status.

[guapisolo]

Publishing 0522d01a03df4df7becbc00ed4b93dc908e6c8f8 will preserve the required ROCm Megatron compatibility patch after a ci-megatron-pr ref checkout, while accepting refs that already contain it and failing explicitly on conflicts. The candidate passed tests/ci/test/test_run_suite.py (87 tests), all applicable pre-commit hooks, and a three-state patch probe (applied, already-present, and incompatible fail-closed) on miles-h200-dev-2.

[guapisolo]

Publishing bb8d57e22433bf75ae8d1370b5f4510b83222a9f will source the Megatron ROCm compatibility patch from the checked-out Miles workspace because the final image removes /tmp/amd_patch, while preserving the verified apply, already-present, and incompatible-ref behavior. The candidate passed tests/ci/test/test_run_suite.py (87 tests), all applicable pre-commit hooks, and the three-state patch probe on miles-h200-dev-2.

[guapisolo]

doc change make sense