Skip to content

Relocate GroupInfo into shared process-group utilities - #1412

Merged
fzyzcjy merged 30 commits into
mainfrom
tom/pr_chain/trainer_ft/dev_revert_reversed/relocate-groupinfo-into-shared-process-group-utilities
Jul 10, 2026
Merged

Relocate GroupInfo into shared process-group utilities#1412
fzyzcjy merged 30 commits into
mainfrom
tom/pr_chain/trainer_ft/dev_revert_reversed/relocate-groupinfo-into-shared-process-group-utilities

Conversation

@fzyzcjy

@fzyzcjy fzyzcjy commented Jun 22, 2026

Copy link
Copy Markdown
Collaborator

Move the GroupInfo dataclass out of training_utils/parallel.py into a shared
process_group_utils module that also provides multi-process-group helpers
(GeneralPGUtil / MultiPGUtil) for collectives over single or hierarchical
process groups, used by the cross-replica / effective-DP code paths.

  • process_group_utils.py: GroupInfo + GeneralPGUtil / MultiPGUtil (+ tests).
  • training_utils/parallel.py: import GroupInfo from the shared module.
  • megatron / fsdp parallel.py: import GroupInfo from the shared module.
  • distributed_utils.py: use GeneralPGUtil for masked-whiten all-reduce.

@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 refactors process group utilities by moving GroupInfo to a new module miles.utils.process_group_utils and introducing GeneralPGUtil and MultiPGUtil to support both native PyTorch and torchft process groups. The feedback identifies two critical issues: first, _NativePGUtil methods will crash if group is None because dist.get_global_rank does not support None groups, so the source/destination rank should default to 0. Second, collective_bool_and creates a tensor on the CPU by default, which will cause a runtime error when used with an NCCL process group; the tensor should be placed on the appropriate device (e.g., CUDA).

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 +122 to +145
def reduce(self, tensor: torch.Tensor, group: dist.ProcessGroup, op: dist.ReduceOp) -> None:
dist.reduce(tensor, dst=dist.get_global_rank(group, 0), op=op, group=group)

def broadcast(self, tensor: torch.Tensor, group: dist.ProcessGroup) -> None:
dist.broadcast(tensor, src=dist.get_global_rank(group, 0), group=group)

def barrier(self, group: dist.ProcessGroup) -> None:
dist.barrier(group=group)

def all_gather(
self, output_tensors: list[torch.Tensor], input_tensor: torch.Tensor, group: dist.ProcessGroup
) -> None:
dist.all_gather(output_tensors, input_tensor, group=group)

def gather(
self,
input_tensor: torch.Tensor,
gather_list: list[torch.Tensor] | None,
group: dist.ProcessGroup,
) -> None:
dist.gather(input_tensor, gather_list=gather_list, dst=dist.get_global_rank(group, 0), group=group)

def gather_object(self, obj: Any, object_gather_list: list[Any] | None, group: dist.ProcessGroup) -> None:
dist.gather_object(obj, object_gather_list, dst=dist.get_global_rank(group, 0), group=group)

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.

high

In _NativePGUtil, several methods (reduce, broadcast, gather, gather_object) call dist.get_global_rank(group, 0). If group is None (which is a common fallback in parallel states when certain parallelisms like pipeline or expert parallel are disabled), dist.get_global_rank(None, 0) will raise an AttributeError or TypeError and crash.

Since the global rank for group=None (the default WORLD group) is simply the group rank itself, we can safely default the destination/source rank to 0 when group is None.

    def reduce(self, tensor: torch.Tensor, group: dist.ProcessGroup, op: dist.ReduceOp) -> None:
        dst = 0 if group is None else dist.get_global_rank(group, 0)
        dist.reduce(tensor, dst=dst, op=op, group=group)

    def broadcast(self, tensor: torch.Tensor, group: dist.ProcessGroup) -> None:
        src = 0 if group is None else dist.get_global_rank(group, 0)
        dist.broadcast(tensor, src=src, group=group)

    def barrier(self, group: dist.ProcessGroup) -> None:
        dist.barrier(group=group)

    def all_gather(
        self, output_tensors: list[torch.Tensor], input_tensor: torch.Tensor, group: dist.ProcessGroup
    ) -> None:
        dist.all_gather(output_tensors, input_tensor, group=group)

    def gather(
        self,
        input_tensor: torch.Tensor,
        gather_list: list[torch.Tensor] | None,
        group: dist.ProcessGroup,
    ) -> None:
        dst = 0 if group is None else dist.get_global_rank(group, 0)
        dist.gather(input_tensor, gather_list=gather_list, dst=dst, group=group)

    def gather_object(self, obj: Any, object_gather_list: list[Any] | None, group: dist.ProcessGroup) -> None:
        dst = 0 if group is None else dist.get_global_rank(group, 0)
        dist.gather_object(obj, object_gather_list, dst=dst, group=group)

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.

