Conversation
There was a problem hiding this comment.
Code Review
This pull request simplifies the conditional check in connect_rollout_engines by moving the None check for model_update_groups into disconnect_rollout_engines_from_distributed. The reviewer suggested wrapping the process group destruction in a try...finally block to ensure that the remote rollout engine destruction references are always awaited, even if destroying the local process group raises an exception.
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.
| if model_update_groups is not None: | ||
| dist.destroy_process_group(model_update_groups) | ||
| ray.get(refs) |
There was a problem hiding this comment.
If dist.destroy_process_group(model_update_groups) raises an exception (e.g., due to a timeout or NCCL error), the subsequent ray.get(refs) will be skipped. This can leave the rollout engines in an inconsistent state where they are still destroying the old process group while the training side attempts to recreate it, potentially leading to deadlocks or port conflicts during reconfiguration/healing.
Using a try...finally block ensures that we always await the remote destroy calls on the rollout engines, even if destroying the local process group fails.
| if model_update_groups is not None: | |
| dist.destroy_process_group(model_update_groups) | |
| ray.get(refs) | |
| try: | |
| if model_update_groups is not None: | |
| dist.destroy_process_group(model_update_groups) | |
| finally: | |
| ray.get(refs) |
References
- To prevent resource leaks (e.g., counters that are not decremented), use constructs like try...finally or a with statement to ensure cleanup logic is always executed, even in the case of exceptions or early returns.
Shi-Dong
left a comment
There was a problem hiding this comment.
I think the Gemini bot raised a valid concern. Please address.
dab12e7 to
2d90999
Compare
f3b1806 to
46e0208
Compare
2d90999 to
790ef78
Compare
46e0208 to
e6ad8ac
Compare
790ef78 to
5f02197
Compare
e6ad8ac to
6b7acdf
Compare
hmm I think the concern will not happen given the overall design? |
Review comment on #1395: if destroying the local process group raised, the ray.get on the engines' already-in-flight destroy calls was skipped, so a caller on the FT reconnect path could recreate the group while engines were still tearing down.
5f02197 to
e3517cb
Compare
6b7acdf to
6d0a516
Compare
e3517cb to
8bf23fa
Compare
6d0a516 to
994f590
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.
8bf23fa to
4a52fcf
Compare
994f590 to
2ed9fcf
Compare
…ft/dev_revert_reversed/always-reconnect-rollout-engines-on-weight-update-setup
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.
destroy_process_groupagainst None.