fix(ci): separate image and nightly FT tag scopes by guapisolo · Pull Request #1632 · radixark/miles

guapisolo · 2026-07-11T06:45:04Z

Summary

Apply distinct FT tag scopes to image, nightly, and all CI.

Symptom & Reproduction

Symptom: Once PR Re-enable ft-short FT e2e tests in CI #1622 enables ft-short, every broad CI trigger still expands to the same unrestricted test-label scope.
Reproduction: Run .venv/bin/python -m pytest -q --confcutdir=tests/ci tests/ci/test/test_run_suite.py -k BroadScopes against the added scope cases.

Root Cause

.github/workflows/pr-test.yml maps every broad trigger to --match-all-labels.
filter_tests cannot exclude scope-specific labels after broad inclusion.

Fix

Add --exclude-labels as a final filter. Image passes ft-short ft-long, nightly passes ft-long, and all passes no exclusions; the workflow applies run-ci-all > nightly > run-ci-image when labels overlap.

Verification

New / updated test: test_image_scope_excludes_all_ft_labels covers image scope.
New / updated test: test_nightly_scope_includes_ft_short_and_excludes_ft_long covers nightly scope.
New / updated test: test_all_scope_includes_every_label covers all scope.
Workflow lint: actionlint accepts all seven updated stage expressions.

Review Focus

Scrutinize the exclusion ordering in filter_tests.
Scrutinize scope precedence in .github/workflows/pr-test.yml.

Remove the disabled= markers from all 11 ft-short FT e2e tests (deterministic / no_failure / with_failure x pp2 / tp2_ep2 / dp4_cp2 + the three real_rollout variants). They were opted out on the 2026-07-08 dev image rebuild pending stabilization; this follow-up PR re-enables them and drives them green via the run-ci-ft-short label.

Two mismatches on the real_rollout per-rollout weight-checksum path, both masked while the FT e2e tests were disabled in CI: 1. Nesting. RolloutManager.check_weights returns the single updatable server's result (server_group -> engine_body); #1329 established this (and test_check_weights_targets_only_updatable_model pins it). But flatten_inference_engine_checksums iterated one extra level, treating dict keys as engine bodies -> AttributeError: 'str' object has no attribute 'get'. Flatten the actual server_group -> engine_body shape. 2. Per-role parallelism_info. sglang's /weights_checker now returns each rank's parallelism_info as a per-role list ([{role: target, ...}, {role: draft, ...}]) for target + draft models, but the merge indexed it as a bare {rank: int} dict -> TypeError: list indices must be integers or slices, not str. Collapse the per-role list to the single shared GPU rank (asserting the roles agree). Update the checksum unit tests and the test_group mock to the real check_weights output and sglang schema, with regression coverage for the multi-role and rank-disagreement cases.

The 06-13 phase_a/phase_b unification made phase_a run the full phase_b workload, roughly doubling these tests' cost; their est_time (600-900s, from the 06-12 measurements) now under-budgets the run so run_suite's per-file timeout (max(1800, est*1.25)) kills them mid dump-comparison. Raising est_time only lifts the timeout ceiling (the test still exits when done).

When MILES_SCRIPT_EXTERNAL_RAY is unset (CI), execute_train boots the ray head with --num-gpus <num_gpus_per_node>, and run_training passed only mode.train_gpus_per_node (the actor GPUs). For the disaggregated real_rollout modes the sglang engines live on separate GPUs (actor 4 + rollout 4 = 8), so ray came up with just 4 GPUs and the 8-GPU placement group could never be scheduled (scheduling_state NO_RESOURCES) -> train.py hung ~30min at ray.get(pg.ready()) until the CI timeout. Non-real-rollout modes were fine because train_gpus_per_node already equals the node total. On rcli MILES_SCRIPT_EXTERNAL_RAY=1 uses an external 8-GPU ray cluster, which masked this. Pass mode.total_node_gpus (train + rollout), matching what scenario_realistic_gsm8k already does for its real-rollout phase.