Please apply.

Comment on lines +341 to +345
def collective_bool_and(*, value: bool, group: dist.ProcessGroup) -> bool:
"""Make a bool `and` operation on all ranks in this process group"""
tensor = torch.tensor([1.0 if value else 0.0], dtype=torch.float32)
GeneralPGUtil.create(group).all_reduce(tensor, group, op=dist.ReduceOp.MIN)
return tensor.item() > 0.5

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.

high

In collective_bool_and, the tensor is created on the CPU by default. If the provided group is an NCCL process group (which is standard for GPU training), calling all_reduce on a CPU tensor will result in a runtime crash (e.g., RuntimeError: Tensors must be CUDA tensors).

To prevent this, we should place the tensor on the appropriate device (e.g., using cuda if CUDA is available).

Suggested change
def collective_bool_and(*, value: bool, group: dist.ProcessGroup) -> bool:
"""Make a bool `and` operation on all ranks in this process group"""
tensor = torch.tensor([1.0 if value else 0.0], dtype=torch.float32)
GeneralPGUtil.create(group).all_reduce(tensor, group, op=dist.ReduceOp.MIN)
return tensor.item() > 0.5
def collective_bool_and(*, value: bool, group: dist.ProcessGroup) -> bool:
"""Make a bool `and` operation on all ranks in this process group"""
device = torch.device("cuda" if torch.cuda.is_available() else "cpu")
tensor = torch.tensor([1.0 if value else 0.0], dtype=torch.float32, device=device)
GeneralPGUtil.create(group).all_reduce(tensor, group, op=dist.ReduceOp.MIN)
return tensor.item() > 0.5

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.

Please apply.

@fzyzcjy
fzyzcjy force-pushed the tom/pr_chain/trainer_ft/dev_revert_reversed/add-reconfiguration-assertions-for-fault-tolerance-tests branch from 18589ad to b3d9c10 Compare June 23, 2026 07:47
@fzyzcjy
fzyzcjy force-pushed the tom/pr_chain/trainer_ft/dev_revert_reversed/relocate-groupinfo-into-shared-process-group-utilities branch from 10b39c7 to 99ca854 Compare June 23, 2026 07:47
@fzyzcjy
fzyzcjy force-pushed the tom/pr_chain/trainer_ft/dev_revert_reversed/add-reconfiguration-assertions-for-fault-tolerance-tests branch from b3d9c10 to d363d93 Compare June 23, 2026 09:26
@fzyzcjy
fzyzcjy force-pushed the tom/pr_chain/trainer_ft/dev_revert_reversed/relocate-groupinfo-into-shared-process-group-utilities branch from 99ca854 to 4828875 Compare June 23, 2026 09:26
@fzyzcjy
fzyzcjy force-pushed the tom/pr_chain/trainer_ft/dev_revert_reversed/add-reconfiguration-assertions-for-fault-tolerance-tests branch from d363d93 to 940e7e7 Compare June 23, 2026 13:30
@fzyzcjy
fzyzcjy force-pushed the tom/pr_chain/trainer_ft/dev_revert_reversed/relocate-groupinfo-into-shared-process-group-utilities branch from 4828875 to 58be551 Compare June 23, 2026 13:30
Comment thread miles/utils/process_group_utils.py Outdated
) -> None:
# AllgatherOptions is not re-exported by torch.distributed (unlike
# AllreduceOptions, BroadcastOptions, GatherOptions). PyTorch omission.
from torch._C._distributed_c10d import AllgatherOptions

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.

Please hoist this import to the beginning

Comment thread miles/utils/process_group_utils.py Outdated
else:
assert object_gather_list is None

