Skip to content

Re-enable ft-short FT e2e tests in CI#1622

Open
fzyzcjy wants to merge 14 commits into
mainfrom
ft/reenable-ft-short
Open

Re-enable ft-short FT e2e tests in CI#1622
fzyzcjy wants to merge 14 commits into
mainfrom
ft/reenable-ft-short

Conversation

@fzyzcjy

@fzyzcjy fzyzcjy commented Jul 10, 2026

Copy link
Copy Markdown
Collaborator

Re-enable the 11 ft-short FT e2e tests (deterministic / no_failure / with_failure × {pp2, tp2_ep2, dp4_cp2} + the three real_rollout variants). They were opted out on the 2026-07-08 :dev image because the FT dependency PRs had not yet merged; those are now merged, and the GPU CI stages reinstall sglang/megatron fresh at test time (skip_dependency_install=false), so the opt-out is obsolete. Add run-ci-ft-short to run them; ft-long (soak) stays disabled.

What this fixes

Reproduced on the faithful CI env (:dev image + sglang-miles + miles-main reinstalled fresh, via rcli / #1461 py-spy dumper on the H100 CI itself):

  1. Weight-checksum consumer (two latent bugs the disabled tests masked, both on the real_rollout per-rollout checksum path):

    • Nesting: RolloutManager.check_weights returns the single updatable server's result (server_group -> engine_body; pinned by test_check_weights_targets_only_updatable_model), but flatten_inference_engine_checksums iterated one level too deep → AttributeError: 'str' object has no attribute 'get'. Flatten the actual shape.
    • Per-role parallelism_info: sglang's /weights_checker now returns each rank's parallelism_info as a per-role list (target + draft) → TypeError: list indices must be integers. Collapse to the single shared GPU rank. Unit tests + test_group mock updated to the real schema, with regression coverage.
  2. est_time for the 8 non-real-rollout tests → 2400s. The 06-13 phase_a/phase_b unification roughly doubled their cost; the old est_time (from 06-12 measurements) under-budgeted them so run_suite's max(1800, est*1.25) per-file timeout killed them mid dump-comparison. Raising est_time only lifts the timeout ceiling.

  3. Start the FT-test ray cluster with actor + rollout GPUs (the real_rollout "hang"). 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). The disaggregated real_rollout modes put the sglang engines on separate GPUs (actor 4 + rollout 4 = 8), so ray came up with just 4 GPUs and the 8-GPU placement group could never schedule (scheduling_state: NO_RESOURCES) → train.py hung ~30 min at ray.get(pg.ready()) until the timeout. Diagnosed on the H100 CI via ray's cluster_resources() / placement_group_table() (the machine has 8 physical GPUs; ray was just started with 4). Fix: pass mode.total_node_gpus (train + rollout), matching what scenario_realistic_gsm8k already does. rcli passed because MILES_SCRIPT_EXTERNAL_RAY=1 there uses an external 8-GPU cluster.

  4. Clean up FT e2e dumps after each test. The comparison scenarios write dense per-param value/grad dumps under /node_public/dumps/<test>, which on CI is the runner's container-writable layer; run_pipeline never removed a test's dump root, so across a shard's tests they accumulated and filled the runner disk (No space left on device). Now the per-test dump root is removed in a finally once its comparison is done.

  5. expandable_segments for the real_rollout train actor (H100 OOM). 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, and dp2 doubles the per-dp-group batch vs the 8-GPU dp4_cp2. On an 80 GB H100 the true peak fits (~71 GiB) but the caching allocator fragments (~11 GiB reserved-but-unallocated against a 10.5 GiB request on the MegatronTrainRayActor) and spuriously OOMs mid-run — surfacing downstream as a detokenizer health-check timeout and "all cells failed". Set PYTORCH_CUDA_ALLOC_CONF=expandable_segments:True for has_real_rollout modes, scoped to the train actor via --train-env-vars (matching the precedent in tests/e2e/fsdp/test_qwen3_0.6B_megatron_fsdp_align.py), leaving the sglang engine's allocator untouched. Safe: the OOM is purely on the actor (sglang runs on separate GPUs), no --offload-train, so it cannot collide with torch_memory_saver; determinism is unaffected. (H200 never OOMed thanks to its larger memory, which is why rcli always passed.)

  6. Do not disable sglang cuda graph in real_rollout. Dropped --sglang-disable-cuda-graph so the real_rollout tests exercise the sglang engine with cuda graphs enabled, matching real usage.

  7. est_time for the 30B real_rollout tests. deterministic_dp2_cp2_real_rollout runs the same 2-phase deterministic compare as deterministic_dp2_cp2_pp2 (~2008 s on H100) plus real sglang generation, but est_time=1500 capped the per-file timeout at 1875 s. Raise it to 3600 (timeout 4500 s) and no_failure_dp2_cp2_real_rollout 1200→2400 so real rollout generation has headroom; the dense 0.6B real_rollout (passed at 1176 s) is unchanged.

