[CI] Refactor CI workflow into stage classified by hardware and gpu num, fix buggy tests - #1149
Conversation
There was a problem hiding this comment.
Code Review
This pull request refactors the CI test registration and filtering system to implement a label-based execution model. Key changes include the introduction of a canonical label registry in tests/ci/labels.py, updating the CIRegistry dataclass to support labels and always_on flags, and refactoring the AST-based test collection logic to validate these labels. The execution logic in run_suite.py now supports filtering tests by intersecting PR labels with test metadata, and the suite taxonomy has been updated to a new naming convention. Additionally, numerous test files were migrated to the new registration signature. Feedback was provided regarding the readability of the control flow in the collect_tests function, suggesting the restoration of an else block to make the logic more explicit.
| if sanity_check: | ||
| raise ValueError(msg) | ||
| else: | ||
| warnings.warn(msg, stacklevel=2) | ||
| continue | ||
|
|
||
| warnings.warn(msg, stacklevel=2) | ||
| continue |
There was a problem hiding this comment.
For improved readability and to make the control flow more explicit, it would be better to restore the else block here. While the current implementation is functionally correct because raise ValueError(msg) will exit the function, an else block clearly separates the code paths for sanity_check=True and sanity_check=False.
| if sanity_check: | |
| raise ValueError(msg) | |
| else: | |
| warnings.warn(msg, stacklevel=2) | |
| continue | |
| warnings.warn(msg, stacklevel=2) | |
| continue | |
| if sanity_check: | |
| raise ValueError(msg) | |
| else: | |
| warnings.warn(msg, stacklevel=2) | |
| continue |
Address the CodeQL warning on PR #1149: explicitly scope the workflow to `contents: read` (the minimum needed for actions/checkout). The workflow only reads PR labels (from the event payload, no extra permission needed), runs tests on self-hosted runners, and uses workflow_dispatch -- none of those need write permissions to contents, issues, or PRs. Verified actionlint 0 errors and pytest tests/ci/ still 45 passed.
Add .humanize/ (RLCR / gen-idea / gen-plan local state) and docs/plans/ (local planning artifacts) to .gitignore so the RLCR loop's clean-working-tree requirement is met without committing volatile planning outputs. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
…LABELS
M1 of the suite-as-runner-class CI refactor (docs/plans/ci-refac-plan-01.md):
- New tests/ci/labels.py exports KNOWN_LABELS: dict[str, str] listing the
9 canonical domain labels (megatron, sglang, fsdp, short, long, ckpt, lora,
precision, glm5). Meta-labels run-ci-image / run-ci-all are intentionally
excluded; they go through a separate --match-all-labels path (M2).
- tests/ci/ci_register.py rewritten:
- register_cuda_ci / register_cpu_ci now take keyword-only labels: list[str]
(required) and always_on: bool = False. num_gpus is removed entirely;
GPU count is encoded in the suite name going forward.
- CIRegistry dataclass gets labels, always_on fields and loses num_gpus.
- New module-level helpers _extract_constant / _extract_list_constant.
- RegistryVisitor._parse_call_args enforces six rules at AST-collection
time: positional args limited to (est_time, suite), unknown kwargs
rejected (including legacy num_gpus=), labels required, labels=[]
requires always_on=True (one-way; never-run is forbidden), every label
must be in KNOWN_LABELS, and labels must be a list literal of strings.
- Unit tests under tests/ci/:
- test_ci_register.py — 22 tests across positive shapes, negative shapes,
and AC-1.1 AST extraction helpers (each fixture writes a temp .py file
and feeds it to ut_parse_one_file).
- test_labels.py — 4 tests for the canonical dict shape.
pytest tests/ci/test_labels.py tests/ci/test_ci_register.py -> 26 passed.
This commit is the upstream gate for the remaining refactor: 83 existing
register_*_ci callsites and the workflow file are intentionally untouched
here (M3 / M5) and will fail under the new validator until those rounds
land. The plan documents this as an atomic-merge mid-state.
Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
…tion
Rewrite `.github/workflows/pr-test.yml` per docs/plans/ci-refac-plan-01.md
milestone M5. Replace 12 hard-coded per-domain jobs (stage-b-fast-gpu,
stage-b-short, stage-b-sglang, stage-c-fsdp, stage-c-megatron,
stage-c-precision, stage-c-ckpt, stage-c-long, stage-c-lora, stage-c-all,
plus old stage-a-fast) with 3 generic jobs that delegate label-based test
selection to `tests/ci/run_suite.py --labels`:
- stage-a-cpu (CPU GitHub runner, no GPU, always-on, no partitions)
- stage-b-cpu (CPU GitHub runner, currently-empty bucket, always-on)
- stage-c-8-gpu-h100 (8-GPU self-hosted H100, matrix partition_id=[0,1])
The glm5 exception job (stage-c-glm5) is kept intact -- same custom
container `radixark/miles:glm5`, same `gpu_lock_exec.py --count 8` direct
invocation, NOT routed through run_suite.py. Its `if:` gate is widened to
`workflow_dispatch || (pr && (run-ci-glm5 || run-ci-image || run-ci-all))`
so meta-labels also trigger it. stage-a-unit-test is preserved as-is
(plan does not list it for removal).
PR label passthrough: each generic job passes ALL PR labels verbatim via
`--labels ${{ join(github.event.pull_request.labels.*.name, ' ') }}`.
run_suite.py's `strip_run_ci_prefix` strips the `run-ci-` prefix on the
Python side and ignores non-`run-ci-*` labels with a warning (Team 1
interface). When the PR carries `run-ci-image` / `run-ci-all`, or the
event is `workflow_dispatch`, an additional `--match-all-labels` flag is
appended via a GitHub Actions ternary expression; it is NOT appended to
the glm5 job (glm5 doesn't consume that flag).
ACs satisfied: AC-4 (3 generic + glm5, all old jobs deleted, no h200 job),
AC-5 (raw run-ci-* passthrough; --match-all-labels gate for generic jobs;
glm5 gate widened via `if:`; workflow_dispatch supported), AC-6
(stage-c-8-gpu-h100 partition_id=[0,1], no partitions on others), AC-7
(glm5 container/skip_dependency_install/gpu_lock_exec preserved verbatim;
gate widened), AC-8 (1-GPU tests will be picked up by stage-c-8-gpu-h100
once Team 2 migrates callsites; no stage-b-fast-gpu remains), AC-11
(actionlint clean, yaml.safe_load clean).
actionlint output:
$ /tmp/actionlint .github/workflows/pr-test.yml
(no output; 0 errors)
$ /tmp/actionlint -verbose .github/workflows/pr-test.yml
Found 0 parse errors in 0 ms for .github/workflows/pr-test.yml
Found total 0 errors in 0 ms for .github/workflows/pr-test.yml
YAML parse + job list:
$ python -c "import yaml; print(list(yaml.safe_load(open('.github/workflows/pr-test.yml'))['jobs'].keys()))"
['stage-a-cpu', 'stage-a-unit-test', 'stage-b-cpu', 'stage-c-8-gpu-h100', 'stage-c-glm5']
4-scenario walk-through:
(1) PR no run-ci-* label:
stage-a-cpu runs; --labels empty, no --match-all-labels ->
filter keeps only always_on=True tests.
stage-b-cpu runs; empty bucket, run_suite.py exits 0.
stage-c-8-gpu-h100 runs (2 partitions); only always_on=True tests.
stage-c-glm5 SKIPPED (no glm5/image/all label).
(2) PR with run-ci-megatron:
stage-a-cpu runs; --labels run-ci-megatron, no --match-all.
stage-a-cpu tests are always_on=True (no
megatron tests registered there); megatron tests
live on stage-c-8-gpu-h100, ignored here.
stage-b-cpu runs; empty bucket.
stage-c-8-gpu-h100 runs (2 partitions); filter keeps always_on
plus tests whose labels intersect {megatron}.
stage-c-glm5 SKIPPED.
(3) PR with run-ci-image:
stage-a-cpu runs WITH --match-all-labels -> all enabled
stage-a-cpu tests (predicate bypassed).
stage-b-cpu runs WITH --match-all-labels (empty bucket).
stage-c-8-gpu-h100 runs WITH --match-all-labels (2 partitions);
every enabled test in the suite runs.
stage-c-glm5 RUNS (gate widened); execute_command is
unchanged direct gpu_lock_exec invocation, NO
--match-all-labels flag (glm5 doesn't go through
run_suite.py).
(4) workflow_dispatch:
stage-a-cpu runs WITH --match-all-labels (empty --labels).
stage-b-cpu runs WITH --match-all-labels (empty bucket).
stage-c-8-gpu-h100 runs WITH --match-all-labels (2 partitions).
stage-c-glm5 RUNS via workflow_dispatch branch of the gate.
Diff: -128 / +47 (net -81 lines).
BitLesson: NONE
… signature
Replace the legacy `register_*_ci(est_time=..., suite=..., num_gpus=...)`
calls under tests/ (excluding tests/ci/) with the Round-0 keyword-only
signature introduced by `dedded3e4`. Every callsite now declares its
PR-side label set explicitly and either gates on labels or opts into the
per-commit baseline via `always_on=True`. `num_gpus=` is gone everywhere;
suite names collapse to the new stage-* taxonomy.
Migration matrix (M3 phase A / B / C / D):
Group A -- 41 CPU-fast callsites (was `stage-a-fast`)
-> register_cpu_ci(est_time=..., suite="stage-a-cpu",
labels=[], always_on=True)
Group B -- 15 1-GPU H100 callsites (was `stage-b-fast-1-gpu`)
-> register_cuda_ci(est_time=..., suite="stage-c-8-gpu-h100",
labels=[], always_on=True)
Group C -- 26 label-gated 8-GPU callsites
-> register_cuda_ci(est_time=..., suite="stage-c-8-gpu-h100",
labels=["<domain>"])
short(6) / sglang(4) / fsdp(4) / megatron(6) / precision(1)
/ ckpt(2) / long(2) / lora(1)
Group D -- 1 glm5 callsite (D6/D7 exception; suite name PRESERVED)
-> register_cuda_ci(est_time=..., suite="stage-c-glm5-8-gpu",
labels=["glm5"])
Existing `disabled=` reasons are preserved verbatim on all 6 disabled
tests (5 FSDP + 1 precision). `est_time` is preserved verbatim on every
callsite. No callsite uses `nightly=` today.
ACs satisfied: AC-3 (all 83 callsites on new signature, collect_tests
returns 83 CIRegistry with 0 ValueError), AC-7 callsite portion (glm5
suite name retained at `stage-c-glm5-8-gpu`, glm5 label declared),
AC-8 callsite portion (all 15 1-GPU tests now register against
`stage-c-8-gpu-h100` so they ride the merged 8-GPU runner under D9
phase 1), AC-12 (CUDA visibility audit on the 15 1-GPU files).
AC-12 audit (15 Group-B files):
0/15 files need a CUDA_VISIBLE_DEVICES fix. None of the 1-GPU tests
read `torch.cuda.device_count()`, shell out to `nvidia-smi`, read
`CUDA_VISIBLE_DEVICES`, or enumerate devices via
`range(torch.cuda.device_count())`. Searched additionally for
`world_size`, `local_world_size`, `cuda.device(`, `num_gpus`,
`n_gpus`, `N_GPUS`; none matched. The 15 tests are safe to run on an
8-GPU runner with `gpu_lock_exec --count 8`; no extra environment
pinning needed in this PR.
Verification:
$ python -c "from pathlib import Path; from tests.ci.ci_register \
import collect_tests; \
files = sorted({str(p) for p in Path('tests').rglob('*.py') \
if ('register_cuda_ci' in p.read_text() or \
'register_cpu_ci' in p.read_text()) \
and 'tests/ci/' not in str(p)}); \
rs = collect_tests(files); \
print(f'{len(rs)} registries from {len(files)} files')"
83 registries from 83 files
$ python -m pytest tests/ci/test_ci_register.py tests/ci/test_labels.py
26 passed in 0.04s
Backend / suite / label distribution check:
Backend: CUDA=42, CPU=41
Suite: stage-a-cpu=41, stage-c-8-gpu-h100=41, stage-c-glm5-8-gpu=1
Always-on=True: 56 (41 CPU + 15 1-GPU CUDA)
Disabled: 6 (5 FSDP + 1 precision)
Labels: short=6 sglang=4 fsdp=4 megatron=6 precision=1 ckpt=2
long=2 lora=1 glm5=1
Out of scope for this commit: `tests/ci/run_suite.py` (Team 1).
BitLesson: NONE
…y (M2)
Implements Milestone 2 of the CI refactor (AC-9 + AC-10):
- run_suite.py
* `PER_COMMIT_SUITES` updated to the new 5-suite taxonomy
(CPU: stage-a-cpu, stage-b-cpu; CUDA: stage-c-8-gpu-h100,
stage-c-4-gpu-h200, stage-c-glm5-8-gpu). All 10 legacy suite
names removed. `stage-c-4-gpu-h200` is reserved for the
H200 follow-up PR (registered tests may target it; no
workflow job yet).
* New helper `strip_run_ci_prefix(raw_labels)` on the Python
side. Empty input -> empty set. Items missing the `run-ci-`
prefix are skipped after `warnings.warn(...)` (decision:
skip rather than include, to avoid bare strings silently
matching the wrong domain label).
* `filter_tests` gains `labels: set[str] | None` and
`match_all_labels: bool` params. Predicate:
- match_all_labels=True: ignore labels + always_on
(subject to hw/suite/nightly/disabled).
- match_all_labels=False: include if `test.always_on or
(set(test.labels) & labels)`.
match_all_labels takes precedence when both are passed.
* argparse: `--labels` (`nargs="*"`, default `[]`) and
`--match-all-labels` (boolean flag) wired through
`run_a_suite` -> `strip_run_ci_prefix` -> `filter_tests`.
All existing flags / defaults untouched; partition logic
unchanged.
- test_run_suite.py (new)
* 6 AC-9 filter scenarios: no labels, single label, multi
labels (OR), --match-all-labels, unknown PR-side label
silent no-op, --match-all-labels precedence over --labels.
* Focused `strip_run_ci_prefix` cases: empty input, prefix
stripping, dedup, non-prefixed input warns+skipped, mixed
input, empty-string entries.
* AC-10 PER_COMMIT_SUITES exactness + no-legacy-name check.
* Cross-suite / cross-backend / nightly isolation tests for
the base predicate.
Test result: pytest tests/ci/test_run_suite.py
tests/ci/test_ci_register.py tests/ci/test_labels.py -v
-> 45 passed in 0.05s.
…-8-gpu-h100 Adds the missing register_cuda_ci(...) call (megatron domain, 900s budget) to the qwen3.5-35B-A3B-cp megatron e2e test. The file was overlooked during the 83-callsite migration in ac32018; this brings the total registered callsite count from 83 to 84. Verified: collect_tests returns 84 registries (was 83); unit tests still green (45 passed).
Address the CodeQL warning on PR #1149: explicitly scope the workflow to `contents: read` (the minimum needed for actions/checkout). The workflow only reads PR labels (from the event payload, no extra permission needed), runs tests on self-hosted runners, and uses workflow_dispatch -- none of those need write permissions to contents, issues, or PRs. Verified actionlint 0 errors and pytest tests/ci/ still 45 passed.
… always run Trigger policy is now determined entirely by labels: - labels omitted / None / [] -> always run on every PR - labels=["x", ...] -> PR must carry run-ci-x (OR semantics) Removes the always_on parameter and the labels=[] never-run rule, which together produced a confusing always_on + disabled semantic overlap. The validator now accepts an optional labels list (or None) and only rejects: - unknown labels not in KNOWN_LABELS - wrong types (labels must be list or None) - positional third argument (labels stays keyword-only) - unknown kwargs (including legacy num_gpus and the now-removed always_on) Unit tests updated: added always-run shapes (labels omitted / None / []), explicit rejection of legacy always_on kwarg, and the AST helper now treats literal None as equivalent to an empty list.
… empty-labels semantic PER_COMMIT_SUITES gains stage-b-8-gpu-h100, the new always-run GPU bucket that will hold the 15 fast GPU tests previously folded into stage-c-8-gpu-h100 with always_on=True. The label filter predicate flips from `t.always_on or (set(t.labels) & label_set)` to `not t.labels or (set(t.labels) & label_set)` -- tests registered with empty labels (None / []) survive regardless of the PR-supplied set. Tests updated accordingly: the _make() factory drops the always_on parameter, fixture tests use labels=[] for the always-run shape, and a new test exercises stage-b-8-gpu-h100 routing.
Following the always_on removal in tests/ci/ci_register.py, the 41 CPU fast tests that previously registered as register_cpu_ci(est_time=..., suite="stage-a-cpu", labels=[], always_on=True) become register_cpu_ci(est_time=..., suite="stage-a-cpu", labels=[]) Same trigger semantics (always run on every PR via the empty labels list); just one fewer redundant kwarg per callsite.
The 15 originally 1-GPU tests that landed in stage-c-8-gpu-h100 with
always_on=True now belong in their own always-run GPU bucket
stage-b-8-gpu-h100, so the suite-as-runner-class taxonomy regains a
clean per-stage trigger split: stage-a / stage-b are always run,
stage-c is label-gated.
Each callsite changes from
register_cuda_ci(est_time=..., suite="stage-c-8-gpu-h100",
labels=[], always_on=True)
to
register_cuda_ci(est_time=..., suite="stage-b-8-gpu-h100", labels=[])
Same trigger semantics (empty labels => always run); just the suite
name (and thus the workflow job that picks them up) changes.
Three CPU-fast tests landed on main during the refactor session (true-on-policy test_model_provider, test_qwen2_true_on_policy_conversion, session_verify_runner) and still used the legacy suite name "stage-a-fast" with the pre-labels signature. They are conceptually identical to the other 41 stage-a-cpu callsites: CPU smoke tests that run on every PR. Rename suite "stage-a-fast" -> "stage-a-cpu" on each. The pre-refactor signature already had no labels kwarg, which under the new optional- labels rules is equivalent to labels=[] (always run) -- so no further edits are needed. After this commit collect_tests over tests/ returns 87 registries with zero legacy suite names left in the registered set.
Add the workflow job that consumes the new stage-b-8-gpu-h100 suite (15 fast GPU tests, all registered with labels=[]). Same shape as the other generic jobs: - always runs on every PR (no per-job label gate) - `needs: [stage-a-cpu]` preserves CPU-gates-GPU sequencing - passes raw `run-ci-*` labels to run_suite.py for in-Python filtering - adds `--match-all-labels` when meta-labels run-ci-image / run-ci-all are present, or on workflow_dispatch No partitioning yet -- the 15 tests are short enough to fit one job's wall-clock budget; add a matrix later if est_time grows. actionlint clean; yaml job list now reads [stage-a-cpu, stage-a-unit-test, stage-b-cpu, stage-b-8-gpu-h100, stage-c-8-gpu-h100, stage-c-glm5]
Add a new `stage-c-4-gpu-h200` job to pr-test.yml routed to runners that carry BOTH the `h200` hw-type label and the `4gpu` gpu-count label, so the H200 fleet (one physical node split into two 4-GPU self-hosted workers pinned to host GPUs 0-3 / 4-7 via runner-launch-time CUDA_VISIBLE_DEVICES) can be addressed unambiguously. Update all existing GPU callers (stage-a-unit-test, stage-b-8-gpu-h100, stage-c-8-gpu-h100, stage-c-glm5) to the symmetric two-label form `["h100", "8gpu"]` so H100 work cannot accidentally route to an H200 host that happens to carry an `8gpu` label. Add two reusable-workflow inputs to _run-ci.yml: miles_test_few_gpu (default '0'): replaces the previous hardcoded env value. The H200 stage passes '1' so FSDP-conditional tests take their <=4-GPU branch. skip_gpu_lock (default false): when true, runs execute_command bare, bypassing the gpu_lock_exec.py wrapper. The H200 stage passes true. The wrapper unconditionally overwrites CUDA_VISIBLE_DEVICES at tests/ci/gpu_lock_exec.py:28; combined with the H200 fleet's shared host /dev/shm (via --ipc=host) and worker-launch-time CVD pinning, leaving the wrapper in place would clobber the worker-injected CVD and cause cross-worker physical-GPU collisions. All other GPU callers keep the wrapper (skip_gpu_lock defaults to false). Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Move 18 tests whose `world_size = max(TP * CP, EP * ETP) * PP` is already <=4 from stage-c-8-gpu-h100 to the new stage-c-4-gpu-h200 suite. For 6 of them, also drop stale 8-GPU GPU-cardinality anchors (NUM_GPUS = 8 -> 4 and/or hardcoded --actor-num-gpus-per-node 8 -> 4) that didn't match the file's actual parallelism configuration. Migrated: ckpt/test_qwen3_4B_ckpt.py TP2 CP2 -> world=4 fsdp/test_qwen3_0.6B_fsdp_distributed.py FEW_GPU path <=2 fsdp/test_qwen3_0.6B_megatron_fsdp_align.py FEW_GPU path =1 fsdp/test_qwen3_4B_fsdp_true_on_policy.py FEW_GPU path <=4 fsdp/test_qwen3_vl_4B_fsdp.py FSDP only =1 lora/test_lora_qwen2.5_0.5B.py all parallelism =1 sglang/test_chat_input_ids_equivalence.py inference sglang/test_r3_router_equivalence.py num_gpus=4 sglang/test_session_server_multi_role.py inference sglang/test_tito_logprob_equivalence.py single-GPU megatron/test_mimo_7B_mtp_only_grad.py TP2 -> world=2 megatron/test_quick_start_glm4_9B.py TP2 CP2 -> world=4 megatron/test_qwen3_4B_ppo.py TP2 CP2 -> world=4 long/test_qwen2.5_0.5B_gsm8k.py FEW_GPU path <=2 long/test_qwen2.5_0.5B_gsm8k_async.py FEW_GPU path <=2 short/test_qwen2.5_0.5B_gsm8k_async_short.py FEW_GPU path <=4 short/test_qwen2.5_0.5B_gsm8k_short.py FEW_GPU path <=4 short/test_qwen3_0.6B_fsdp_colocated_2xGPU.py FEW_GPU path <=2 Stale anchor fixes (NUM_GPUS = 8 -> 4 and/or --actor-num-gpus-per-node 8 -> 4): ckpt/test_qwen3_4B_ckpt.py, fsdp/test_qwen3_vl_4B_fsdp.py, lora/test_lora_qwen2.5_0.5B.py, megatron/test_mimo_7B_mtp_only_grad.py, megatron/test_quick_start_glm4_9B.py, megatron/test_qwen3_4B_ppo.py. Left on stage-c-8-gpu-h100 (out of scope for this commit): megatron/test_glm47_flash_r3_mtp.py user-spec exclusion megatron/test_qwen3_5_35B_A3B_cp.py user-spec exclusion ckpt/test_glm47_flash_ckpt.py EP=8 reduction pending megatron/test_qwen3_30B_A3B.py TP=2 + EP=4 reduction pending megatron/test_qwen3_30B_A3B_r3.py ditto + routing-replay risk sglang_config/test_sglang_config.py NUM_GPUS=8 hardcoded sglang_config/test_sglang_config_mixed_offload.py ditto sglang_config/test_sglang_config_mixed_offload_ft.py ditto Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Detect when an e2e test registered to stage-c-4-gpu-h200 still has a GPU-count anchor that wouldn't fit a 4-GPU host: NUM_GPUS=8, --actor-num-gpus-per-node 8, --rollout-num-gpus-per-engine >=5, --tensor-model-parallel-size >=5, --context-parallel-size >=3, --expert-model-parallel-size >=5, --expert-tensor-parallel-size >=2, --pipeline-model-parallel-size >=2. These per-axis bounds are necessary (not sufficient) for `world_size = max(TP*CP, EP*ETP) * PP <= 4`. The lint runs on stage-a-cpu (file scan only; no GPU needed). A second test asserts specificity by checking that the two h100-exclusion files (megatron/test_glm47_flash_r3_mtp.py, megatron/test_qwen3_5_35B_A3B_cp.py) still trip at least one pattern -- otherwise the regex set is vacuous. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Reduce parallelism on three MoE tests whose current world_size = 8 (per `max(TP * CP, EP * ETP) * PP`) and migrate them to stage-c-4-gpu-h200, landing each at world_size = 4. NUM_GPUS and --actor-num-gpus-per-node are dropped to 4 in lockstep with the parallelism config. ckpt/test_glm47_flash_ckpt.py: EP 8 -> 4 (TP=4 and CP=1 unchanged; dense side already at 4). world = max(4*1, 4*1) * 1 = 4. megatron/test_qwen3_30B_A3B.py: TP 4 -> 2 AND EP 8 -> 4 simultaneously (CP=2 unchanged; both groups were saturated at 8). The non-INT4 rollout branch's --rollout-num-gpus-per-engine drops 8 -> 4; the INT4 branch already uses 1 and is unchanged. world = max(2*2, 4*1) * 1 = 4. megatron/test_qwen3_30B_A3B_r3.py: Same TP/EP/NUM_GPUS/actor diff as _30B_A3B; --rollout-num-gpus-per-engine also drops 8 -> 4. Routing-replay correctness at EP=4 (vs the originally EP=8 recorded routing) is a behavior-change observation point for the first H200 PR run -- if the replay diff is intolerable, this test will need a fresh capture at EP=4 (out of scope for this commit; tracked in the plan as a known follow-up). est_time values are kept at their h100 baselines and will be empirically recalibrated on H200 after the first real PR run (deferred by design). After this commit, stage-c-4-gpu-h200 has 16 enabled tests (5 fsdp disabled-for-other-reasons remain in the registry) and stage-c-8-gpu-h100 residual is 5 enabled (2 user-spec exclusions + 3 sglang_config files that hardcode NUM_GPUS=8 and are tracked as a separate follow-up). Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
The previous commit `6dec5b19d` switched all GPU callers to a two-label
`runs_on` of the form `'["h100", "8gpu"]'` / `'["h200", "4gpu"]'`. That
intent was correct, but the substitution mechanism was wrong: with
`runs_on: { type: string }` and `runs-on: ${{ inputs.runs_on }}`, GitHub
Actions injects the value verbatim as a string scalar, so the runner
selector saw one label literal `'["h200", "4gpu"]'` (brackets included)
rather than two labels `h200` and `4gpu`. No runner carries that literal,
so every GPU job on PR #1149 queued indefinitely.
Fix:
- Wrap `inputs.runs_on` in `fromJSON(...)` so the string is parsed into a
YAML list before reaching `runs-on:`.
- Change the default from `'8gpu'` to `'["8gpu"]'` so the default
preserves single-label `8gpu` semantics under the new JSON convention.
All callers already pass JSON-encoded strings (verified in pr-test.yml).
Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
With 2 H200 workers in the fleet, each runs 2 partitions sequentially. Total wall time is unchanged (bounded by the longest single test -- test_session_server_multi_role.py at est_time=9600s), but each partition is smaller, so a retry only repeats ~1/4 of the suite instead of ~1/2. Will collapse back to 2 partitions once a follow-up PR splits test_session_server_multi_role.py into shorter sub-tests. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
….5MoE SGLang now requires `--mamba-scheduler-strategy extra_buffer` and `SGLANG_ENABLE_SPEC_V2=1` whenever speculative decoding is combined with radix cache on `Qwen3_5MoeForConditionalGeneration`; the failure is: ValueError: Speculative decoding for Qwen3_5MoeForConditionalGeneration is not compatible with radix cache when using --mamba-scheduler-strategy no_buffer. The prod script `scripts/run_qwen3_5_35b_a3b_mtp_cp2_ep8.py` already pairs the two (lines 126 + 164); the e2e test had drifted. Add the same two settings to the test so it stays aligned with prod. Pre-existing upstream dependency drift, surfaced by PR #1149's first real CI run (commit 8d07a0c) on stage-c-8-gpu-h100. Not caused by the H200 migration; the test was not touched by any prior commit in this PR. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
`run_session_verify` defaults `num_gpus=8` (miles/utils/test_utils/session_verify_runner.py:86), which becomes `--actor-num-gpus-per-node 8` at the launch site. After the suite swap to stage-c-4-gpu-h200, the test runs on a 4-GPU host and the launch fails immediately when it cannot allocate 8 GPUs. Override `num_gpus=4` explicitly in `_run_one`. Update the module docstring from "Requires 8 GPUs" to reflect the new override. The GPU-cardinality lint (tests/utils/test_gpu_cardinality_lint.py) does not catch this case because the `8` lives in a helper-function default in `miles/utils/test_utils/`, not as a literal in the test file. Cross-module taint tracking is over-engineering for a one-off audit; the targeted override is sufficient. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Adds docker-compose.override.yml defining scitix-72-0 / scitix-72-1
services (h200, 4gpu labels) to register on radixark/miles, pinned to
runner image 2.334.0 (the 2.329.0 from the base compose was deprecated
by GitHub Actions). Updates tests/ci/README.md path example to use the
checkout location actually used on these runners.
Pre-RLCR baseline for the upcoming "delete gpu_lock_exec.py + parameterize
DATA_ROOT" plan, which will modify this override file to add per-runner
CUDA_VISIBLE_DEVICES pinning and switch hardcoded /data/miles_ci to
\${DATA_ROOT:-/data}.
…LE_DEVICES; parameterize DATA_ROOT
Deletes the 199-line userspace fcntl GPU coordinator and replaces its role
with runner-launch-time CUDA_VISIBLE_DEVICES pinning, extending the existing
scitix-73 H200 split-worker pattern to scitix-72. The two wrapper invocation
sites (_run-ci.yml's execute step and pr-test.yml's stage-c-glm5
execute_command) are flipped to run the bare command. The gpu_count and
skip_gpu_lock workflow inputs are removed from _run-ci.yml and from every
pr-test.yml callsite.
GPU partitioning now has two layers. Driver layer: --gpus all remains in
container.options so the inner job container's nvidia-smi -L still shows
every physical GPU (removing it would default NVIDIA_VISIBLE_DEVICES=void
and zero out GPU access). CUDA layer: each runner container sets
CUDA_VISIBLE_DEVICES in its compose environment (0,1,2,3 on scitix-72-0,
4,5,6,7 on scitix-72-1) and the new bare `--env CUDA_VISIBLE_DEVICES` flag
in container.options propagates that value to the inner container. miles
code already reads CVD (train_actor.py, sglang_engine.py) so this is the
natural pin. Single-runner H100 hosts (novita) intentionally leave CVD
unset; the bare --env flag forwards "unset" and CUDA defaults to all 8
GPUs. scitix-73 was already using this convention via actions-runner/.env;
no external compose changes are required for it or novita.
Introduces a single DATA_ROOT parameter for the host-side data tree:
docker-compose.yml + docker-compose.override.yml use ${DATA_ROOT:-/data}
identically on both sides of the bind mount and in the runner's --work
flag (identity is mandatory for docker-out-of-docker path forwarding when
the runner spawns sibling job containers via the mounted docker.sock).
_run-ci.yml exposes a data_root workflow input (default '/data') and
plumbs it into the four -v lines on the host side only; container-side
targets (/data/miles_ci, /root/models, /root/datasets, /root/.cache/huggingface)
stay fixed because 50+ test files hardcode them. pr-test.yml's four
h100/8gpu callsites (stage-a-unit-test, stage-b-8-gpu-h100,
stage-c-8-gpu-h100, stage-c-glm5) pass data_root: '/mnt/nvme0n1';
stage-c-4-gpu-h200 omits it and rides the /data default; CPU callsites
need no opt-in.
Cleanup: docker-compose.yml's base runner service drops replicas: 8 to
replicas: 1 (the value was previously neutralized by the override's
profiles: disabled, but kept fragile against fresh clones that omit the
override). README.md and .env.example are rewritten to document the new
conventions, the DooD identity rule, the CUDA-layer partitioning, the
external-host note (scitix-73 / novita already match), and the two
restart modes (env-only via compose down/up, label changes via deleting
.runner).
Verification done locally:
- docker exec scitix-72-0 printenv CUDA_VISIBLE_DEVICES -> 0,1,2,3
- docker exec scitix-72-1 printenv CUDA_VISIBLE_DEVICES -> 4,5,6,7
- docker compose config validates with DATA_ROOT=/data, =/mnt/nvme0n1, and unset (falls back to /data)
- Both runners online and 'Listening for Jobs' (visible in GitHub API)
- python3 yaml.safe_load passes on both workflow files
- grep "gpu_lock_exec|skip_gpu_lock" repo-wide returns zero matches
- grep "gpu_count:" .github/workflows/ returns zero matches
Deferred to post-merge CI verification (require workflow_dispatch / PR trigger):
- task12: h200/4gpu canary (AC-2, AC-3)
- task13: h100/8gpu canary on /mnt/nvme0n1 (AC-6, AC-8)
- task14: dual-runner concurrent canary on scitix-72 (AC-2, optional per DEC-4)
- task16: full pr-test.yml end-to-end (AC-8)
Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
|
synced offline with Yusheng |
Disabled in #1149 as "Disabled due to bugs" with a FIXME pointing at Megatron. Since then the Megatron pin picked up the dist-ckpt load fix for bare BytesIO _extra_state (radixark/Megatron-LM#53, fixes #1293), and the sibling qwen3_4B ckpt test disabled in the same sweep proved stale on re-run (#1271). The training config itself (TP2/PP2/CP2/EP4 + MTP + EAGLE) already runs green in test_glm47_flash/test_r3_mtp; the save -> load -> async_save -> load roundtrip is the only untested part. Re-enable and let the run-ci-ckpt CI run on 8xH100 validate it. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Disabled in #1149 as "Disabled due to bugs" with a FIXME pointing at Megatron. Since then the Megatron pin picked up the dist-ckpt load fix for bare BytesIO _extra_state (radixark/Megatron-LM#53, fixes #1293), and the sibling qwen3_4B ckpt test disabled in the same sweep proved stale on re-run (#1271). The training config itself (TP2/PP2/CP2/EP4 + MTP + EAGLE) already runs green in test_glm47_flash/test_r3_mtp; the save -> load -> async_save -> load roundtrip is the only untested part. Re-enable and let the run-ci-ckpt CI run on 8xH100 validate it. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
(written by human)
Motivation
Existing CI workflow is classified by label. But when scaling up to different type of hardware, (h100, h200 or even b/gb), the current design sucks, hard to delegate tasks to different runners.
Design
Worker split
Split to 3 types of workers, each include:
8-gpu-h100:fp16still insufficient. (especially tests withdeepep, but not sure about the reason.)4-gpu-h200:2-gpu-h200:These three worker matching to the name of stages. And remove old
gpu_exec_lock.pydesign.Test split
_common.py. Reused by other CI. e.g.tests/e2e/megatron/test_qwen3_30B_A3B/_common.pyHardcode removal
NUM_GPUSlabel orCaseConfig, and infer parallelism settings and train/rollout GPU placement from formulas instead of hardcoding them. This will make future CI migration easier.Keep PR label-trigger
Contributor still need to add labels manually to trigger CI. No auto-detect logic.
Skills
Improved runner setup skills. one-click to setup different type of runner.
Existing CI Bug fixes
Notice: This PR is mostly about CI workflow refactor, only including naive CI bug fix.
wandb.init(), with no response for 90 seconds. Fix by extending default time.--use-kl-loss, so the r3 check foractor&refat step 0 is always skipped. Add back the check and loose threshold to5e-3.--num-rolloutfor megatron test from 3 to 2. 3 is not effective but consume more gpus./tmpfolder into container, which cause race condition of writing ray logs.Other buggy CI are disabled and easy repro on local machine, and
FIXMEwill cover them.