from torch.distributed.distributed_c10d import _get_object_coll_device

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.

Please hoist this import to the beginning

@Shi-Dong Shi-Dong 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.

Posted a couple of nits, but please also apply Gemini's suggestions as they look valid.

from ...training_utils.parallel import GroupInfo, ParallelState
from miles.utils.process_group_utils import GroupInfo

from ...training_utils.parallel import ParallelState

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.

Please switch relative import to absolute import.

from ..training_utils.parallel import GroupInfo, ParallelState, get_parallel_state
from miles.utils.process_group_utils import GroupInfo

from ..training_utils.parallel import ParallelState, get_parallel_state

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.

Please switch relative import to absolute import.

Comment on lines +122 to +145
def reduce(self, tensor: torch.Tensor, group: dist.ProcessGroup, op: dist.ReduceOp) -> None:
dist.reduce(tensor, dst=dist.get_global_rank(group, 0), op=op, group=group)

def broadcast(self, tensor: torch.Tensor, group: dist.ProcessGroup) -> None:
dist.broadcast(tensor, src=dist.get_global_rank(group, 0), group=group)

def barrier(self, group: dist.ProcessGroup) -> None:
dist.barrier(group=group)

def all_gather(
self, output_tensors: list[torch.Tensor], input_tensor: torch.Tensor, group: dist.ProcessGroup
) -> None:
dist.all_gather(output_tensors, input_tensor, group=group)

def gather(
self,
input_tensor: torch.Tensor,
gather_list: list[torch.Tensor] | None,
group: dist.ProcessGroup,
) -> None:
dist.gather(input_tensor, gather_list=gather_list, dst=dist.get_global_rank(group, 0), group=group)

def gather_object(self, obj: Any, object_gather_list: list[Any] | None, group: dist.ProcessGroup) -> None:
dist.gather_object(obj, object_gather_list, dst=dist.get_global_rank(group, 0), group=group)

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.

Please apply.

Comment on lines +341 to +345
def collective_bool_and(*, value: bool, group: dist.ProcessGroup) -> bool:
"""Make a bool `and` operation on all ranks in this process group"""
tensor = torch.tensor([1.0 if value else 0.0], dtype=torch.float32)
GeneralPGUtil.create(group).all_reduce(tensor, group, op=dist.ReduceOp.MIN)
return tensor.item() > 0.5

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.

Please apply.

fzyzcjy added a commit that referenced this pull request Jul 8, 2026
fzyzcjy added a commit that referenced this pull request Jul 8, 2026
… (PR #1412)

Review comment on #1412: a CPU tensor crashes all_reduce on NCCL
groups. Derive the device from the process group itself (cpu for gloo,
cuda for nccl).
@fzyzcjy
fzyzcjy force-pushed the tom/pr_chain/trainer_ft/dev_revert_reversed/add-reconfiguration-assertions-for-fault-tolerance-tests branch from 940e7e7 to 2980130 Compare July 8, 2026 03:53
@fzyzcjy
fzyzcjy force-pushed the tom/pr_chain/trainer_ft/dev_revert_reversed/relocate-groupinfo-into-shared-process-group-utilities branch from 58be551 to aa46de3 Compare July 8, 2026 03:53
@fzyzcjy
fzyzcjy force-pushed the tom/pr_chain/trainer_ft/dev_revert_reversed/add-reconfiguration-assertions-for-fault-tolerance-tests branch from 2980130 to 9d6a573 Compare July 8, 2026 05:55
@fzyzcjy
fzyzcjy force-pushed the tom/pr_chain/trainer_ft/dev_revert_reversed/relocate-groupinfo-into-shared-process-group-utilities branch from aa46de3 to 51bc303 Compare July 8, 2026 05:55
fzyzcjy added 6 commits July 10, 2026 10:04
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).
fzyzcjy added 23 commits July 10, 2026 10:04
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.
Add a per-process identity helper that uniquely keys each training process, used
to attribute structured fault-tolerance events to their originating process.

- miles/utils/process_identity.py and tests.
Add the structured event models (Event / EventBase hierarchy) for the
fault-tolerance event log, each tagged with the originating ProcessIdentity.

