[Train] Micro-batch scheduling on rollout side + variable global batch size (port slime #1926+#1930+#1933) - #119
[Train] Micro-batch scheduling on rollout side + variable global batch size (port slime #1926+#1930+#1933)#119aoshen02 wants to merge 1 commit into
Conversation
There was a problem hiding this comment.
Code Review
This pull request introduces a robust per-rollout token-weighted mean reduction mechanism for context parallel (CP) training, allowing sibling samples from a single rollout to be correctly aggregated even when split across micro-batches. It refactors the data-parallel scheduling logic into a pure-Python module (dp_schedule.py) supporting Virtual Pipeline Parallelism (VPP) and dynamic batching, accompanied by extensive unit and distributed tests. The review feedback highlights two important improvements: first, correcting the destination rank in dist.gather_object to use group-relative rank 0 instead of the global dp_src_rank to prevent crashes in multi-GPU setups; second, strengthening the step-size assertion in the DP scheduler to require at least align_to samples when VPP is enabled, avoiding downstream scheduling failures.
| if dist.get_rank() == dp_src_rank: | ||
| gathered = [None] * dp_size | ||
| dist.gather_object(log_dict, gathered, dst=dp_src_rank, group=dp_group) | ||
| reduced: dict = {} | ||
| for key in log_dict: | ||
| values = [d[key] for d in gathered] | ||
| first = values[0] | ||
| if isinstance(first, tuple) and len(first) == 2: | ||
| total_sum = sum(v[0] for v in values) | ||
| total_count = sum(v[1] for v in values) | ||
| reduced[key] = total_sum / total_count if total_count else 0.0 | ||
| else: | ||
| reduced[key] = sum(values) / dp_size | ||
| return reduced | ||
| dist.gather_object(log_dict, None, dst=dp_src_rank, group=dp_group) | ||
| return None |
There was a problem hiding this comment.
In PyTorch distributed collective APIs (such as dist.gather_object), the dst parameter specifies the destination rank within the process group (dp_group), not the global rank.
Using the global rank dp_src_rank as dst will cause incorrect behavior or crashes (e.g., ValueError: Invalid destination rank) in multi-GPU/multi-node configurations where the DP source rank is non-zero or exceeds the size of the DP subgroup. Since the source rank is always the first rank in the DP group, its group-relative rank is always 0.
Please change dst=dp_src_rank to dst=0 to ensure correct group-relative routing.
| if dist.get_rank() == dp_src_rank: | |
| gathered = [None] * dp_size | |
| dist.gather_object(log_dict, gathered, dst=dp_src_rank, group=dp_group) | |
| reduced: dict = {} | |
| for key in log_dict: | |
| values = [d[key] for d in gathered] | |
| first = values[0] | |
| if isinstance(first, tuple) and len(first) == 2: | |
| total_sum = sum(v[0] for v in values) | |
| total_count = sum(v[1] for v in values) | |
| reduced[key] = total_sum / total_count if total_count else 0.0 | |
| else: | |
| reduced[key] = sum(values) / dp_size | |
| return reduced | |
| dist.gather_object(log_dict, None, dst=dp_src_rank, group=dp_group) | |
| return None | |
| if dist.get_rank() == dp_src_rank: | |
| gathered = [None] * dp_size | |
| dist.gather_object(log_dict, gathered, dst=0, group=dp_group) | |
| reduced: dict = {} | |
| for key in log_dict: | |
| values = [d[key] for d in gathered] | |
| first = values[0] | |
| if isinstance(first, tuple) and len(first) == 2: | |
| total_sum = sum(v[0] for v in values) | |
| total_count = sum(v[1] for v in values) | |
| reduced[key] = total_sum / total_count if total_count else 0.0 | |
| else: | |
| reduced[key] = sum(values) / dp_size | |
| return reduced | |
| dist.gather_object(log_dict, None, dst=0, group=dp_group) | |
| return None |
There was a problem hiding this comment.
This is a false positive — keeping as-is.
For dist.gather_object, dst is the global rank (PyTorch docstring: "Destination rank on global process group (regardless of group argument)"); the group-relative parameter is group_dst. The guard here is if dist.get_rank() == dp_src_rank, and dist.get_rank() returns the global rank — so dst=dp_src_rank is in the same (global) coordinate system and is correct.
Switching to dst=0 would actually introduce the bug you describe: on multi-node, the DP group's first rank is a non-zero global rank, so gathering to global rank 0 while the receiving branch is dp_src_rank would mismatch. This also matches slime upstream verbatim. No change.
| assert len(sample_indices) >= dp_size, ( | ||
| f"step {step_i}: {len(sample_indices)} samples < dp_size {dp_size}; " | ||
| f"each step needs at least one sample per rank." | ||
| ) |
There was a problem hiding this comment.
The assertion currently checks if len(sample_indices) >= dp_size. However, if Virtual Pipeline Parallelism (VPP) is enabled (vpp_size > 1), the schedule requires the number of microbatches to be a multiple of align_to = dp_size * mb_group.
Since each microbatch must contain at least one sample, the step must have at least align_to samples. If len(sample_indices) is between dp_size and align_to - 1, this assertion passes, but the scheduling downstream will fail with a confusing assertion error about maximal splitting or static path alignment.
Updating the assertion to check len(sample_indices) >= align_to provides a much clearer error message and prevents downstream failures.
| assert len(sample_indices) >= dp_size, ( | |
| f"step {step_i}: {len(sample_indices)} samples < dp_size {dp_size}; " | |
| f"each step needs at least one sample per rank." | |
| ) | |
| assert len(sample_indices) >= align_to, ( | |
| f"step {step_i}: {len(sample_indices)} samples < align_to {align_to}; " | |
| f"each step needs at least {align_to} samples to satisfy DP and VPP constraints." | |
| ) |
There was a problem hiding this comment.
Valid but minor — keeping >= dp_size for now. The downstream alignment assert (target_K/align_to) already rejects the VPP-with-too-few-samples case, just later in the call. Holding this byte-aligned with slime upstream to ease future syncs; noted as a follow-up if we want an earlier/clearer error message.
|
cpu CI — all green (fresh
The five tests the chain ships (dp_schedule scheduling, CP-utils, CP loss-invariance, metric reporting incl. distributed) exercise the core of the refactor on CPU. |
… batch size (port slime #1926+#1930+#1933) Ports the coupled THUDM/slime chain (one feature line, three sequential PRs) into vime as a single change since they touch the same files interdependently: - #1926: move the per-step micro-batch schedule from the actor (train) side to `_split_train_data_by_dp` on the rollout side, computed once over the full global batch. New `vime/utils/dp_schedule.py` (`build_dp_schedule`, `compute_dynamic_global_batch_size`) holds the Ray/engine-free logic so it is unit-testable. Drops the DP-wide `all_reduce(num_microbatches, MAX)`; `get_data_iterator` becomes a thin consumer reading `num_microbatches` / `micro_batch_indices` straight out of `rollout_data`. - #1930 + #1933 [1/N,2/N]: variable global batch size — per-step split by rollout id so each rollout yields a fixed training-step count regardless of how many samples it produced; CP-aware loss/metric handling. vime reconciliation notes: - vime already carried `--use-dynamic-global-batch-size` / `--disable-rollout-trim-samples` (args + inline `_compute_dynamic_global_batch_size`) from an earlier partial sync; #1926 removes that inline method in favor of `dp_schedule.compute_dynamic_global_batch_size`, applied cleanly. - `_split_train_data_by_dp` keeps vime's diverged extra keys (`rollout_routed_experts`, `teacher_log_probs`, `dynamic_global_batch_size`). - `train_parallel_config` is extended with `cp_size` / `vpp_size` / `microbatch_group_size_per_vp_stage` (set in actor.py) and read by `build_dp_schedule`. - import-block hunks hand-reconciled (vime's vllm seam, no SGLangEngine import). Adds the chain's CPU tests (registered in the always-on cpu job): test_dp_schedule, test_cp_utils, test_loss_cp_invariance, test_metric_report, test_metric_report_dist (+ tests/_cp_dist_helpers.py). NOTE: this changes training micro-batch counts / bin-packing (numerics) — needs GPU validation before merge; cpu validation here covers the shipped CPU tests. Signed-off-by: aoshen02 <aoshen@inferact.ai>
87269fc to
de09f3b
Compare
|
Closing for now (slime→vime sync being restructured). Branch is preserved — can reopen anytime. |
…#1940) Port of THUDM/slime#1939 (add more cpu ci) + #1940 (run cpu test on main): - tests/test_rm_{deepscaler,f1,gpqa,math,math_dapo}.py: pure-cpu unit tests for vime/rollout/rm_hub reward fns (107 tests, validated locally). - vime/utils/misc.py: lazy 'import ray' so cpu-only paths (rm_hub scoring, plugin contracts) don't require ray. - pr-test.yml.j2: register the 5 rm tests in the cpu (num_gpus:0) matrix; enable push-to-main trigger gated to cpu jobs only (GPU/self-hosted jobs stay PR/label-gated so push never burns the fleet). slime #1939 also added tests/test_sample.py, which references Sample.rollout_id (introduced by the #1926-chain / vime PR #119) — that test is stacked on #119 instead of here. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> Signed-off-by: aoshen02 <aoshen@inferact.ai>
…80/#1967/#1938/#1988) (#138) * [CI] add reward-fn cpu tests + cpu-on-main trigger (port slime #1939+#1940) Port of THUDM/slime#1939 (add more cpu ci) + #1940 (run cpu test on main): - tests/test_rm_{deepscaler,f1,gpqa,math,math_dapo}.py: pure-cpu unit tests for vime/rollout/rm_hub reward fns (107 tests, validated locally). - vime/utils/misc.py: lazy 'import ray' so cpu-only paths (rm_hub scoring, plugin contracts) don't require ray. - pr-test.yml.j2: register the 5 rm tests in the cpu (num_gpus:0) matrix; enable push-to-main trigger gated to cpu jobs only (GPU/self-hosted jobs stay PR/label-gated so push never burns the fleet). slime #1939 also added tests/test_sample.py, which references Sample.rollout_id (introduced by the #1926-chain / vime PR #119) — that test is stacked on #119 instead of here. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> Signed-off-by: aoshen02 <aoshen@inferact.ai> * [Doc] sync customization/trace/pd-disagg docs (port slime #1942) Port of THUDM/slime#1942 (update docs). Brought the technical doc updates (customization.md agentic-workflows section, trace.md, pd-disaggregation.md, en+zh), rebranded slime->vime / sglang_rollout->vllm_rollout / SGLang->vLLM. Divergence handling: - README.md / README_zh.md: kept vime's version (slime's changes were SGLang-branded architecture text + 'Projects built upon slime' marketing + an agentic-examples list — vime uses vLLM and trimmed examples in #126). - Dropped the broken examples/search-r1 link from the added customization text (trimmed in vime); kept examples/multi_agent + examples/fully_async (exist). Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> Signed-off-by: aoshen02 <aoshen@inferact.ai> * [Chore] remove redundant replay tool + harden unwrap_model import (port slime #1943) Port of THUDM/slime#1943 (remove redundant file): - remove tools/replay_openai_jsonl.py (redundant). - vime/backends/megatron_utils/model.py: make unwrap_model import resilient to the Megatron move from megatron.core.utils to megatron.core.pipeline_parallel.utils (try/except fallback). Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> Signed-off-by: aoshen02 <aoshen@inferact.ai> * [Sync][C] FLOPs MLA fix, PYTHONUNBUFFERED typo, spec-config guard, fault-tolerance docs Mechanical / docs sweep for the slime→vime sync (mega-PR C additions, on top of the cherry-picked #1939+#1940 / #1942 / #1943): - slime #1980: fix FLOPs accounting for non-MLA attention. Gate the MLA-shaped qkv/attention flop formulas on `multi_latent_attention` instead of inferring MLA from q_lora_rank/qk_pos_emb_head_dim/v_head_dim (which misattributed flops for non-MLA models). vime/utils/flops_utils.py. - slime #1967: fix PYTHONBUFFERED=16 typo -> PYTHONUNBUFFERED=1 across 12 scripts + command_utils.py (both name and value, matching slime canonical). - slime #1938: guard `vllm_speculative_config` read in _compute_spec_metrics with getattr(...) so --debug-train-only (skip_vllm leaves the attr unset) doesn't AttributeError. vime/ray/rollout.py. - slime #1988: rewrite docs/en/advanced/fault-tolerance.md (scope, health checks, debug/replay path, production pattern), rebranded sglang→vllm / slime→vime, /health_generate→/health, link vllm-config.md. Dropped from this PR after source review: - #1987/#1990 (R3 ci ref/logprob+kl guards) — already in main via vime #93 (b1009dd "Backports slime #1987 + #1990"). - #1974/#1989 — touch examples/coding_agent_rl + its README bullet; that feature is not in vime yet (lands with the agent mega-PR). - #1975 — sglang conda-ci image resolution + a test vime already has + build_conda micromamba fix; no real version-file port for vime. Deferred to after the rollout-data-model PR: #1986/#1985 (they edit that PR's tests). Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> Signed-off-by: aoshen02 <aoshen@inferact.ai> * docs(mega-C): faithfully port slime #1942 docs (fault-tolerance/pd-disagg/trace) Audit (一模一样): mega-C's original #1942 doc port was incomplete — several docs were stubs or truncated vs slime. Port them faithfully (modulo sglang→vllm): - fault-tolerance.md en: add trailing (trainer-failure note + Related Docs). - fault-tolerance.md zh: was a 13-line stub → full 76-line port. - pd-disaggregation.md en+zh: were 7-line stubs → full 87-line port. - trace.md en+zh: port slime's build_meta example as build_vllm_meta_trace_attrs(output) (vime's real signature). Translation: SGLang→vLLM, /health_generate→/health (vime's real endpoint), sglang-config→vllm-config, `sglang:`→`vllm:` YAML key (vime vllm_config.py requires the 'vllm' key), server→engine; `--prefill-num-servers` flag preserved. All 6 files now match slime line counts (76/76, 87/87, 119/119). Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> * docs(mega-C): port slime #1942 customization gaps (multi-sample return + teacher_log_probs) Surgically add the two slime #1942 additions that mega-C missed, keeping vime's legitimate divergences (vllm_rollout path, VIME_CONTRACT_* env, search-r1 removed in #74): - "#### Returning multiple training samples for one prompt" section (en+zh): custom_generate may return list[Sample] with shared group_id; signature updated to `-> Sample | list[Sample]`. - `teacher_log_probs` field in the Sample-fields dict (en+zh). Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> * [Sync][C] align to slime exactly: drop #1961 over-port, fix #1938/#1943 placement - customization.md (en+zh): revert custom_generate signature to `-> Sample` and remove the "Returning multiple training samples" section. That content is from slime #1961 (not #1942, despite the prior commit message) and documents Sample.group_id / Sample.rollout_id + list[Sample] return — all tied to the deferred rollout-data-model refactor that vime does not yet implement (verified: 0 occurrences of group_id/rollout_id in types.py and the rollout loop). Defers with #1986/#1985. - model.py: move the unwrap_model try/except to slime #1943's exact position (after the tqdm import) — import block now byte-identical to slime. - rollout.py: drop the 4-line comment around the spec-metrics getattr guard; slime #1938 is a pure one-line change with no added comment. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> * [Sync][C] cleanup sglang/dead-link residue: /health_generate help text + search-r1 refs - arguments.py: --rollout-health-check-{interval,timeout} help text said `/health_generate` (sglang's endpoint) but the code hits `/health` (vllm_engine.py). Fix the help strings to `/health`. - customization.md (en+zh) + index.rst (en+zh): remove the dead `examples/search-r1` references — that example does not exist in vime. Note: the method name `VLLMEngine.health_generate()` (vllm_engine.py) and its caller (health_monitor.py) keep the sglang-flavored name but already hit `/health`; renaming the method is a separate change, left out here. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> * [Sync][C] remove dead examples/retool toctree refs examples/retool does not exist in vime (same dead-link situation as search-r1); drop it from the docs toctree in index.rst (en+zh). Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> * [Sync][C] docs: add Ray Distributed Debugger step-by-step section (port slime #2001) Appends the 'Step-by-Step Debugging with Ray Distributed Debugger' section (debugpy + RAY_DEBUG_POSTMORTEM + VS Code attach) to developer_guide/debug.md (en + zh). Content is engine-neutral (Ray/debugpy/VS Code), ported verbatim; no sglang->vllm translation needed. --------- Signed-off-by: aoshen02 <aoshen@inferact.ai> Co-authored-by: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Ports the coupled THUDM/slime chain #1926 → #1930 → #1933 (one variable-global-batch feature line, three sequential PRs) into vime as one PR — they touch the same files interdependently (
dp_schedule.py,actor.py,data.py,model.py,loss.py,ray/rollout.py), so splitting would mean reconciling the same diverged files three times and a merge-order hazard. One PR = one CI run.What
_split_train_data_by_dpon the rollout side, computed once over the full global batch. Newvime/utils/dp_schedule.py(build_dp_schedule,compute_dynamic_global_batch_size) holds the Ray/engine-free logic so it's unit-testable. Drops the DP-wideall_reduce(num_microbatches, MAX);get_data_iteratorbecomes a thin consumer readingnum_microbatches/micro_batch_indicesout ofrollout_data.vime reconciliation
--use-dynamic-global-batch-size/--disable-rollout-trim-samples(args + an inline_compute_dynamic_global_batch_size) from an earlier partial sync; #1926 removes that inline method in favor ofdp_schedule.compute_dynamic_global_batch_size— applied cleanly._split_train_data_by_dpkeeps vime's diverged extra keys (rollout_routed_experts,teacher_log_probs,dynamic_global_batch_size).train_parallel_configextended withcp_size/vpp_size/microbatch_group_size_per_vp_stage(set inactor.py:92, read bybuild_dp_schedule).SGLangEngineimport).slime/→vime/,slime.→vime.);/root/slimemount andSLIME_TEST_*envs preserved.Tests / CI
Adds the chain's CPU tests to the always-on cpu job:
test_dp_schedule,test_cp_utils,test_loss_cp_invariance,test_metric_report,test_metric_report_dist(+tests/_cp_dist_helpers.py). j2 updated + yml re-rendered.Validation
Upstream: THUDM/slime#1926, #1930, #1933. Part of the slime→vime sync tracked in #107 (chain registered there; not ticking boxes pending sign-off).