The comparison scenarios write dense per-param value/grad dumps (+ ckpt, rollout data, events) under /node_public/dumps/<test_name>, which on CI lives on the runner's container-writable layer (not a mounted volume). run_pipeline never removed a test's dump root, so across a shard's tests they accumulated and eventually filled the runner disk -> 'No space left on device' mid-run. Remove the per-test dump root in a finally once its comparison is done (dumps are only needed within the test), so disk is reclaimed between tests.

The disaggregated real_rollout modes put the actor on only train_gpus_per_node GPUs (4) with no tensor/pipeline/expert sharding, so each actor GPU holds the full unsharded 30B model plus the deterministic dumper's value/grad tensors. On an 80GB H100 the true peak fits, but the caching allocator fragments (~11GiB reserved-but-unallocated against a 10.5GiB request) and spuriously OOMs midway through deterministic_dp2_cp2_real_rollout. Set PYTORCH_CUDA_ALLOC_CONF= expandable_segments:True for has_real_rollout modes so those reserved segments can be reused. Safe: no --offload-train and sglang enable_memory_saver is off, so it cannot collide with torch_memory_saver; determinism is unaffected.

deterministic_dp2_cp2_real_rollout runs the same 2-phase deterministic compare as deterministic_dp2_cp2_pp2 (which took ~2008s on H100) plus real sglang generation, but its est_time=1500 capped the per-file timeout at 1875s. Raise it to 3600 (timeout 4500s) and no_failure_dp2_cp2_real_rollout 1200->2400 so real rollout generation on H100 has headroom. est_time only lifts the timeout ceiling and rebalances sharding; it does not extend actual runtime.

Drop --sglang-disable-cuda-graph so the real_rollout tests exercise sglang with cuda graphs enabled, matching real usage.

…-env-vars The H100 actor OOM fix previously set PYTORCH_CUDA_ALLOC_CONF job-wide via the ray runtime env, which also reached the sglang engine. The OOM is purely on the train actor (disaggregated real_rollout puts sglang on separate GPUs), so scope it to the train workers via --train-env-vars instead, matching the existing precedent in tests/e2e/fsdp/test_qwen3_0.6B_megatron_fsdp_align.py. This leaves the sglang engine's allocator untouched. A shared get_train_env_vars_arg helper builds a single --train-env-vars per scenario (merging the deterministic env vars where needed) so there is never a duplicate argument.

Add the stage-c-8-gpu-h200 workflow job and register it as a per-commit CUDA suite. Move every ft-short and ft-long entry to that stage so PR #1622 runs the re-enabled fault-tolerance tests on the H200 fleet, while keeping CI docs and roster tests aligned.

Recalculate 11 stage-c-8-gpu-h200 FT test estimates from PR #1622 passed timings using 25% headroom and bucket rounding so CI partitioning reflects observed runtime.

Exclude all FT tags from run-ci-image and only ft-long from nightly while keeping run-ci-all free of tag exclusions. Add runtime exclusion support and focused coverage for all three scopes.

gemini-code-assist

Code Review

This pull request introduces a mechanism to exclude specific test labels from broad CI scopes (such as run-ci-image and nightly) by adding an --exclude-labels option to the test runner. It updates the corresponding documentation, test suite filtering logic, and unit tests to support and verify these exclusions. The feedback suggests optimizing the label exclusion check in tests/ci/run_suite.py by using any() with a generator expression instead of instantiating a new set for every test in the list comprehension.

Important

The consumer version of Gemini Code Assist on GitHub is being sunset. Starting June 18, 2026, new organization installations will be blocked, and all code review activity will officially cease on July 17, 2026.
For more details on the timeline and next steps, please review the Help Documentation.

yushengsu-thu