- miles/utils/event_logger/models.py and tests.
Add the structured event logger that records typed events keyed by per-process
identity, wire it through the logging helper and CLI argument, and start it from
the train entrypoints.

- miles/utils/event_logger/logger.py, logging_utils.py, arguments.py and entrypoint wiring, with tests.
Add snapshot/restore for the structured event log so the event history survives
cell restarts during fault-tolerant training.

- miles/utils/event_logger/checkpoint.py and tests.
Add the `MetricEvent` model (a discriminated-union member) and emit every
tracking metric into the structured event log: `tracking_utils.log` now forwards
`{metrics}` to `get_event_logger().log(MetricEvent, ...)` when the event logger
is initialized.
Add the witness id allocator and `WitnessInfo` carrier used to assign and track
witness ids for fault-tolerance verification.

- miles/utils/witness/allocator.py and tests.
Thread witness ids through the model by injecting witness parameters, so the
event log can later verify they propagate correctly.

- miles/utils/witness/module.py, model_provider.py and tests.
Add the first event-analyzer rules that replay the structured event log and flag
weight-checksum inconsistencies: a `checksum_compare` helper (flatten nested
dicts, diff flat checksum maps) plus two rules built on it — cross-replica weight
checksum consistency and inference-engine weight checksum consistency — with unit
tests.

- miles/utils/event_analyzer/rules/{checksum_compare,cross_replica_weight_checksum,inference_engine_weight_checksum_consistency}.py and tests.
Add the witness-tracing rule for the event analyzer: it follows witness ids
through the replayed event log to verify they are propagated correctly across the
training pipeline, with unit tests.

- miles/utils/event_analyzer/rules/witness.py and tests.
Add the analyzer that replays the structured event log and applies the analysis
rules (checksum-consistency and witness tracing) to verify fault-tolerance
behaviour offline, with unit tests.

- miles/utils/event_analyzer/analyzer.py and tests.
Add comparison helpers used by fault-tolerance tests to compare dumped tensors and
inference-engine checksums offline (generic comparators, dump comparison, and an
inference-engine checksum comparison built on the event-analyzer checksum rule).

- miles/utils/test_utils/comparisons/{comparators,dumps,inference_engine_checksums}.py and tests.
Add the metric comparison helpers used by fault-tolerance tests to compare logged
training metrics offline.

- miles/utils/test_utils/comparisons/metrics.py.
Add reusable reconfiguration assertions used by fault-tolerance tests to verify
cell reconfigure / healing behaviour, with unit tests.

- miles/utils/test_utils/reconfigure_assertions.py and tests.
Move the GroupInfo dataclass out of training_utils/parallel.py into a shared
process_group_utils module that also provides multi-process-group helpers
(GeneralPGUtil / MultiPGUtil) for collectives over single or hierarchical
process groups, used by the cross-replica / effective-DP code paths.

- process_group_utils.py: GroupInfo + GeneralPGUtil / MultiPGUtil (+ tests).
- training_utils/parallel.py: import GroupInfo from the shared module.
- megatron / fsdp parallel.py: import GroupInfo from the shared module.
- distributed_utils.py: use GeneralPGUtil for masked-whiten all-reduce.
@fzyzcjy
fzyzcjy force-pushed the tom/pr_chain/trainer_ft/dev_revert_reversed/add-reconfiguration-assertions-for-fault-tolerance-tests branch from 9d6a573 to c043ccf Compare July 10, 2026 02:11
@fzyzcjy
fzyzcjy force-pushed the tom/pr_chain/trainer_ft/dev_revert_reversed/relocate-groupinfo-into-shared-process-group-utilities branch from 51bc303 to 932660e Compare July 10, 2026 02:11
Base automatically changed from tom/pr_chain/trainer_ft/dev_revert_reversed/add-reconfiguration-assertions-for-fault-tolerance-tests to main July 10, 2026 03:18
…ft/dev_revert_reversed/relocate-groupinfo-into-shared-process-group-utilities
@fzyzcjy
fzyzcjy merged commit 0b6104d into main Jul 10, 2026
6 checks passed
@fzyzcjy
fzyzcjy deleted the tom/pr_chain/trainer_ft/dev_revert_reversed/relocate-groupinfo-into-shared-process-group-utilities branch July 10, 2026 03:18
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.

3 participants