What happened to each of the 17 review comments

read this  #17 · Condemn a wedged shard instead of ending the run

single_controller.py:1576 · the comment · fixed in 1e4d5fc8c, then 2f762b8d8 reverted by 02b7dd0d6

Which case this is. Not the crash case. A crash makes a shard DEAD — Ray reports the dead process and the probe confirms it — so the group can be rebuilt without it and the refit retried. This is the hang case: every process is still alive, nothing is DEAD, so there is nobody to drop and the code raised and ended the run.

The ask. The ledger already knows the culprit — a wedged engine has been timing out its own generations and is sitting in SUSPECT the whole time. So condemn that shard, rebuild smaller, and retry.

What landed, and stayed. Exactly that, including the "exactly one suspect, or not at all" rule:

# the abort is independent evidence that something stopped participating;
# SUSPECT says which. Exactly one, or not at all.
suspects = self._gen_fleet.suspected_shards()
if len(suspects) == 1:
    culprit = suspects[0]
    ... "condemning it as the silent participant and retrying"

Then he went further — and this part is worth understanding, because the reasoning was good. Condemning a shard only changed bookkeeping: "the fleet said DEAD while the process sat there." A py-spy dump from job 6521181 showed both trainers stuck in init_nccl_communicator — building the new group — while their old ncclCommAbort threads were still in native code 25 minutes later. His read:

An asymmetry in the tests fit that perfectly. The harness breaks a shard two ways: SIGKILL it (process dies, sockets close) or SIGSTOP it (alive but frozen, sockets stay open). The SIGKILL variant of this same reshard test had always passed; only the frozen one wedged.

So 2f762b8d8 tried to turn the frozen case into the killed case. ray.kill is the forceful one — it terminates the worker's process, the same outcome SIGKILL produces in the test — so the fix killed every worker of a shard the fleet had condemned while its process was still up, before dispatching the rebuild. Sockets close, the peers' pending aborts retire, the rebuild proceeds down the path the killed variants already prove. Note this is terminating a process, not aborting a communicator — two different things that both get called "killing the shard".

Why it was reverted: the experiment refuted it. Job 6523731 ran the kill in exactly the intended order —

So the sockets were never the blockage. The real cause is local to the trainers: aborting a communicator does not retire CUDA work already enqueued on a stream, and those orphaned kernels sit on the trainers' own devices. Nothing done to the remote rank — killing it, closing its sockets, anything — can retire them. The kill changed product behaviour and bought nothing, so it went:

Where it leaves the behaviour — now split by transport, which is more precise than the comment had it:

The design said so before the comment was written, in sync_stream_within: "Recovering a frozen-but-alive rank on this transport stays out of scope." The comment stated the ask as universal; it was right for one transport and wrong for the other.

read this  #14 · The same GPU gets numbered twice

vllm_backend.py:288 · the comment · fixed in 1847b135b

What goes wrong. There are two different things get_rank() can mean inside a vLLM engine, and the line above assumes the first one:

• Normally, each engine is its own little world of tp × pp processes. get_rank() returns the position inside that engine — 0, 1, 2 — and rank_prefix is what slides the engine into the right slot of the big group. Adding the two is correct.
• When expert_parallel_size > tensor_parallel_size, NeMo-RL switches vLLM into its own data-parallel mode by setting VLLM_DP_SIZE (vllm_generation.py:236). vLLM then makes get_rank() global across all the engines. The offset is now already inside that number, and adding rank_prefix counts it a second time.

Worked case: tp=1, pp=1, ep=8, eight generation GPUs, T training ranks, nothing dead.

Shards 4 through 7 land outside the group. Some ranks raise, the rest sit in communicator setup waiting for ranks that will never show up.

What landed. Comment #12’s guard landed, so check_nccl_reshard_refit_support now runs from setup_single_controller and rejects the config before any rank arithmetic. The author also routed :285 through resolve_rollout_rank, which the comment asked him not to do:

Safe today anyway, because the guard means data-parallel vLLM can never reach the reshard path on SingleController, so the branch that would misfire never runs.

What to watch. Correctness now rests on the guard rather than on the arithmetic. A config check is a list of restrictions people relax over time; if this one is ever relaxed, the bug comes back on the rebuild path, where it is quieter than it was at setup. Worth one comment at the call site saying so.

read this  #10 · The abort misses the smaller groups

stateless_process_group.py:133 · the comment · fixed in 1ce2cadc3

The ask. abort() was called on the parent only. NCCL gives each split child its own flag — unless you set NCCL_COMM_SPLIT_SHARE_RESOURCES=1, which nothing in this repo does. So aborting the parent flips the parent’s flag and reaches nothing else. A rank stuck in a call on a child is never woken up.

That is worse than getting the wrong error type back. The call never returns at all, so the with block never exits, so the line that would ask “did the watchdog fire?” is never reached. The timeout goes off and the run hangs anyway — the exact outcome this PR exists to remove, on a path it did not cover. The comment proposed calling the existing clear_xferdtensor_python_caches before the parent abort.

What landed — better than the ask. A new helper that aborts the children rather than running the cleanup path:

# Before the parent: once nccl_communicator is None the cache keys derived
# from it cannot be recovered, and the children would be stranded as well
# as un-aborted.
from nemo_rl.weight_sync.xferdtensor_python import (
    abort_xferdtensor_python_subcommunicators,
)
abort_xferdtensor_python_subcommunicators(self)

The helper I suggested tears children down with finalize()/destroy(), and those are collective — every rank in the group has to call them, in the same order. A dead rank cannot. So my version would have moved the hang rather than removed it. Abort is one-sided, which is the whole reason the parent uses it too. The new function’s docstring now carries that reasoning and the splitShare detail, so the next reader does not have to rediscover either.