Traced the new scope expression through every trigger/label combination (schedule, workflow_dispatch, and PRs carrying nightly / run-ci-image / run-ci-all in all overlaps) — the claimed precedence run-ci-all > nightly > run-ci-image holds, and the Python side is safe (labels=None is normalized to [] by ci_register, so the unconditional exclusion filter can't crash). Docs and tests are updated consistently.

Approving — nothing blocking. Three non-blocking issues inline: (1) an exclusion silently overrides an explicitly requested run-ci-ft-short label, (2) --exclude-labels values aren't validated against KNOWN_LABELS so a YAML typo fails silently, (3) the 7x-duplicated scope expression is a maintenance trap and its precedence logic is only covered by actionlint, not by the unit tests.

yushengsu-thu · 2026-07-12T19:51:44Z

      intersect the PR-supplied set.
+    * `exclude_labels`: after either inclusion mode, remove tests carrying any
+      excluded label. This lets broad scopes omit a specific expensive class;
+      an exclusion always wins over inclusion.


"An exclusion always wins over inclusion" has one surprising consequence: a PR carrying run-ci-image and run-ci-ft-short — someone explicitly asking for FT coverage on an image bump — gets ft-short silently stripped (match-all mode discards the --labels set, then the exclusion removes it). Consider exempting labels that were explicitly present in the stripped --labels set from exclusion, or at least printing a warning when an excluded label was also explicitly requested, and documenting the interaction in 01-label.md.

yushengsu-thu · 2026-07-12T19:51:44Z

        ci_tests = [t for t in ci_tests if not t.labels or (set(t.labels) & label_set)]

+    excluded_label_set = exclude_labels or set()
+    ci_tests = [t for t in ci_tests if not (set(t.labels) & excluded_label_set)]


Two small things here:

exclude_labels values are never validated against KNOWN_LABELS. A typo in the workflow YAML (ft_long, ft-lonng) excludes nothing, silently — the failure mode is expensive soak tests quietly running on every image bump (or the reverse). strip_run_ci_prefix already warns on malformed --labels input; a one-line unknown-label check here would match that defensive style.

(nit) Guarding with if excluded_label_set: skips the per-test set() construction on the majority of runs that have no exclusions, and — worth a docstring sentence either way — the filter also removes matching disabled tests from skipped_tests, so excluded-and-disabled tests vanish from the skip report entirely.

Also: the three new tests all use match_all_labels=True; the docstring promises the exclusion applies "after either inclusion mode", but the match_all_labels=False + exclusion path is untested.

yushengsu-thu · 2026-07-12T19:51:44Z

        --auto-partition-id ${{ matrix.partition_id }} --auto-partition-size 4
        --labels ${{ join(github.event.pull_request.labels.*.name, ' ') }}
        ${{ (github.event_name == 'workflow_dispatch' || contains(github.event.pull_request.labels.*.name, 'run-ci-image') || contains(github.event.pull_request.labels.*.name, 'run-ci-all') || (github.event_name == 'schedule' || contains(github.event.pull_request.labels.*.name, 'nightly'))) && '--match-all-labels' || '' }}
+        ${{ ((github.event_name == 'schedule' || contains(github.event.pull_request.labels.*.name, 'nightly')) && !contains(github.event.pull_request.labels.*.name, 'run-ci-all') && '--exclude-labels ft-long') || (contains(github.event.pull_request.labels.*.name, 'run-ci-image') && !contains(github.event.pull_request.labels.*.name, 'run-ci-all') && github.event_name != 'schedule' && !contains(github.event.pull_request.labels.*.name, 'nightly') && '--exclude-labels ft-short ft-long') || '' }}


This ~470-char expression is now copy-pasted 7 times — this PR is itself evidence of the cost (7 near-identical line edits), and a one-stage drift would be invisible until a nightly quietly runs the wrong test set. The precedence logic here is also the riskiest artifact of the PR, and it's only covered by actionlint (syntax, not behavior) — the new unit tests verify filter_tests given already-correct flags, not that the workflow emits those flags per trigger.

Suggested follow-up: compute a scope-flags output once in a preliminary job (resolve-ci-image is a natural home) and reference it per stage — or better, pass the raw event name + labels into run_suite.py and compute the scope in Python where it's unit-testable.

(nit: clause 2's github.event_name != 'schedule' && !contains(..., 'nightly') guards are unreachable — any case where they'd matter is already consumed by clause 1 or killed by !contains(run-ci-all) — so they could be dropped to lighten the line.)

The 7-line block above the PYTORCH_CUDA_ALLOC_CONF assignment narrated the OOM-debugging history (fragmentation numbers, torch_memory_saver interaction) rather than a constraint the code needs; the setting is conventional enough to stand on its own. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>

fzyzcjy and others added 14 commits July 10, 2026 11:59

Do not disable sglang cuda graph in FT real_rollout tests

731dc5a

Drop --sglang-disable-cuda-graph so the real_rollout tests exercise sglang with cuda graphs enabled, matching real usage.

Merge branch 'main' into ft/reenable-ft-short

27d9811

Merge branch 'main' into ft/reenable-ft-short

ae5aaec

tune FT CI estimates from passed runtime data

62b211b

Recalculate 11 stage-c-8-gpu-h200 FT test estimates from PR #1622 passed timings using 25% headroom and bucket rounding so CI partitioning reflects observed runtime.

fix(ci): separate image and nightly FT tag scopes

5a350bc

Exclude all FT tags from run-ci-image and only ft-long from nightly while keeping run-ci-all free of tag exclusions. Add runtime exclusion support and focused coverage for all three scopes.

guapisolo requested review from yueming-yuan and yushengsu-thu as code owners July 11, 2026 06:45

gemini-code-assist Bot reviewed Jul 11, 2026

View reviewed changes

Comment thread tests/ci/run_suite.py

guapisolo added the nightly label Jul 11, 2026

yushengsu-thu approved these changes Jul 12, 2026

View reviewed changes

Base automatically changed from ft/reenable-ft-short to main July 12, 2026 22:39

guapisolo requested review from Shi-Dong, Zhichenzzz, fzyzcjy, jybsuper and maocheng23 as code owners July 12, 2026 22:39

guapisolo and others added 2 commits July 12, 2026 15:39

Merge branch 'main' into fix/ci-tag-scopes

9121497

guapisolo merged commit fecd5d9 into main Jul 12, 2026
16 checks passed

guapisolo deleted the fix/ci-tag-scopes branch July 12, 2026 22:44

guapisolo mentioned this pull request Jul 12, 2026

refactor(ci): resolve broad CI scopes in run_suite.py, not per-stage YAML #1644

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Uh oh!

fix(ci): separate image and nightly FT tag scopes#1632

fix(ci): separate image and nightly FT tag scopes#1632
guapisolo merged 16 commits into
mainfrom
fix/ci-tag-scopes

guapisolo commented Jul 11, 2026

Uh oh!

gemini-code-assist Bot left a comment

Uh oh!

Uh oh!

yushengsu-thu left a comment

Uh oh!

yushengsu-thu Jul 12, 2026

Uh oh!

yushengsu-thu Jul 12, 2026

Uh oh!

yushengsu-thu Jul 12, 2026

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

Uh oh!

Conversation

guapisolo commented Jul 11, 2026

Summary

Symptom & Reproduction

Root Cause

Fix

Verification

Review Focus

Uh oh!

gemini-code-assist Bot left a comment

Choose a reason for hiding this comment

Code Review

Uh oh!

Uh oh!

yushengsu-thu left a comment

Choose a reason for hiding this comment

Uh oh!

yushengsu-thu Jul 12, 2026

Choose a reason for hiding this comment

Uh oh!

yushengsu-thu Jul 12, 2026

Choose a reason for hiding this comment

Uh oh!

yushengsu-thu Jul 12, 2026

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants