Conversation
There was a problem hiding this comment.
Code Review
This pull request introduces a new det_nccl backend to enable bitwise-deterministic SUM and AVG reductions using a fixed-order fold over all-gather operations, along with integration into Megatron initialization, Ray train actors, and comprehensive tests. The review feedback suggests several key improvements: optimizing the pairwise tree reduction by using in-place addition (.add_) to reduce memory overhead, refining the process group type check to prevent native PyTorch groups from bypassing the efficient _allgather_base path, and using getattr when accessing command-line arguments to avoid potential AttributeError exceptions when using mock configuration objects.
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.
| while len(partials) > 1: | ||
| partials = [partials[i] + partials[i + 1] for i in range(0, len(partials), 2)] |
There was a problem hiding this comment.
The current implementation of the pairwise tree reduction for power-of-two world sizes uses out-of-place addition (+), which allocates new tensors at every step of the reduction. For large tensors (such as gradient buffers in MoE models), this can lead to significant temporary memory overhead and potential GPU OOM.
Since the docstring explicitly states that the function may reuse/mutate the gathered buffers as accumulators, we should perform the addition in-place using .add_ to avoid any extra memory allocations.
| while len(partials) > 1: | |
| partials = [partials[i] + partials[i + 1] for i in range(0, len(partials), 2)] | |
| while len(partials) > 1: | |
| partials = [partials[i].add_(partials[i + 1]) for i in range(0, len(partials), 2)] |
| if isinstance(group, dist.ProcessGroup): | ||
| # ProcessGroup wrappers (torchft) inherit ``_allgather_base`` from the C++ base, but it | ||
| # dispatches to a per-device backend they never register; only the overridden list-form | ||
| # ``allgather`` is safe. ``hasattr`` cannot discriminate here. | ||
| rows = list(output.view(group.size(), -1).unbind(dim=0)) | ||
| group.allgather([rows], [input], AllgatherOptions()).wait() | ||
| else: | ||
| group._allgather_base(output, input, AllgatherOptions()).wait() |
There was a problem hiding this comment.
Using isinstance(group, dist.ProcessGroup) to identify wrapper classes (like torchft) is problematic because all native PyTorch process groups (such as ProcessGroupNCCL) also inherit from dist.ProcessGroup. As a result, native process groups will always bypass the highly efficient _allgather_base path and fallback to the slower list-based allgather path in production, introducing unnecessary overhead.
To correctly identify external wrappers while allowing native PyTorch process groups to use _allgather_base, we can check if the group's module does not start with torch.distributed.
| if isinstance(group, dist.ProcessGroup): | |
| # ProcessGroup wrappers (torchft) inherit ``_allgather_base`` from the C++ base, but it | |
| # dispatches to a per-device backend they never register; only the overridden list-form | |
| # ``allgather`` is safe. ``hasattr`` cannot discriminate here. | |
| rows = list(output.view(group.size(), -1).unbind(dim=0)) | |
| group.allgather([rows], [input], AllgatherOptions()).wait() | |
| else: | |
| group._allgather_base(output, input, AllgatherOptions()).wait() | |
| if isinstance(group, dist.ProcessGroup) and not type(group).__module__.startswith("torch.distributed"): | |
| # ProcessGroup wrappers (torchft) inherit ``_allgather_base`` from the C++ base, but it | |
| # dispatches to a per-device backend they never register; only the overridden list-form | |
| # ``allgather`` is safe. ``hasattr`` cannot discriminate here. | |
| rows = list(output.view(group.size(), -1).unbind(dim=0)) | |
| group.allgather([rows], [input], AllgatherOptions()).wait() | |
| else: | |
| group._allgather_base(output, input, AllgatherOptions()).wait() |
1aed265 to
b224cb0
Compare
9554ea9 to
9fb687f
Compare
b224cb0 to
6380283
Compare
9fb687f to
9009b02
Compare
6380283 to
4060739
Compare
9009b02 to
56c010c
Compare
|
|
||
| def test_reduce_op_of_extracts_reduceop_from_options_object(): | ||
| """_reduce_op_of reads .reduceOp from an options object and passes a bare ReduceOp through.""" | ||
| from torch.distributed.distributed_c10d import AllreduceOptions, ReduceScatterOptions |
There was a problem hiding this comment.
Nit: Move this to the beginning?
| instance, or buffer layout, and reduce-scatter takes its shard from the same full | ||
| fold, so reduce-scatter and all-reduce agree bitwise by construction. | ||
|
|
||
| Debug/test use only: the fold trades bandwidth and synchrony for determinism. |
There was a problem hiding this comment.
nit: should we put in debug_utils?
4060739 to
d130d9a
Compare
56c010c to
c5e88d2
Compare
d130d9a to
290d96c
Compare
c5e88d2 to
913fdcb
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.
Add an opt-in deterministic NCCL process-group backend (`--debug-deterministic-collective`) that folds order-sensitive SUM/AVG reductions into a fixed order so training collectives are bit-reproducible, registering it as the training world's distributed backend and requiring synchronous grad sync. - det_process_group.py (+ GPU test, dist test helper). - train_actor.py: register the backend and select it when enabled. - initialize.py: assert synchronous grad reduce under the deterministic backend.
290d96c to
c7798b5
Compare
913fdcb to
d0b41fc
Compare
…ft/dev_revert_reversed/add-a-deterministic-nccl-backend-for-order-stable-collectives
Add an opt-in deterministic NCCL process-group backend (
--debug-deterministic-collective)that folds order-sensitive SUM/AVG reductions into a fixed order so training
collectives are bit-reproducible, registering it as the training world's
distributed backend and requiring synchronous grad sync.