Validation

On rcli (H200, faithful deps) all 11 ft-short tests pass end-to-end. On H100 CI the last full run was 10/11: one shard 5/5 green (incl. with_failure_..._real_rollout_dense), the other 4/6 with the only genuine failure being the deterministic_dp2_cp2_real_rollout actor OOM described in fix 5 (the 6th test never ran because run_suite stops at the first failure on regular PRs). Fixes 5 and 6 target exactly that OOM and the real_rollout timeout budget. The OOM only reproduces on 80 GB H100 (H200 has the headroom), so those two fixes are validated by the memory arithmetic + existing miles-wide expandable_segments precedent rather than a local repro.

ci-sglang-pr: sglang-miles
ci-megatron-pr: miles-main

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.

@gemini-code-assist gemini-code-assist Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Code Review

This pull request re-enables several fault-tolerance end-to-end tests by removing the "disabled" parameter. It also updates the weight checking logic in rollout_manager.py to return a list, and refactors checksum utility functions to handle multi-role parallelism information (e.g., target and draft roles) sharing a single GPU rank. Feedback was provided on checksum_utils.py to ensure backward compatibility by checking if "parallelism_info" is a dictionary before treating it as a list of dictionaries.

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.

Comment on lines +37 to +41
def _gpu_rank(rank_info: dict[str, Any]) -> int:
parallelism_info = rank_info["parallelism_info"]
gpu_ranks = {role_info["rank"] for role_info in parallelism_info}
assert len(gpu_ranks) == 1, f"expected one GPU rank across roles, got {gpu_ranks}: {rank_info!r}"
return next(iter(gpu_ranks))

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

medium

To ensure backward compatibility and robust handling of different parallelism_info formats (e.g., if older versions or other components return a single dictionary instead of a list of dictionaries), we should check if parallelism_info is a dictionary before attempting to iterate over it as a list. This prevents a TypeError when iterating over a dictionary's keys.

Suggested change
def _gpu_rank(rank_info: dict[str, Any]) -> int:
parallelism_info = rank_info["parallelism_info"]
gpu_ranks = {role_info["rank"] for role_info in parallelism_info}
assert len(gpu_ranks) == 1, f"expected one GPU rank across roles, got {gpu_ranks}: {rank_info!r}"
return next(iter(gpu_ranks))
def _gpu_rank(rank_info: dict[str, Any]) -> int:
parallelism_info = rank_info["parallelism_info"]
if isinstance(parallelism_info, dict):
return parallelism_info["rank"]
gpu_ranks = {role_info["rank"] for role_info in parallelism_info}
assert len(gpu_ranks) == 1, f"expected one GPU rank across roles, got {gpu_ranks}: {rank_info!r}"
return next(iter(gpu_ranks))

@fzyzcjy fzyzcjy force-pushed the ft/reenable-ft-short branch from f2b8bc2 to 77d7a67 Compare July 10, 2026 06:27
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.
@fzyzcjy fzyzcjy force-pushed the ft/reenable-ft-short branch 2 times, most recently from 00af764 to ab529f8 Compare July 10, 2026 10:17
fzyzcjy added 2 commits July 10, 2026 18:37
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.
@fzyzcjy fzyzcjy force-pushed the ft/reenable-ft-short branch from 8a4c945 to 068bc69 Compare July 10, 2026 10:38
fzyzcjy and others added 6 commits July 11, 2026 00:17
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.
guapisolo and others added 3 commits July 10, 2026 16:48
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.
@fzyzcjy

fzyzcjy commented Jul 11, 2026

Copy link
Copy Markdown
Collaborator Author

(rm label to avoid the new push triggering expensive ci)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants