Conversation
There was a problem hiding this comment.
Code Review
This pull request introduces a --delay-split-train-data-by-dp option to delay splitting training data by data parallel size until the training side, rather than doing it immediately during rollout generation. This involves moving the splitting logic to a new utility file miles/utils/data_utils.py and updating the rollout manager, actor group, and data processing pipeline accordingly. Feedback on the changes highlights several issues in the newly added split_train_data_by_dp function, including redundant variable initialization, unsafe in-place mutation of the input dictionary, and a lack of validation for sequence-level and token-level tensors.
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.
| rollout_data = {} | ||
|
|
||
| if "prompt" in data: | ||
| rollout_data["prompt"] = data["prompt"] | ||
|
|
||
| total_lengths = [len(t) for t in data["tokens"]] | ||
| data["total_lengths"] = total_lengths | ||
|
|
||
| if args.balance_data: | ||
| partitions = get_seqlen_balanced_partitions(total_lengths, dp_size, equal_size=True) | ||
| else: | ||
| partitions = [range(i, len(total_lengths), dp_size) for i in range(dp_size)] | ||
|
|
||
| ans = [] | ||
|
|
||
| for i in range(dp_size): | ||
| rollout_data = {} | ||
| partition = partitions[i] | ||
| rollout_data["partition"] = partition | ||
| for key in [ | ||
| "tokens", | ||
| "multimodal_train_inputs", | ||
| "response_lengths", | ||
| "rewards", | ||
| "truncated", | ||
| "loss_masks", | ||
| "round_number", | ||
| "sample_indices", | ||
| "rollout_log_probs", | ||
| "rollout_routed_experts", | ||
| "prompt", | ||
| "teacher_log_probs", | ||
| "seq_witness_ids", | ||
| "weight_versions", | ||
| ]: | ||
| if key not in data: | ||
| continue | ||
| val = [data[key][j] for j in partition] | ||
| rollout_data[key] = val | ||
| # keys that need to be splited at train side | ||
| for key in [ | ||
| "raw_reward", | ||
| "total_lengths", | ||
| "dynamic_global_batch_size", | ||
| ]: | ||
| if key not in data: | ||
| continue | ||
| rollout_data[key] = data[key] | ||
| ans.append(rollout_data) | ||
| return ans |
There was a problem hiding this comment.
There are three main issues in the current implementation of split_train_data_by_dp:
- Dead Code / Redundant Initialization: The initialization of
rollout_dataand the assignment ofrollout_data["prompt"]on lines 8-11 are completely redundant.rollout_datais re-initialized as an empty dictionary inside the loop on line 24, discarding any values set here. Furthermore,"prompt"is already correctly handled and sliced inside the loop (line 38). - In-place Mutation of Input Argument: Mutating the input
datadictionary in-place (data["total_lengths"] = total_lengthson line 14) is a bad practice that can lead to side effects or runtime errors (e.g., if the dictionary or its values are read-only when retrieved from Ray's object store). - Missing Validation for Sequence/Token-Level Tensors: When processing lists of sequence-level or token-level tensors (such as
rollout_log_probsandteacher_log_probs), we should validate that the list lengths match up front and perform per-sample shape checks to prevent silent mismatches or broadcast failures (e.g., GRPO-style scalar-broadcast traps).
We can resolve these issues cleanly by:
- Removing the redundant initialization on lines 8-11.
- Avoiding the in-place mutation of
data. - Directly assigning
total_lengthstorollout_data["total_lengths"]inside the loop. - Adding up-front validation of list lengths and per-sample shape checks for the sequence/token-level tensors.
total_lengths = [len(t) for t in data["tokens"]]
tensor_keys = ["rollout_log_probs", "teacher_log_probs"]
active_tensor_keys = [k for k in tensor_keys if k in data]
if active_tensor_keys:
base_len = len(data["tokens"])
for key in active_tensor_keys:
if len(data[key]) != base_len:
raise ValueError(f"Length mismatch for {key}: expected {base_len}, got {len(data[key])}")
for idx, (t, tok) in enumerate(zip(data[key], data["tokens"])):
if hasattr(t, "shape") and hasattr(tok, "shape"):
if t.shape[0] != tok.shape[0]:
raise ValueError(f"Shape mismatch at index {idx} for {key}: {t.shape} vs {tok.shape}")
if args.balance_data:
partitions = get_seqlen_balanced_partitions(total_lengths, dp_size, equal_size=True)
else:
partitions = [range(i, len(total_lengths), dp_size) for i in range(dp_size)]
ans = []
for i in range(dp_size):
rollout_data = {}
partition = partitions[i]
rollout_data["partition"] = partition
for key in [
"tokens",
"multimodal_train_inputs",
"response_lengths",
"rewards",
"truncated",
"loss_masks",
"round_number",
"sample_indices",
"rollout_log_probs",
"rollout_routed_experts",
"prompt",
"teacher_log_probs",
"seq_witness_ids",
"weight_versions",
]:
if key not in data:
continue
val = [data[key][j] for j in partition]
rollout_data[key] = val
for key in [
"raw_reward",
"dynamic_global_batch_size",
]:
if key not in data:
continue
rollout_data[key] = data[key]
rollout_data["total_lengths"] = total_lengths
ans.append(rollout_data)
return ansReferences
- When processing lists of sequence-level or token-level tensors (such as advantages, student log probabilities, and teacher log probabilities) in RL or distillation pipelines, validate that the list lengths match up front, and perform per-sample shape checks to prevent silent mismatches or broadcast failures (e.g., GRPO-style scalar-broadcast traps).
| ans = [] | ||
|
|
||
| for i in range(dp_size): | ||
| rollout_data = {} |
There was a problem hiding this comment.
Hmm I could be wrong but it seems that the previous rollout_data["prompt"] = data["prompt"] is shadowed by this line?
| rollout_data = ray.get(rollout_data_ref[dp_rank].inner) | ||
| def process_rollout_data( | ||
| args, | ||
| rollout_data_ref, |
There was a problem hiding this comment.
I think it'd be better to add type annotation for rollout_data_ref.
| import numpy as np | ||
| import ray | ||
|
|
||
| from .data_utils import split_train_data_by_dp |
There was a problem hiding this comment.
Let's avoid relative import whenever possible.
50c878d to
8308c92
Compare
1aed265 to
b224cb0
Compare
8308c92 to
6bc4b5d
Compare
b224cb0 to
6380283
Compare
6bc4b5d to
ac429f4
Compare
6380283 to
4060739
Compare
| from miles.utils.seqlen_balancing import get_seqlen_balanced_partitions | ||
|
|
||
|
|
||
| def split_train_data_by_dp(args, data: dict[str, Any], *, dp_size: int) -> list[dict[str, Any]]: |
There was a problem hiding this comment.
this seems to be duplicated with the original split_train_data_by_dp? should we unify
| "round_number", | ||
| "sample_indices", | ||
| "rollout_log_probs", | ||
| "rollout_routed_experts", |
There was a problem hiding this comment.
rollout_indexer_topk is missing
Review comments on #1397: the FT delayed-split copy in miles/utils/data_utils.py had drifted from the rollout-side original - it was missing rollout_indexer_topk and opd_reverse_kl (added on main after the fork), so the delayed-split path silently dropped them. Delete data_utils.py: the split logic lives only in train_data_conversion.py as split_train_data_by_dp_raw (key list is the union; seq_witness_ids is injected only on the delayed path), with split_train_data_by_dp a thin wrapper that ray.puts each partition. Also drop the dead pre-loop rollout_data/prompt block that shadowed itself.
Review comments on #1397: the FT delayed-split copy in miles/utils/data_utils.py had drifted from the rollout-side original - it was missing rollout_indexer_topk and opd_reverse_kl (added on main after the fork), so the delayed-split path silently dropped them. Delete data_utils.py: the split logic lives only in train_data_conversion.py as split_train_data_by_dp_raw (key list is the union; seq_witness_ids is injected only on the delayed path), with split_train_data_by_dp a thin wrapper that ray.puts each partition. Also drop the dead pre-loop rollout_data/prompt block that shadowed itself.
Review comments on #1397: the FT delayed-split copy in miles/utils/data_utils.py had drifted from the rollout-side original - it was missing rollout_indexer_topk and opd_reverse_kl (added on main after the fork), so the delayed-split path silently dropped them. Delete data_utils.py: the split logic lives only in train_data_conversion.py as split_train_data_by_dp_raw (key list is the union; seq_witness_ids is injected only on the delayed path), with split_train_data_by_dp a thin wrapper that ray.puts each partition.
ac429f4 to
410cb55
Compare
4060739 to
d130d9a
Compare
410cb55 to
ac21e10
Compare
d130d9a to
290d96c
Compare
Add a `deterministic_random` reward that hashes the sample tokens + response to produce a stable pseudo-random 0/1 reward, used for reproducible fault-tolerance / CI tests. - rm_hub/__init__.py (+ test).
Add `inplace_modify_args`, a context manager that temporarily overrides args attributes and restores them on exit (asserting they weren't clobbered), used to scope per-attempt argument overrides in the fault-tolerant trainer. - argparse_utils.py (+ test).
Small shared-utility additions used by the fault-tolerant trainer: hash non-contiguous tensors safely (reshape before viewing as bytes), an `enable_experimental_ft_trainer` env flag, forward NCCL_DEBUG/NCCL_DEBUG_FILE to worker environments, and a `filter_keys` helper. - ci_utils.py / environ.py / external_utils/command_utils.py / misc.py.
Thread the original backend through ReloadableProcessGroup so that, when a process group is rebuilt (e.g. after a reconfigure/heal), it is recreated with the same backend instead of hard-coding NCCL. - reloadable_process_group.py: carry `backend` in the reload group info.
Add small foundation utilities used across the fault-tolerance trainer: a strict pydantic base model, a retry helper, a tensor checksum helper, a per-cell megatron world-size computation, the TrainStepOutcome enum, and the IndepDPInfo dataclass describing a cell's independent-DP identity. - pydantic_utils.py / retry_utils.py / checksum_utils.py / megatron_args_utils.py / types.py / indep_dp.py and tests.
Add a `log_structured` helper that emits logfmt-style key/value log lines, used by the fault-tolerance components for greppable structured logs. - structured_log.py (+ test).
Add a small `Clock` interface (`RealClock` plus a controllable fake clock) so time-dependent fault-tolerance code (health checks, heartbeats) can be driven deterministically in tests. - miles/utils/clock.py and tests.
Add a fault-injector test utility used to deterministically exercise fault-tolerance code paths. - miles/utils/test_utils/fault_injector.py.
Add the shared data models for the fault-tolerance control server (e.g. the `TriState` health value), used by the health checker and later by the HTTP control server. - miles/utils/control_server/models.py.
Add the periodic health checker (debounced TriState status driven by a Clock) and heartbeat utilities used to monitor train-cell liveness. - miles/utils/health_checker.py, miles/utils/heartbeat_utils.py and tests.
Add the nvidia-resiliency-ext dependency, the "ft" CI test label, the FT test fixtures in the rollout conftest, and route startup logging through configure_logger_raw. The fault-tolerance CLI arguments themselves now live with the features that consume them (distributed across the per-feature commits).
Unconditionally disconnect-then-reconnect the model-update process group when (re)connecting rollout engines, guarding the destroy against a missing group, so a reconfigured/healed engine set can rebuild the NCCL group from scratch. - broadcast.py: drop the "only disconnect if group exists" short-circuit; guard `destroy_process_group` against None.
Expose an `inject_fault` Ray method on TrainRayActor (in its own concurrency group) that triggers a configured failure mode via the fault injector, so fault-tolerance tests can crash/hang specific actors on demand. - train_actor.py: `inject_fault` RPC.
Extract the DP split into a witness-aware split_train_data_by_dp_raw helper (with unit tests; the key list also carries seq_witness_ids) and use it to split the training data on the actor side when delay_split_train_data_by_dp is set, deferring the DP split from the rollout side to actor-side processing. split_train_data_by_dp stays a thin wrapper that ray.puts each partition. - miles/ray/rollout/train_data_conversion.py (+ tests), miles/utils/data.py, actor_group.py, rollout_manager.py.
ac21e10 to
a838a0b
Compare
290d96c to
c7798b5
Compare
…ft/dev_revert_reversed/delay-splitting-train-data-by-dp-until-actor-side-processing
Extract the DP split into a witness-aware split_train_data_by_dp_raw
helper (with unit tests; the key list also carries seq_witness_ids) and
use it to split the training data on the actor side when
delay_split_train_data_by_dp is set, deferring the DP split from the
rollout side to actor-side processing. split_train_data_by_dp stays a
thin wrapper that ray.puts each partition.