feat(sc): keep training when a generation shard dies - #3591
Merged
Conversation
Splits rollout failures into infrastructure (the prompt is fine, the fleet is not) and data (deterministic and prompt-specific). That split drives the retry policy landing in follow-up commits: infra failures re-dispatch the prompt onto another generation shard, data failures get a small budget because another shard would fail identically. HTTP errors are classified by status rather than by type. vLLM answers an over-long prompt with 400 "This model's maximum context length is ...", so treating every aiohttp ClientError as infrastructure would spend the retry budget on a prompt no shard can serve. 5xx plus 408/429 are infra, which matches the set NeMo-Gym itself retries in nemo_gym/openai_utils.py. Anything unrecognized is classified as data so that unexpected exceptions fail loudly rather than being retried into silence. Adds the async_rl.rollout_failure and async_rl.watchdog config blocks plus the three timeout fields. Every default is inert: the timeouts default to null, so a config that does not mention these fields behaves exactly as it did before. The validators reject combinations that would silently do nothing -- notably on_data_exhausted=skip with a zero skip budget, which would otherwise behave exactly like fail_fast while reading as though bad prompts were tolerated. No behaviour change yet: nothing consumes the taxonomy or the config until the following commits. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Signed-off-by: asolergibert <asolergibert@nvidia.com>
_run_single_rollout caught every exception from _generate_response, printed it,
and broke out of the turn loop. Execution then fell through and built a
Completion holding only the prompt with reward=0.0, which generate_and_push
committed as a training row.
It did not even crash downstream. add_grpo_token_loss_masks_and_generation_logprobs
zero-fills a missing generation_logprobs and sets token_loss_mask=0, so the row is
well formed and contributes no gradient -- but its zero reward does enter the
per-prompt GRPO baseline. With use_leave_one_out_baseline a single dead vLLM
worker silently shifts the advantage of every sibling in the group, for the rest
of the run, with nothing in the logs but a print.
Generation failures are now classified and raised: infrastructure errors become
GenerationUnavailable, everything else RolloutDataFailure, both carrying the
prompt and trajectory coordinates a raw traceback lacks. The try body is narrowed
to the generation call so the surrounding bookkeeping can no longer be skipped
half-done, and CancelledError still passes through untouched because it is not an
Exception.
Two adjacent fixes the propagation exposes:
- asyncio.gather propagates the first exception but leaves the remaining
awaitables running detached, so a failed group left N-1 generations queued
against the fleet for a result already being discarded.
_gather_cancelling_siblings cancels and drains them first. This was latent
before -- calculate_rewards could already raise -- and matters more once
retries land.
- The gen_leader_worker_idx catch is narrowed from Exception to
(IndexError, TypeError, ValueError). It is a load-accounting metric and must
not fail a rollout, but the catch should not hide unrelated errors.
Behaviour change: a dead generation shard now fails the run loudly instead of
quietly degrading the batch. The re-dispatch policy that keeps the run alive
lands in a follow-up commit; correctness first.
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Signed-off-by: asolergibert <asolergibert@nvidia.com>
Nothing in the SingleController rollout path had a deadline. NeMo-Gym compounds this: it passes aiohttp ClientTimeout() -- every field None, so all timeouts disabled -- and retries ClientOSError and ServerDisconnectedError in uncapped 0.5s loops. A dead vLLM endpoint therefore parks a rollout forever, and each parked rollout permanently holds one _buffer_capacity and one max_inflight_prompts permit. Enough of them and the rollout pump blocks on sem.acquire() while the train pump spins, with no exception raised anywhere. Adds deadlines at the three waits, resolved from async_rl into a RolloutTimeouts dataclass so async_rl remains the single place a default lives: - the whole NeMo-Gym prompt-group stream (rollout_timeout_s) - one generate_async turn on the native path (generation_timeout_s) - one calculate_rewards environment step (env_timeout_s) The gym deadline deliberately spans the entire stream rather than each await. Gym yields rows as they finish, so a per-await budget would reset every time a fast row landed and never fire for the slow row actually holding the group up. The env deadline frees the rollout, not the thread: Python cannot kill a running thread, so a hung env call keeps its thread-pool slot until its own ray.get returns. Unblocking the rollout is still the point. _Deadline wraps asyncio.timeout and consults expired() before relabelling, so a TimeoutError raised by the wrapped code is not reported with our deadline's duration -- that would send anyone debugging it to the wrong knob. Expiry surfaces as RolloutTimeout, an infra failure, so the retry policy will treat it as retriable. Outer cancellation still propagates as CancelledError. Also reclassifies the truncated-gym-stream error from bare RuntimeError to GymTransportError and names the missing rows. Rows going missing is a transport problem and must be retriable rather than reading as a bad prompt. All three default to null, so a config that does not set them behaves exactly as before. Two pre-existing object.__new__ test fakes are updated for the new attribute. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Signed-off-by: asolergibert <asolergibert@nvidia.com>
P0.2 made a dead generation shard fail the run loudly rather than quietly
corrupting the batch. This restores availability without giving the corruption
back: generate_and_push now retries, and no prompt is discarded for
infrastructure reasons.
An infra failure says the fleet is unwell, not the prompt, so the attempt is
retried. The retry re-enters generation-shard selection, which is what makes it
land somewhere else -- generate_and_push itself knows nothing about shard health,
and does not need to. Exhausting the infra budget therefore means the same
failure followed the prompt across repeated selections, which is reported as
RolloutRedispatchExhausted rather than absorbed.
Deterministic failures get their own, much smaller budget. Another shard rejects
the prompt identically, so retrying mostly burns time -- but one retry is still
worth taking, because a shard under memory pressure can return an empty
generation that looks deterministic and is not. On exhaustion the default is to
fail the run, since a genuinely deterministic failure is almost always a config
bug (usually max_total_sequence_length versus the engine's max_model_len).
on_data_exhausted=skip exists for long soaks and is bounded by a run-wide
max_skipped_prompts.
Details worth knowing when reading this:
- The slot is reserved inside the loop, so each attempt owns a fresh group_id
and a failed attempt's rows cannot collide with the retry's.
- The loop condition is the infra budget, so exhaustion exits through a normal
terminal rather than raising from inside the handler. RolloutRetryPolicy
rejects a zero budget, which is what makes that terminal's invariant hold.
- Data failures do not back off. Waiting cannot help a deterministic failure.
- Cancellation is caught by a separate `except BaseException` and never
retried: tearing down the controller must not look like a transient fault.
- A SKIPPED prompt never reaches the buffer, so the train pump will never
release its backpressure permit. _dispatch_one_prompt releases it directly;
getting this wrong leaks one slot per skipped prompt until the pump wedges.
There is a parametrized test pinning both branches.
RolloutStats counts commits, skips, re-dispatches and data failures by reason.
A rising re-dispatch count is the only externally visible sign that the fleet is
degrading, so it is not optional bookkeeping. Wiring it into the SC logger comes
with the watchdog.
Known gap, deliberately deferred: a gym retry currently redoes the whole prompt
group rather than only the rows that never arrived. _run_rollouts already tracks
received_row_indices and trajectory_collector.py has the prior art, so partial
re-dispatch is a follow-up rather than a redesign. Correct and wasteful before
efficient.
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Signed-off-by: asolergibert <asolergibert@nvidia.com>
Every other guard in this phase reacts to something raising. The wedge this work exists to prevent raises nothing at all: rollouts sit in NeMo-Gym's uncapped retry loop, the train pump spins on sleep(0.005), and Ray reports everything healthy. The only way to see it is to notice that committed groups stopped moving while rollouts are still in flight. _watchdog_pump runs as a third asyncio task and does two things. It publishes the RolloutStats counters plus in-flight and idle-time gauges, so a degrading fleet is visible before it wedges -- rollout/idle_s is the leading indicator. And it reports a stall, defined as in-flight rollouts with no commits for stall_timeout_s. Progress is measured by the committed counter rather than a timestamp because "no group has landed" is the property that matters, whatever the cause. An idle controller with nothing in flight is explicitly not a stall; between epochs there is legitimately no work. stall_action defaults to warn so the threshold can be tuned against a real workload before it is allowed to end a run. NemoGym gains health_check(), a thin wrapper over NeMo-Gym's own RunHelper.poll(). Gym already implements that check and calls it every 60s from run_forever(); NeMo-RL only ever called rh.start(), so it never ran. Without it a dead tool server surfaces as unexplained rollout timeouts rather than a named process. The watchdog polls every environment handle that exposes the method and skips those that do not -- only NeMo-Gym has subprocess servers to lose. While there: NemoGym now declares rh/rch/head_server_config/node_ip/ head_server_port in __init__ and guards the methods that need them. Ray recreates a restarted actor through __init__ alone, which does not start the Gym servers, so a restarted NemoGym previously surfaced that state as an AttributeError from deep inside a rollout. It now says what actually happened. shutdown() became a no-op in that state too, since it runs in a finally block and must not mask a real training error. run() awaits the watchdog first when several tasks finish together: it only completes by raising, and its diagnosis is more specific than the pumps', whose own symptom would just be "waiting". Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Signed-off-by: asolergibert <asolergibert@nvidia.com>
Fault injection found this: killing a vLLM generation worker wedged the loop for
six minutes while the watchdog watched and said nothing.
The stall condition required rollouts to be in flight, on the reasoning that an
idle controller has legitimately no work. The wedge has none. _sync_weights
clears _rollout_permitted on entry and only sets it on exit, so a hung weight
sync leaves the gate shut and the rollout pump parked at
`await self._rollout_permitted.wait()` -- before dispatch, before anything can
fail. Nothing is in flight to count, nothing fails, and the run sits there.
The watchdog's own metrics from that run diagnose it exactly:
rollout/inflight: 0.0 at every sample
rollout/idle_s: 371.8 <- it saw the idleness
rollout/redispatch_total: 0.0 <- no rollout ever failed
rollout/committed_total: 10, frozen
and train/loss stopping one step short of the watchdog's own step counter places
the hang inside _sync_weights: _train_pump increments _train_steps, then syncs,
then logs. The sync is a NCCL broadcast to an inference rank that no longer
exists, and ray.get on the trainer future comes before the one that would have
raised ActorDiedError.
Progress is now the pair (committed groups, completed train steps), and what
separates a stall from an idle gap is whether work remains, not whether anything
is in flight. rollout/train_steps is published alongside so the two can be told
apart from outside.
Also adds tests/functional/grpo_dp_single_controller_chaos.sh, the harness that
found this. Two things it has to do that are not obvious:
- Teardown reaps VLLM::EngineCore and the policy workers, not just the driver.
vLLM runs its engine in a child process that survives its parent actor being
killed -- which is exactly what this test does on purpose -- and the orphan
holds tens of GB of device memory. A leaked run made the next one fail in
placement-group setup, reading as an unrelated flake. It is hard to spot
because nvidia-smi in a container reports host pids, so the offender is
invisible to `ps -p`.
- Startup refuses to run on a dirty GPU, so a leftover allocation cannot be
misreported as a failure of the code under test.
On the run that passes, the re-dispatch path catches the kill in ~10s:
ActorDiedError -> GenerationUnavailable -> three attempts -> exhausted. Ray
reports actor death immediately, so the generation deadline never has to fire.
Not in any CI lane: it needs GPUs and is timing-sensitive.
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Signed-off-by: asolergibert <asolergibert@nvidia.com>
A gym retry redid the whole prompt group. NeMo-Gym's stream dies on its first
failing row, so one bad row takes every later row with it -- at
num_generations_per_prompt=16 that means paying 16 generations to recover
however many were actually lost.
_run_rollouts now keeps completed rows across attempts and re-sends only the
pending ones, which is the same shape as the legacy collector's pending-group
retry in trajectory_collector.py. A 5-row group that loses rows 3-4 costs 7
row-generations instead of 10.
Three details that shape the implementation:
- The deadline still belongs to the prompt group, not to each attempt.
Wrapping each attempt instead would silently multiply rollout_timeout_s by
max_gym_row_attempts.
- Only infrastructure failures are re-dispatched. A prompt NeMo-Gym cannot
serve fails the same way every time, and retrying it here would also
multiply against the outer data budget in generate_and_push.
- Row indices are validated against the original group rather than the pending
subset, because a re-dispatched row keeps its original _rowidx so results
stay ordered. That makes "_rowidx equals position" a contract of
_run_rollouts, so it is now checked up front -- the alternative is a KeyError
several frames deeper.
max_gym_row_attempts defaults to 3, matching the legacy
_MAX_NEMO_GYM_STREAM_RETRIES. The RolloutRetryPolicy default stays 1 so a
directly-constructed RolloutManager does not silently gain retries.
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Signed-off-by: asolergibert <asolergibert@nvidia.com>
The harness selected its victim with pgrep -f 'ray::.*[Gg]eneration'. In a run against vLLM 0.25.1 that matched a process which was NOT the serving generation worker: the kill landed, generation carried on, training reached step 3/50, and the test was on course to report a wedge that never happened -- a false failure of exactly the containment behaviour it exists to prove. Widen the pattern to match both forms the worker appears as (ray::-titled and isolated-venv path; both were observed), but the assertions matter more than the pattern: * print the victim's cmdline, so a mis-target is visible in the log rather than masquerading as a hang; * refuse to proceed unless the victim looks like a generation worker; * verify it actually died, rather than assuming SIGKILL landed. Also raise DEATH_DEADLINE_S from 300s to 600s. An observed run took 222s to fail -- 5 re-dispatch attempts with capped exponential backoff, which is the designed behaviour -- leaving only 26% headroom on a workstation that is faster than CI. Verified on 2xA6000 with TransformerEngine rebuilt for sm_86: [chaos] killing generation worker pid=69107 [chaos] cmdline: ray::VllmAsyncGenerationWorker [chaos] PASS: bounded, attributable failure 222s after the kill Signed-off-by: asolergibert <asolergibert@nvidia.com>
The harness was excluded as 'inherently timing-sensitive'. That no longer holds: the death deadline is 600s against an observed 222s, and the victim is now asserted rather than assumed. The recovery tests already in this lane kill processes the same way, so excluding this one was also inconsistent. It matters more than the others. A wedge raises no exception and fails no assertion anywhere else, so without this test a regression that restores the silent hang -- the failure this whole series exists to remove -- would be caught by nothing. Full mode only; it costs ~10 minutes and deliberately drives the job to a non-zero exit. Signed-off-by: asolergibert <asolergibert@nvidia.com>
Adding `timeouts` as a required positional argument broke every direct construction of AsyncRolloutImpl. That went unnoticed because production always builds it through the config path, which passes the field -- the only direct construction is in tests. Upstream's new tests/unit/experience/test_rollout_manager_router_replay.py (from #3378) is one, so after syncing onto current main it failed with TypeError: __init__() missing 1 required positional argument: 'timeouts'. The rebase was textually clean; the incompatibility is in the signature, not in any line either side edited, so nothing flagged it. RolloutTimeouts is a frozen dataclass whose fields all default to None, meaning "no deadline, wait indefinitely" -- the historical behaviour. So defaulting the parameter restores the old semantics for callers that do not ask for deadlines, rather than silently imposing one. Same reasoning already applied to RolloutRetryPolicy, whose defaults reproduce single-attempt behaviour so a directly-constructed manager does not silently gain retries. No separate regression test: upstream's file is the regression test, and it runs in our lane. Signed-off-by: asolergibert <asolergibert@nvidia.com>
…ich state
Victim selection was a coin flip. `pgrep -f` on a loose
'[Gg]enerationWorker' substring, then `head -1`, which picks by pid
order. Sampling a live run 378 times shows what that actually matched:
/opt/ray_venvs/...VllmAsyncGenerationWorker/bin/python a CHILD, not the actor
bash -c exec /opt/ray_venvs/...GenerationWorker... the launcher shell
ray::VllmAsyncGenerationWorker the actor, between calls
ray::vllm_policy-0-0:VllmAsyncGenerationWorker.__init__ the actor, constructing
ray::VllmAsyncGenerationWorker.generate_async the actor, serving a rollout
ray::VllmAsyncGenerationWorker.init_collective_async the actor, setting up refit
ray::VllmAsyncGenerationWorker.shutdown the actor, tearing down
Three distinct processes across five states, because Ray retitles a
worker with setproctitle for the exact duration of each call.
It never failed, and that is the point: every one of those scenarios does
end in a bounded attributable failure, which is all the test asserted. The
divergence was only visible as wall-clock across branches -- 7s on one,
222s on another, for what was supposed to be the same test.
The existing guard did not help. `case $VICTIM_CMD in *[Gg]enerationWorker*`
was added earlier to catch a mis-targeted kill, and it passes for all
seven forms above, including the launcher shell and the venv child. It
checked WHAT was killed and never IN WHAT STATE, so it was structurally
blind to this while reading like protection against it.
Now the actor is matched structurally -- anchored ray:: prefix, optional
<name>: infix -- so the child and the shell cannot match, and the state is
pinned:
idle (default) no method suffix. Nothing is in flight, so the loss
must be DETECTED, by health probe or by the stall detector.
serving .generate_async. An in-flight rollout RPC dies with the worker
and surfaces immediately, detection doing no work.
Not "any method suffix": __init__, init_collective_async and shutdown are
three further distinct scenarios, and folding them in would reintroduce
the ambiguity being removed. The title is re-read immediately before the
kill and the run fails if it changed, so the sub-millisecond window
between scan and kill cannot quietly restore the coin flip.
Both modes registered in the lane. Pinning to idle alone would silently
drop a scenario the old selection used to hit by chance, and the serving
path costs seconds.
Verified on 2xA6000: idle twice -> ray::VllmAsyncGenerationWorker, 222s
both times; serving -> ray::VllmAsyncGenerationWorker.generate_async, 12s,
GenerationUnavailable + RolloutRedispatchExhausted. Regexes checked
against all seven observed titles: each mode matches exactly one.
Signed-off-by: asolergibert <asolergibert@nvidia.com>
The pre-flight check took the max memory.used across every GPU and aborted above 1GiB. That is an OR over the whole host, so it gets likelier to trip the bigger the machine: this test pins itself to 2 GPUs, so on an 8-GPU CI runner a single unrelated process on one GPU aborts it with six sitting idle. The message says 'clean up before running', which reads like a real problem with the machine rather than an over-strict check. Count free GPUs and require $GPUS of them instead. It still catches the case the check was written for -- a previous test in the lane leaking a VLLM::EngineCore -- because that drops the free count below the requirement. Verified across simulated host sizes: 2-GPU box, both idle free=2 proceed 2-GPU box, leftover EngineCore free=1 ABORT (still caught) 8-GPU node, all idle free=8 proceed 8-GPU node, one GPU busy free=7 proceed (was: ABORT) 8-GPU node, six busy free=2 proceed 8-GPU node, seven busy free=1 ABORT GPUS is now defined once and feeds both cluster.gpus_per_node and the check, so the two cannot drift apart. Re-ran the chaos test after the change: killed ray::VllmAsyncGenerationWorker in state idle, bounded failure at 222s -- identical to the two runs before it. Signed-off-by: asolergibert <asolergibert@nvidia.com>
Two files imported the same module twice instead of merging the names,
which ruff's isort rules (I001) reject:
from nemo_rl.experience.rollout_manager import RolloutManager, RolloutTimeouts
from nemo_rl.experience.rollout_manager import (RolloutRetryPolicy,)
`ruff check` did not catch this locally, and neither did the SLURM run --
both reported "All checks passed!". The repo's .pre-commit-config.yaml
registers the ruff hook twice:
- id: ruff
args: ["--fix"]
- id: ruff
args: ["check", "--select", "I", "--fix"]
Only the second selects `I`, and `I` is not in the default selection that
a plain `ruff check` uses. So the rule that failed in CI was never being
run locally. Verified by reproducing it: `ruff check --select I` reports
exactly the two errors GitHub reported, and the fix it applies is
byte-identical to the diff in the CI log.
Both files are ones this branch introduced changes to; no unrelated files
were touched.
Signed-off-by: asolergibert <asolergibert@nvidia.com>
Merging main brought in #2518, which moves GRPOConfig from TypedDict to a pydantic BaseModel, so `master_config.grpo["key"]` becomes `master_config.grpo.key`. The merge was textually clean and left three sites broken. Two were visible: test_rollout_pump.py fed `grpo={"max_num_epochs": 1}` into _rollout_pump, which #2518 migrated to attribute access, giving "AttributeError: 'dict' object has no attribute 'max_num_epochs'" on both parametrisations. The third was not visible, and is the one that mattered. single_controller.py read `self._master_config.grpo["max_num_steps"]` while test_watchdog_pump.py passed a dict -- self-consistent, so the unit tests stayed green while production, which now receives a real pydantic config, would have raised at runtime in the watchdog. A functional test would have caught it; no unit test could, because the fake and the code agreed with each other and both disagreed with reality. Fixtures now build GRPOConfig.model_construct(...), matching how #2518 migrated upstream's own fixtures in the same file. 315 passed across tests/unit/single_controller and tests/unit/experience. Signed-off-by: asolergibert <asolergibert@nvidia.com>
GRPOConfig.model_construct(...) made two assignments exceed the line limit, so ruff-format wanted to wrap them. CI's ruff-format hook would have rejected this. It was reported as clean locally because the check itself was wrong: `ruff format --check ... | grep -o 'already formatted'` matches the tail of "2 files would be reformatted, 542 files already formatted". Checking the exit code instead of grepping the message. Signed-off-by: asolergibert <asolergibert@nvidia.com>
The chaos harness picked its victim by matching `ray::` process titles. That worked on a workstation and found ZERO generation actors on a GB200 cluster (job 5861743): the poll never matched, the run simply finished, and both chaos variants reported "job died before the kill" -- a green-looking failure that never killed anything. Process titles are a runtime implementation detail; the GCS actor table is the runtime's own record, so ask that instead. Titles are still used for the idle-vs-serving distinction, because Ray only exposes the running method through the dashboard state API and init_ray disables the dashboard. So: discover authoritatively, refine by title where titles exist, and say so plainly where they do not -- VICTIM_STATE=any then kills a generation actor without pinning the state, which still asserts a bounded attributable failure but stops separating the detection path from the in-flight-RPC path. Also unbuffers the driver. The harness detects progress by grepping the run log, and the driver's stdout is a redirected file, so Python block-buffered it: job 5892910 wrote "train step 3/24" at 10:40:38 and the harness did not see it until 10:48:21, by which time there was nothing left to kill. The actor prints with flush=True, but that only reaches the driver. The remaining changes are all about being able to diagnose the next failure: ACTOR_WAIT_S separates "no actors yet" from "actors never reached the state", which were previously indistinguishable in the log; ACTOR_QUERY_TIMEOUT_S bounds one discovery attempt; and the helper's stderr is printed inline rather than to a side file, because two attempts at a side file never produced one that survived to be read. Developed while getting the recovery tests to run on the cluster, and belongs here: none of it depends on anything the later branches add, and without it this branch's chaos test cannot pass on that hardware at all. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Signed-off-by: asolergibert <asolergibert@nvidia.com>
…01-containment # Conflicts: # nemo_rl/environments/nemo_gym.py
The main sync brought upstream's test_logs_setup_timing_metrics, which builds
actor_args as a SimpleNamespace listing only the fields upstream's own __init__
reads. env_handles is not among them -- upstream declares it on
SingleControllerActorArgs but never touches it in __init__ -- so our
environment-health-check read turned a field nobody else needs into a
construction requirement, and the test died with AttributeError.
getattr with a {} default. Every production caller goes through the dataclass and
still supplies it, so behaviour is unchanged; only test fixtures that do not
exercise environment health are let off.
Identical in shape to the `timeouts` regression from the previous main sync: a
required parameter we added, discovered by an upstream test constructing the
object directly. Worth remembering as the recurring cost of reading new fields
in __init__ rather than where they are used.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Signed-off-by: asolergibert <asolergibert@nvidia.com>
Both SC rollout paths pick a generation shard by static round-robin with no idea
whether the shard is alive -- the native path in
VllmGeneration._async_generate_base, and the NeMo-Gym path inside Gym's own
_resolve_client. This adds the missing half: which shards are eligible to serve,
and why. Nothing consumes it yet.
GenerationFleetMonitor is deliberately a pure state machine. Probing, restarting
and pushing membership are I/O and belong to the caller, which keeps every
transition testable without Ray, a network or a GPU, and keeps one description of
"eligible" that both routing adapters will read.
The transition that carries the most weight is the one that does not exist: a
shard cannot go from DEAD back to HEALTHY on its own. A restarted engine holds
whatever weights it loaded at init, so re-admitting it because it started
answering probes again would feed training rollouts generated from a checkpoint
hundreds of steps stale -- invisible, and worse than the outage that caused it.
Recovery must pass through STALE and a completed refit, and DEAD/RESTARTING/STALE
all ignore successful probes to make that hard to get wrong by accident.
Other decisions worth knowing:
- SUSPECT still serves traffic. Draining on a single failed probe would make a
transient blip cost a shard's worth of throughput; only unhealthy_threshold
consecutive failures condemn a shard.
- The membership epoch advances only when the *serving set* changes, so
HEALTHY -> SUSPECT does not disturb it. Downstream reconciliation can then be
an integer comparison in the common case.
- Retirement is terminal and bounded by max_restart_attempts_per_shard, with
min_healthy_shards as the floor below which the run stops being worth
continuing.
- HealthyShardSelector uses least-outstanding rather than round-robin: it
steers away from a shard that is merely slow or wedged without that having to
be diagnosed first.
async_rl.fleet_health declares only the knobs P1 consumes. on_dead_shard is a
Literal accepting just "fail_fast" so the recovery modes that need the
communicator rebuild are rejected rather than silently doing nothing.
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Signed-off-by: asolergibert <asolergibert@nvidia.com>
_async_generate_base advanced a counter modulo dp_size and sent the request wherever it landed, with no idea whether that shard was alive. A dead shard therefore kept receiving its full 1/N share of traffic for the rest of the run, and each request rediscovered the same corpse. Selection now goes through HealthyShardSelector when fleet health is enabled: a quarantined shard is skipped, and least-outstanding steers away from one that is merely slow without that having to be diagnosed first. With no selector attached the historical round-robin is reproduced exactly, so an unconfigured run is unchanged. The other half is reporting. A dead worker surfaces as ray.exceptions.RayError, which is now fed to the monitor before being re-raised as GenerationUnavailable. Reporting is what lets the *next* request skip the shard; the retype is what tells the rollout retry policy the prompt is fine and worth re-dispatching elsewhere. Together they close the loop that P0.4 could only half-close -- re-dispatch already existed, but nothing steered it away from the failure. The generation body moved into _generate_on_shard so the acquire/release of the in-flight count can sit in a finally around the whole stream. A leaked count would permanently bias selection away from a shard that is actually fine. setup_single_controller builds the monitor when async_rl.fleet_health.enabled and hands it to the SingleController through actor args; a backend without attach_fleet_health raises rather than silently ignoring the request. Nothing drives the probe loop yet, so detection is currently failure-driven only. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Signed-off-by: asolergibert <asolergibert@nvidia.com>
…reeze gap
Wires the P1.1 monitor into the SingleController and adds the external observer
the in-actor watchdog cannot be.
The watchdog tick now probes every serving shard for Ray actor liveness, folds
the result into the monitor, publishes fleet state, and raises
GenerationFleetExhausted once too few shards remain to be worth continuing. Only
serving shards are probed: a quarantined shard answering again says nothing about
whether its weights are current, and the monitor ignores such probes anyway. Ray
liveness is the cheap authoritative signal for "the process is gone" but misses a
vLLM engine core dying under a live worker, which is why the routing adapters
also report what they observe -- both feed the same counters.
Two loop-freeze fixes, which is the residual risk once the monitor lives on the
SC actor:
- invalidate_kv_cache moves to asyncio.to_thread. It was the one call into the
workers made directly on the event loop; run there against a wedged worker it
would freeze the loop itself, taking the watchdog -- an asyncio task on that
same loop -- down with it.
- The driver polls ping() around the run. The in-actor watchdog cannot observe
its own loop being blocked, and the driver is already a separate process
holding the handle, which makes it the cheapest possible external observer.
ping() has existed since the SC landed and had no caller until now.
Environment teardown is also bounded, with a ray.kill fallback, matching what the
legacy GRPO path already does. It runs in a finally block, so a hung shutdown
would otherwise replace a real training error with an indefinite wait.
Validated on 2xA6000 by the chaos harness with fleet health enabled: the monitor
takes shard 0 healthy -> suspect -> dead and the job stops 10s after the kill
with both GPUs released. Two caveats worth stating rather than implying
otherwise. The threshold was crossed largely by adapter-reported failures rather
than by probes, since the rollout path reaches the dead shard before the third
probe lands. And with 1 training + 1 inference GPU dp_size is 1, so there is
nowhere to fail over to -- what P1 actually buys, traffic moving to a surviving
shard, is covered by unit tests only and needs >=3 GPUs to show end to end.
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Signed-off-by: asolergibert <asolergibert@nvidia.com>
NeMo-Gym picks a policy endpoint by static round-robin over a list fixed at
process start, never fails over, and retries a refused connection in an uncapped
loop with no HTTP timeout. A dead vLLM endpoint therefore keeps receiving ~1/N of
new rollouts for the rest of the run.
Rather than change Gym, hand it a single URL NeMo-RL owns. Gym's
VLLMModelConfig.base_url accepts one string, so its round-robin becomes a no-op
and the routing decision moves next to the fleet health that already knows which
shards serve. The switch is one expression in setup_single_controller.
Three decisions carry this:
- The URL never changes. The port is reserved once and passed in, so Ray
recreating a restarted actor rebinds the same address. Gym is never
reconfigured and never has to fail over, which matters because failing over
is exactly what it cannot do. If the router picked a fresh free port on
restart -- the way everything else here allocates ports -- Gym would hold a
dead URL forever, since it never re-resolves.
- Every piece of state is built in __init__, including the server thread. A
restarted actor is immediately usable. This is the deliberate inverse of the
NemoGym mistake, where servers were started from a _spinup that Ray never
re-runs.
- The no-healthy-backend status must stay outside Gym's retry set. Gym retries
429/500/502/503/504/520, and for the rate-limit subset it raises its own
retry ceiling per attempt, so answering with one of those would spin forever
-- recreating the hang the router exists to prevent. It defaults to 409 and
the config validator rejects the retried codes outright rather than leaving
it to be discovered in production.
Not a redirect, though aiohttp does follow 307 with the body intact. A redirect
puts Gym's socket back on a vLLM endpoint directly, so a backend dying
mid-request drops it into that same uncapped retry loop.
Membership is pushed from the SingleController watchdog as the full serving set
whenever the fleet's membership epoch moves. Full sets rather than deltas mean a
dropped, reordered or post-restart update converges on the next tick without
sequence numbers or replay. A restarted router comes up believing every backend
serves, which is self-correcting and strictly better than serving nothing.
Bodies stream through in both directions rather than being buffered; a completion
carrying per-token logprobs is large and this sits on every rollout's critical
path.
Tested against real aiohttp servers rather than fakes -- header handling,
streaming and status propagation only misbehave over an actual socket -- plus a
live Ray-actor check confirming the proxy, the 409 drain path and the metrics.
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Signed-off-by: asolergibert <asolergibert@nvidia.com>
policy_router.enabled was enabled by no test anywhere. The branch's other three features ride on the chaos test, whose branch-2 version turns on fleet_health.enabled, but the router did not -- so a regression in the proxy that fronts every NeMo-Gym rollout would have shipped silently. Registers the existing Gym functional test a second time with the router and fleet health on. gen_kl_error is what earns its keep here: it compares vLLM's logprobs against the trainer's recomputation, so a proxy that corrupts or truncates a response blows it up. A run that merely completes would not prove the payload survived the extra hop. Verified on 2xA6000 before registering: exit 0, reward 0.5, Gym's only endpoint the router on :6081 while vLLM served on :3038, and the resolved config showing PolicyRouterConfig(enabled=True) -- an override for a field that does not exist is accepted silently, and the test would then pass while exercising nothing. Full mode only; it is a second ~20 minute Gym run. Signed-off-by: asolergibert <asolergibert@nvidia.com>
Same rule as the fix on the containment branch: ruff's `I` rules are only selected by the second ruff hook in .pre-commit-config.yaml, not by a plain `ruff check`, so these were invisible locally and failed in CI. Three orderings: third-party `ray` before `torchdata`/`transformers`, `policy_router` after `interfaces`, and a stray blank line splitting an import block. Signed-off-by: asolergibert <asolergibert@nvidia.com>
probe_interval_s was decorative. The probe ran inside _watchdog_pump, so its real cadence was watchdog.interval_s (30s), and nothing anywhere read the configured value -- it existed only to be validated against probe_timeout_s. That put dead-shard detection at unhealthy_threshold * 30s = 60-90s, against a config comment promising ~15s. Worse, it is longer than refit_timeout_s: a refit hung on a dead rank always aborted before the monitor had decided which rank was gone, so the rebuild the abort exists to trigger saw an empty absent set and did nothing, and the retry died on a communicator that had been aborted and never rebuilt. Arithmetic rather than a race -- it could not have worked. Job 5925668. The probe now runs in its own pump at probe_interval_s, created only when fleet health is enabled so a default run gains no timer. Shards are probed concurrently: sequentially a round costs probe_timeout_s per shard, so any fleet larger than probe_interval_s / probe_timeout_s could not finish a round inside its own interval, and the config validator compares only those two -- silently assuming one probe per tick. The router membership push moves with it, so a membership change reaches the router at detection speed rather than on the watchdog's clock. It is epoch-gated, so an unchanged serving set still costs nothing. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Signed-off-by: asolergibert <asolergibert@nvidia.com>
Elastic recovery rebuilds the refit communicator whenever the generation
fleet's membership changes, so init_collective goes from running once per
job to running once per recovery. Two things had to change before that is
safe.
Add StatelessProcessGroup.abort(). It is idempotent, safe on a group whose
communicator was never built, and drops its reference *before* calling
abort so a failed release cannot leave broadcast() pointing at a dead
communicator. abort(), not destroy(): NCCL documents destroy as an
intra-node collective that every rank must call or it hangs, which is
precisely what a rank whose process has died cannot do. Verified on
2xA6000 -- with a peer SIGKILLed mid-broadcast, a survivor blocked in the
collective was released 0.15s after another thread called abort().
Release the previous group on both sides of the refit before rebuilding.
Both init_collective implementations previously overwrote
self.model_update_group outright, stranding the old NCCL communicator and
its TCPStore. That is invisible in a one-shot job, which is why it
survived until membership became dynamic, and unbounded once recovery can
repeat.
model_update_group is now declared on both classes instead of springing
into existence on first assignment, so a rebuild can test for a previous
group without probing for the attribute. That also removes a
pyrefly ignore[implicitly-defined-attribute] on the vLLM side.
broadcast() now raises with a diagnostic instead of an AttributeError
when the group has no communicator -- the failure a rebuild bug produces.
Verification: 8 new process-group tests; 30/30 in the vllm lane including
2 new init_collective tests; ruff clean; pyrefly errors drop 9 -> 7 (the
7 remaining are missing optional imports, fastokens and awscrt, in files
this commit does not touch). test_vllm_generation.py was run with and
without this change and is identical at 9 failed / 38 passed / 3 skipped
-- those failures are a pre-existing vLLM/dynamo engine-init issue on
A6000 ('QKVParallelLinear' object has no attribute 'workspace'),
structurally upstream of anything this commit changes.
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Signed-off-by: asolergibert <asolergibert@nvidia.com>
A NCCL broadcast needs every rank in the communicator to take part, so
when a generation rank dies the refit blocks forever inside NCCL: no
exception, no progress, and Ray still reporting every actor healthy. That
silent wedge is the failure this effort exists to remove. This commit
converts it into a precise error; rebuilding over the survivors, which
turns the stop into a recovery, is the next step.
Add WeightSynchronizer.reconcile_communicator(absent_shards). It is
non-abstract and defaults to a no-op, so the transports that own no NCCL
world of their own -- IPC, HTTP, checkpoint-engine -- are unaffected. The
two NCCL transports refuse the refit when a rank is missing.
Call it from the top of _sync_weights. Reconciling on a schedule rather
than on a death event is idempotent and converges after a missed or
reordered health update, and that point is the only one where the refit
group is provably idle and every rank is synchronized -- which matters
because the operations that change membership are themselves collectives.
"Absent" is deliberately not the complement of "serving". A SUSPECT shard
is failing probes but not yet condemned, and a STALE shard has reloaded
and holds old weights; both are withheld from traffic, and both processes
are alive and join a refit normally. Reading the serving set would abort
a run on a single probe blip, and would abort it precisely when a STALE
shard is waiting to be refit -- which is the recovery, not the failure.
GenerationFleetMonitor.absent_shards() therefore uses its own state set,
{DEAD, RESTARTING, RETIRED}.
The two transports raise different messages on purpose. The plain
broadcast could in principle drop a receiver, but nccl_reshard cannot:
prepare_nccl_reshard_refit_info derives each parameter's destination
placements from gen_world_size, so resizing without regenerating the plan
would leave survivors holding slices nobody wrote -- silent corruption,
worse than stopping.
Inert by default: async_rl.fleet_health.enabled is false, so there is no
monitor, no notion of a shard being gone, and the transport keeps the
membership it was built with.
Verification: 18 new tests, including the cases pinning SUSPECT and STALE
as present and DEAD and RESTARTING as absent; 1259 passed / 7 skipped
across algorithms, experience, weight_sync, single_controller,
distributed, fleet health and the router; ruff clean; pyrefly unchanged
at the same 7 pre-existing missing-import errors in files this commit
does not touch. Three existing _sync_weights tests build their controller
by hand and needed _fleet_monitor added.
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Signed-off-by: asolergibert <asolergibert@nvidia.com>
…ards Turns the previous stop into a recovery. When a generation shard dies the refit communicator still contains its ranks, so the broadcast blocks forever inside NCCL. P3a.2 detected that and failed loudly; this rebuilds the communicator without the dead ranks so training continues on what is left. Rebuild rather than shrink. The pinned NCCL runtime exports ncclCommShrink but not ncclCommGrow, so a shrunk world could never take a recovered engine back -- one mechanism that works in both directions beats two that each work in one. It is also what nccl_reshard will need, since that transport must regenerate its refit plan rather than resize. The rank arithmetic is a pure function in weight_sync/membership.py, separate from the Ray dispatch that applies it, because it is the part that has to be exactly right and the part that cannot be exercised here: observing a real shard loss needs at least three GPUs, so that losing one still leaves a fleet. An off-by-one does not crash, it points a receiver at the wrong slice of the broadcast. Survivors are compacted to contiguous prefixes rather than leaving a hole where the dead shard was. Not cosmetic: the nccl_reshard destination mesh is torch.arange(offset, offset + num_gpus), so a gap would misalign every parameter's placements. It also matches what shrink does to a live communicator, so both paths describe the same world. VllmGeneration.rebuild_collective addresses the surviving DP leaders directly instead of going through run_all_workers_multiple_data, which walks every worker in the group and would therefore dispatch to the shard we are rebuilding *because* it is gone. Only leaders are called; each collective_rpcs into its own TP/PP workers. Trainers are never excluded, so rank 0 stays a trainer and the broadcast root is stable across a rebuild. Each rebuild takes a fresh port, since the previous world's rendezvous store may still be bound, and StatelessProcessGroup.abort() now releases that store so repeated recoveries do not accumulate one per recovery. nccl_reshard still refuses, with a message pointing at the transport that does recover. Regenerating its plan is the next step. Adds tests/functional/grpo_sc_generation_shard_recovery.sh: kills one of two generation shards mid-run and asserts the job completes all steps, that the rebuild actually happened, and that the metrics are intact -- completion alone would also be satisfied by a run that never noticed the death. It needs >= 3 GPUs and self-skips below that rather than passing vacuously, and is registered in the SingleController lane in full mode. Verification: 36 new unit tests (18 on the rank layout alone); 801 passed across weight_sync, single_controller, distributed, experience, fleet health and the router; ruff clean; pyrefly back to the same 7 pre-existing missing-import errors with membership.py added to scope. End-to-end recovery is NOT verified here -- it needs the >= 3 GPU functional test. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Signed-off-by: asolergibert <asolergibert@nvidia.com>
…d one A frozen-but-alive rank is a Ray actor that never answers, and Ray puts no timeout on an actor call. So the controller's await of sync_weights never resolves no matter how well every worker behaves. Job 6508251 measured that end state on 4xGB200. The deadline fired, the workers aborted, the trainers returned, every actor was idle at the 1800s dump -- nothing blocked in NCCL or CUDA anywhere -- and the run still sat until the harness killed it, because this await had no bound. `_sync_weights` logged "refit membership absent=[] rebuilt=False" and never logged "sync done"; the recovery never ran; the pump held 0 rollouts in flight for 1505s. Both refit awaits now go through _sync_weights_within, which times out and raises RefitAborted so this joins the existing recovery path rather than inventing a second one. The caller reconciles membership and retries once, and by then the probe has usually condemned the silent shard -- so the rebuild can exclude it and the retry may actually succeed rather than merely failing faster. Budget is refit_timeout_s plus a grace, so the workers' own attributable error wins the race; if this fired first we would report only "the refit never came back", which is true and less useful. refit_timeout_s of None keeps the await unbounded, so a run that configures no deadline behaves exactly as before. A DEDICATED DAEMON THREAD, not asyncio.to_thread: to_thread runs on the default ThreadPoolExecutor, whose workers are non-daemon and joined at interpreter exit, so a thread still parked on the frozen actor would hang shutdown -- trading a wedge in the refit for a wedge on the way out. wait_for cannot cancel a running thread either way; this only controls whether the orphan can block exit. The orphan is a real consequence and is documented on the method, not glossed. Eight tests. The fixture now carries refit_timeout_s rather than the code reaching for it with getattr -- a stand-in for a constructed actor has to carry every field the methods under test read, and the missing field broke every pre-existing test in that file until it was added. Fifth and last unbounded wait on this path. The four before it -- the defaulted-off deadline, the unnamed abort on reshard, the type lost across vLLM's RPC, the unbounded CUDA sync -- were each real and each uncovered this one. Signed-off-by: asolergibert <asolergibert@nvidia.com>
…ild can be serviced Job 6509685 got further than any run before it: the controller's bound fired at 72.5s, the probe condemned the silent shard, and the reconcile rebuilt -- "rebuilding nccl_reshard communicators without shards [0]; gen world 1". Then it failed anyway, with the surviving generation worker timing out for 300s, twice, dialling a rendezvous store that was never created. The trainers are why. Their last line is their own abort; they log nothing afterwards. The rebuild's init_collective was sent to those same actors and queued behind the refit still running on them, so rank 0 -- the store master -- never created the store. The reviewer predicted this exactly, on config.py:302: "_recover_from_failed_refit rebuilds, which calls init_collective on those still-blocked trainers. That call queues behind the blocked task and never runs." It was hidden behind five other layers. Raising max_concurrency does NOT fix it, which is what I set out to do. Ray runs a sync actor method directly in the event loop -- sync_to_async wraps it as `async def wrapper: return func(...)`, with no executor -- so the refit occupies the loop itself. max_concurrency interleaves coroutines, and a coroutine blocked in C never yields. The docstring on this very method already said "the controller cannot reach this actor to break it because its event loop is inside the transfer"; I had written the mechanism down and not connected it to the recovery call. So nccl_reshard_refit is now async and hands the blocking transfer to a daemon thread. The loop stays free, and this actor can service init_collective while the old refit is stuck -- which is the property the rebuild depends on. Daemon for the same reason as elsewhere: asyncio.to_thread's default executor is non-daemon and joined at interpreter exit, so an orphan parked in NCCL would hang shutdown. No timeout here; bounding the wait is the controller's job. Callers are unaffected -- Ray dispatches async actor methods through .remote() identically. Four tests, the load-bearing one asserting the loop still services work while the blocking call runs, plus an AST guard that this entrypoint stays async and delegates. The helper lives in refit_watchdog because MegatronPolicyWorker cannot be imported outside its venv. Scope: the collective sibling has the same shape but recovers today (job 6428488 ends in 40s and never needs the rebuild), so it is left alone. Signed-off-by: asolergibert <asolergibert@nvidia.com>
f0b4e98 moved the reshard refit off the actor's event loop and broke the healthy path doing it. Job 6510914 died on its very first refit, before any fault was injected: ray::MegatronPolicyWorker.nccl_reshard_refit() nccl.bindings.nccl.NCCLError: UnhandledCudaError (1): unhandled cuda error CUDA's current device is thread-local. A fresh thread starts on device 0 rather than this worker's, so NCCL ran against the wrong GPU. torch.cuda.current_stream() inside the transfer reads the same thread-local state, so the transfer was also on the wrong stream. The device is now read on the loop thread, where it is correct, and applied on the worker thread before the transfer starts. Worth stating plainly: moving GPU work to another thread has a wider surface than I judged when I proposed it, and this is the second mechanism I got wrong in this sub-task -- the first being max_concurrency, which cannot work at all. If the next run still fails inside the refit rather than at the fault, the right move is to revert the off-loop change and keep the bounded-failure behaviour from 6509685 (690s, attributable, recovery reached) rather than keep pushing on a change with this much reach. Signed-off-by: asolergibert <asolergibert@nvidia.com>
**Dead code that asserted the opposite of the live check (mine).** The FREEZE_VICTIM block at :528 terminates on every path -- four `exit 1` and an `exit 0` -- so the second FREEZE_VICTIM block below it and the frozen branch of the final PASS were unreachable. They also claimed "the run survived" and "finished all N steps", which is precisely what that variant does not do; leftover from before the variant was consolidated. Twenty lines deleted and the PASS collapsed to its one live branch. Worth doing here rather than later: #3592 edits a string inside the region that was dead. **getattr probing where the attribute is declared.** `pp_comm_group` is a class attribute on AbstractPolicyWorker, which MegatronPolicyWorkerImpl inherits, so `getattr(self, "pp_comm_group", None)` could never take its default. Verified at runtime, not just read. Same smell already removed for `_refit_membership` in 8779394. **A forward reference that outlived the thing it pointed at.** "Recovery modes arrive with the communicator rebuild" sat on `on_dead_shard` in both the config and the exemplar. The rebuild has since landed, and recovery is not selected through that field at all -- the reconcile rebuilds over the survivors whenever a shard becomes absent, whatever it says. **An undocumented parameter among documented siblings.** `refit_timeout_s` was missing from the factory's Args block. The reviewer also expected it missing on both synchronizers; it is already documented there, so only the factory needed it. Found by review on #3591. Signed-off-by: asolergibert <asolergibert@nvidia.com>
…a shard dies absent_shards() never empties again -- nothing in production calls mark_restarting or mark_loaded -- and _sync_weights reconciles twice per step. Neither synchronizer remembered what it had last built with, so both rebuilt unconditionally whenever the absent set was non-empty. A run that lost a shard at step 10 and trained to 10,000 paid roughly 20,000 full rebuilds: a fresh port, a fresh TCPStore and a fresh NCCL bootstrap across every train and inference rank each time, plus a plan regeneration on nccl_reshard. The steady state this feature exists to produce was the expensive one. Each synchronizer now records the absent set its current communicator was built with and skips when that has not changed. Recorded after the rebuild, not before, so a rebuild that raises leaves the cache describing the communicator we still have. Compared as frozensets, because absent_shards() returns a sequence and the same loss in another order is the same membership. THE RECOVERY MUST OVERRIDE IT, and this is the part that would have broken quietly. After an abort the absent set is identical and the communicator is gone, so skipping there would retry over nothing and fail with "no generation shard could be identified as absent" -- the recovery's own refusal message, on a path that had nothing wrong with it. _sync_weights' recovery call passes force=True; the two pre-refit reconciles do not, and there are tests for both directions. Three comments claimed this was already the behaviour. Two said the reconcile was "a no-op" in the common case; it was idempotent but never cheap. The third, on membership_epoch, said "the weight-sync path compares this against the epoch its communicator was built with" -- that comparison did not exist anywhere, and grep for membership_epoch returned only fleet_health and its own test. That comment now says what is true: the epoch is a metric, nothing reads it, and the skip lives on the synchronizer because the synchronizer is what knows which membership its communicator was built with. Mutation-checked in both directions: removing the skip fails the two tests that assert it, and ignoring force fails the two that assert the recovery still rebuilds. Found by review on #3591. Cost derived from the code path; not measured on hardware. Signed-off-by: asolergibert <asolergibert@nvidia.com>
…un over it is_alive() is `return True`. It is answered by the Ray actor and never touches the engine, so the post-abort probe can only ever see a dead PROCESS. An engine that is wedged with its actor healthy is invisible to it: never absent, never rebuilt around, and the recovery gave up on it -- the one failure mode most likely to break a refit in the first place. It loses the race to the other detector too. report_failure condemns only after unhealthy_threshold (3) consecutive generation timeouts at 900s each, up to 45 minutes, and report_success clears the streak -- so an intermittently-working engine may never reach DEAD at all, while refits run every training step. The evidence was already in the ledger and was being discarded. That shard's own generations have been timing out, so it sits in SUSPECT while the fleet is otherwise HEALTHY. The abort is independent confirmation that something stopped participating; SUSPECT says which. The recovery now condemns that shard, re-reconciles, and continues. Exactly one, or not at all. With several suspects there is no way to tell which broke the collective, and condemning the wrong one costs a healthy shard AND leaves the real culprit inside the rebuilt communicator -- the same hang, one shard smaller. With none there is nothing to go on. Both keep the refusal, and its message now names what it found so a reader can tell those two cases apart. The suspicion has to be read back out of state_before_partial, which is the part that is easy to get wrong. Step 2 of the recovery marks every SERVING shard's weights partial, and SUSPECT is a serving state, so the suspicion is overwritten by STALE microseconds before anyone asks. That field was added for a different reason -- the laundering bug in 2de01f5 -- and turns out to be what makes this attributable at all. condemn_silent_participant is deliberately not record_actor_death: the latter means "Ray says the process is gone", which is exactly what is not true here. This is a judgement and the ledger records it as one. REVERSES WHAT THE FROZEN VARIANTS ASSERT. They were written around the old limit -- a frozen rank ends the run -- which job 6511119 measured as a pass 50s after the freeze. They now assert completion by attribution instead. That is a prediction, not a measurement: the next cluster run decides whether the retry genuinely carries the run. Design doc updated to v13 (section 8.5.4) with the same caveat stated. Mutation-checked: disabling the single-suspect rule fails the two tests that assert condemnation and leaves the three that assert the refusal passing. Found by review on #3591. Signed-off-by: asolergibert <asolergibert@nvidia.com>
…conditions on SC Two findings on the same transport, both pre-existing and both first reachable here. **The rank offset was added twice.** init_nccl_reshard_comm_group computed `train_ranks_per_stage + rank_prefix + get_rank()`. Under vLLM's external data parallelism each engine's torch world spans the whole rollout, so get_rank() is already global and the prefix counts the shard offset a second time -- the higher shards get NCCL ranks past the end of the group. init_collective, forty lines up, resolves the same thing through resolve_rollout_rank, whose own comment says why: "External DP ranks are already global; adding the prefix would double-count." Without external DP the two forms agree, which is how this survived: each engine's world is tp x pp, get_rank() indexes within the shard, and the prefix is exactly what is missing. It bites at the initial build, before any recovery, whenever expert_parallel_size exceeds tensor_parallel_size. **The precondition guard never ran here.** check_nccl_reshard_refit_support validates every nccl_reshard precondition that can be decided from config, and had exactly one production caller: grpo.setup. run_grpo_single_controller goes straight to setup_single_controller, so on this path none of it was enforced -- colocated.enabled, enable_eplb and the rest reached the first refit before anything noticed. This PR makes that worse rather than better, since recovery rebuilds the reshard communicators and hands a bad config a second, later chance to fail. Called after validate_single_controller_config so the SC-specific errors a reader is likelier to have caused are reported first. Two AST tests, because both defects are "this call site does not go through the helper the other one does" and that is exactly what AST can see. Found by review on #3591. Signed-off-by: asolergibert <asolergibert@nvidia.com>
… that was missed Two latent findings, neither reachable on a config that exists today, both one line. **The rebuild bootstrapped with the wrong protocol.** init_communicator passes nccl_peer=sender_spec.nccl_peer; reconcile_communicator did not, so a rebuilt communicator silently fell back to the "nemo" default. The receiver's bootstrap is not negotiable -- "nemo" publishes a raw unique ID and warms up with a rank-0 broadcast, "vllm" adds a pickled ID key and warms up with an all-reduce -- and mismatched warmups on one communicator HANG rather than error. That is precisely the failure this path exists to remove, reappearing inside the recovery. Unreachable today only because the one backend reporting "vllm" is Dynamo, which SC setup rejects outright. **The second generation refit hook never got the deadline.** This stack widened nccl_reshard_refit on the policy interface and update_weights_from_collective on the generation one, writing the rule into the latter's docstring -- "a backend that omits the parameter does not fail at import or type-check time, it fails at the Ray boundary during the first refit" -- and then left GenerationInterface.nccl_reshard_refit at (self). The rule was stated, applied twice, and missed once. Both are guarded by AST tests, because both defects are "this call site does not go through what the other one does", which is what AST can actually see. Writing the first of those found a third thing: matching on the method name alone compares the two SIDES of the reshard refit against each other, and they legitimately differ -- kv_scales is read off the trainer and rides the misc broadcast, so the generation side never sees it. The helper is scoped by receiver, with that written down. The reconcile fakes now carry get_collective_sender_spec and accept nccl_peer. A stand-in has to carry every hook the code under test reads, or it tests a shape the product never has -- the third time that has come up in this stack. Found by review on #3591. Signed-off-by: asolergibert <asolergibert@nvidia.com>
…is a streak again consecutive_reported_failures is the only counter that can condemn a wedged engine, and it is meant to be a STREAK. On the NeMo-Gym router path nothing ever cleared it. report_success has exactly one caller -- the native adapter -- and the router only ever counted failures, so the count was monotonic and every shard reached unhealthy_threshold eventually, however healthy it was. The reviewer's breaking case, now a test: three transport blips days apart against unhealthy_threshold=3, each in a window that also served thousands of requests. The streak walks 1, 2, 3 and condemns a shard that was healthy throughout. report_success's own docstring names the hazard -- "without it the reported streak is monotonic and every shard eventually reaches unhealthy_threshold given a long enough run" -- and that was the router path's behaviour. Pre-existing from #3590, but this stack is what makes it expensive. There, DEAD meant "stop routing here". Now it also means absent_shards() drops the shard from the rebuilt refit communicator, and #3592 restarts and re-admits it, so a false condemnation costs a restart and a full refit rather than a routing change. drain_backend_failures becomes drain_backend_outcomes and hands over (successes, failures) together. Not two calls: the ledger needs both halves of the SAME window, and draining them apart would let the windows interleave and reintroduce this. Successes are applied first, because replaying a failure onto a streak that a success in that window should already have cleared is the bug. A genuinely wedged shard produces no successes, so its condemnation timing is unchanged -- there is a test for that too. Deliberately the success half, not a reset on a clean probe: #3590 kept the reported streak separate from the probe streak precisely because a wedged engine still answers is_alive. Mutation-checked: ignoring successes fails both the three-blips case and the ordering-within-a-window case. Found by review on #3591. Signed-off-by: asolergibert <asolergibert@nvidia.com>
Contributor
|
/ok to test b8b69eb |
**WeightSynchronizer.reconcile_communicator never got `force`.** Both concrete synchronizers took it; the interface they implement did not, so pyrefly resolved the call through the interface and reported bad-argument-count. That is the same bug class as the finding fixed two commits ago -- a signature widened in some places -- committed in the change that fixed it. Noted in the design doc rather than quietly patched: it is the fourth instance in this stack and the pattern is evidently not self-correcting. **check_nccl_reshard_refit_support was annotated `dict` and reads `master_config.policy`.** The annotation was always wrong; nothing typed called it until the single-controller path did. Widened to Any, with the actual requirement written down -- an object exposing `.policy` as a mapping -- because grpo's MasterConfig and the SC one are different classes and there is no single concrete type to name. **Two files had unsorted imports.** Both are ones where I inserted an import by hand. The project's own `ruff check` does not select I; the pre-commit hook runs `ruff check --select I --fix` separately, so a clean local run says nothing about it. Worth knowing: the repo lint is four hooks, and only two of them are what `uv run ruff check` does. Local reproduction of the full lint now matches CI: ruff, ruff --select I, ruff-format and pyrefly all clean, except the pre-existing transfer_queue import error, which is an optional dependency absent from this container and installed on the runner. 717 tests pass. Signed-off-by: asolergibert <asolergibert@nvidia.com>
Fifteen commits, no conflicts. The ones adjacent to this work -- #3771 skipping reference logprobs when KL is disabled, #3599 on prompt skips across checkpoint restore, #3770's effort-level reward shaping in the SingleController -- all merged cleanly and the SC suites pass alongside them. 1065 tests pass; the one failure is the dev container's uid-1001 problem on megatron_core.egg-info, on a file this stack does not touch. Two lint notes for whoever runs this locally. pyrefly reports an import error for transfer_queue.utils.mooncake_utils, new from #2935: an optional dependency absent from this container and installed on the runner, so it is a local-only artifact. And `ruff format --check .` from the repo root walks into 3rdparty/, which reports an unformatted file inside the Automodel submodule; pre-commit only sees this repo's tracked files, so scope it to nemo_rl tests examples tools to match CI. Signed-off-by: asolergibert <asolergibert@nvidia.com>
…e recovery
Job 6512153 ran the full resiliency lane: 8 of 9 pass, including the reshard frozen variant
that 6511119 established. The one failure is recovery-reshard-refit -- kill mid-refit on
reshard -- and it is a regression from this stack.
The log gives the order plainly:
853 MegatronPolicyWorker[rank=0] deadline exceeded after 60.0s
961 RefitAborted: the bulk parameter transfer did not retire within 60.0s
963 refit: rebuilding nccl_reshard communicators without shards [0]
965 MegatronPolicyWorker[rank=1] deadline exceeded after 60.0s
1014 client socket has timed out after 300000ms
The rebuild starts at 963, two lines before rank 1's watchdog fires at all. ray.get raises
on the first future that fails and leaves the rest running, so the controller went into the
recovery holding rank 0's failure while rank 1 was still inside the old refit. A communicator
rebuild is itself a collective; dispatched at ranks that have not left the previous one, it
builds a rendezvous nobody joins. The surviving generation worker then spent 300s twice
failing to reach it and the run died at 690s having done everything else right.
So the failure propagates only once every train rank has settled. That is the actual
invariant -- a rebuild needs all of them -- rather than tuning a deadline so the race is
merely rarer.
Bounded at the ranks' own refit_timeout_s plus a margin, because a straggler gives up when
its own watchdog fires; past that it rebuilds anyway and says how many had not unwound. A
caller blocked here would be a worse wedge than the one being recovered from. Whatever the
stragglers raise is swallowed: they are unwinding from the same failure the caller already
holds, and replacing it would lose the diagnosis.
Applied to both transports. The collective path has the identical shape and has simply not
been unlucky yet -- it recovers via ActorDiedError fast enough that the race has not opened.
Six unit tests plus an AST guard on both call sites; mutation-checked.
Worth recording: the bounded CUDA sync that makes this race reachable was aimed at a frame
py-spy caught once, and never fired in the three runs after it. Now that it does fire, it
gives each rank an independent exit from a collective -- which is exactly what the rebuild
cannot tolerate.
Signed-off-by: asolergibert <asolergibert@nvidia.com>
…ccl_peer gap there Jobs 6512153 and 6513879 both die the same way: after an ABORTED reshard refit the rebuild runs, and the surviving generation worker then fails to CONNECT to the new store for 300s, twice. The same rebuild works after a clean kill -- recovery-reshard passes in both runs -- so the fault is specific to rebuilding over an aborted state. A connect timeout names only the address. It does not say whether the master never bound the port, or whether the two sides disagreed about world_size or rank; the party that got it wrong simply waits. I have now proposed two mechanisms for this exact failure -- an init_collective queued behind a busy actor, then rank desync -- and both were wrong. So this prints what each side was actually handed, on both sides, and the next run answers it instead of me. Also closes the nccl_peer gap in THIS synchronizer. The reshard rebuild's init_collective never passed it, so it bootstrapped with the "nemo" default exactly as the collective one did before that was fixed -- and mismatched warmups on a communicator hang rather than error. Whether it is what breaks this rendezvous is unproven; it is a real gap either way, and if the instrumentation shows both sides agreeing on address, rank and world_size then this becomes the leading candidate. That gap is the fifth instance of the same bug class the design doc records in 8.5.5, and it survived because the AST guard written for the fourth instance checked only collective_weight_synchronizer.py. It is parameterised over both now. Fourth time a test fake has been missing a hook the code reads -- the reshard fake's own comment already said the lesson about _refit_membership, for the same reason. Signed-off-by: asolergibert <asolergibert@nvidia.com>
Job 6517523 answered the question it was built for, by absence: on a rebuild after an
aborted reshard refit, neither the [train] nor the [gen] reshard-rendezvous line appears,
and "nccl_reshard bulk comm group IPs/ports" prints once for the initial build only. The
bulk group is never reached. The failing port (27342) is not the reshard group's (27398).
So the rebuild dies one step earlier, in the shared model_update_group -- the same
init_collective the collective transport uses and rebuilds successfully. My instrumentation
was aimed one function too late.
Three points in the chain now print: the synchronizer's dispatch, the trainer's
init_collective, and the generation side's. That distinguishes the three cases a 300s
connect timeout cannot:
no dispatch line -> the rebuild never got this far
dispatch but no [train] -> the call never reached the trainer; rank 0 is this store's
master, so nobody bound the port
both, values disagree -> a membership mismatch, and the line says which field
Also rules in/out what the last round could not: peer= and master= are printed, so a
"nemo"/"vllm" mismatch or a master that is not rank 0 is visible rather than inferred.
Ruled out so far, all by measurement rather than argument: trainers still busy (both
reported did not retire, and the settle found none pending), rank or world_size mismatch
(every party agreed on the initial build), and nccl_peer (6517523 ran with that fix).
Not a regression from this stack: recovery-reshard, which rebuilds after a clean kill with
no abort, passes. Rebuilding after an ABORT on this transport has most likely never worked.
Signed-off-by: asolergibert <asolergibert@nvidia.com>
Job 6518381 finally produced a real log, and it says the opposite of what the last three rounds concluded. On the rebuild every party agreed -- two train ranks plus the survivor, ranks 0/1/2 of world_size 3, one port, master on rank 0 -- and the store was never bound: dispatching model_update_group rebuild addr=10.109.22.117:25669 world_size=3 train_world_size=2 [gen] addr=...:25669 rank=2 world_size=3 prefix=0 [train] addr=...:25669 rank=0 world_size=3 master=True [train] addr=...:25669 rank=1 world_size=3 master=False [rank0] client socket has timed out after 300000ms while trying to connect to (...:25669) StatelessProcessGroup binds with is_master=(rank == 0). rank 0 printed its line, so it was inside the function, and exactly one statement sat between the print and the bind: the previous group's abort(). abort() calls abort_xferdtensor_python_subcommunicators before the parent, and those split children exist ONLY on the Python reshard path -- on the packed-broadcast path that call finds no cache entry and returns at once. ncclCommAbort joins the communicator's proxy thread, and a proxy thread blocked reading from a SIGSTOPped peer never returns: the socket is open and idle, so nothing errors and nothing times out. SIGKILL closes it and the proxy errors out immediately, which is why every killed variant passes and why recovery-reshard has always passed. Rebuilding after an ABORT on this transport had most likely never worked. So: bind first, release afterwards, and bound the release. rank 0 is the store's master, so every other rank is already spending a 300s connect budget against a port it has not bound yet; anything slow that runs first is spent from that budget, and this one was not slow but infinite. release_within runs it on a daemon thread that is deliberately never joined -- asyncio.to_thread and a bare ThreadPoolExecutor both use non-daemon threads the interpreter joins at exit, which would move the wedge to shutdown rather than remove it. All four rendezvous sites had the same bare abort() before the build: both init_collective implementations and both init_nccl_reshard_comm_group implementations. Three were found only because the fourth was, so the contract test is parametrised over every site rather than over the one that failed. Alongside, the same bug class one level up: sync_weights settled only futures_train on failure and left futures_inference running, in BOTH synchronizers, while the comment above it stated the invariant for every rank. The rebuild dispatches init_collective to the generation side too. Not a regression from this stack. recovery-reshard, which rebuilds after a clean kill with no abort, has passed throughout. Design doc: section 8.5.7 (v14), and two new rows in section 8.5.5's table. Signed-off-by: asolergibert <asolergibert@nvidia.com>
…t relabel it Job 6521181 held past the 1800s deadline for the first time and produced a py-spy dump. Both trainers: initialize (nccl/core/communicator.py:442) init_nccl_communicator (stateless_process_group.py:202) init_collective (base_policy_worker.py:91) with two unnamed, frame-less threads apiece -- the abandoned ncclCommAborts from release_within, still in native code 25 minutes later. NCCL cannot bootstrap a new communicator on a device while a previous one there is wedged mid-abort, and that abort cannot finish while the frozen peer holds its sockets open and idle. The previous commit's bounded release moved the wall by exactly one statement. Nothing that leaves the victim running moves it further. The gap is that condemning a shard changed bookkeeping and nothing else: the fleet said DEAD while the process sat there. That is the entire frozen/killed asymmetry these tests have been circling -- SIGKILL closes sockets, SIGSTOP does not, and recovery-reshard (the killed variant on this exact transport) has passed throughout. So make the verdict true. _evict_absent_but_alive ray.kills every worker of a shard that goes absent while its process is still up, before the rebuild is dispatched -- before, because the rebuild is what the survivors are blocked in. Its sockets close, every peer's pending abort retires, and the rebuild proceeds down the path the killed variants prove. no_restart, because Ray restarting the actor underneath us would readmit a shard the fleet has just declared absent; readmission is a deliberate decision made elsewhere with a weight version attached, not something to acquire by accident. Best-effort and idempotent: a shard already gone raises, and that is the outcome this wanted -- the eviction must never be the reason a recovery fails. Two fixtures grew the fields the new path reads rather than the path growing getattr, which is the rule this stack has now applied five times. Design doc: section 8.5.7 (v14). Signed-off-by: asolergibert <asolergibert@nvidia.com>
asolergi-nv
force-pushed
the
feat/sc-resiliency-03-elastic-recovery
branch
from
August 25, 2026 21:49
1647e26 to
02b7dd0
Compare
…viction Reverts 2f762b8. The eviction was aimed at a mechanism that turned out to be wrong, and job 6523731 is the disproof: ray.kill reached the victim before the rebuild was dispatched, in the right order, and both trainers wedged in init_nccl_communicator exactly as they had without it. A product behaviour change that kills a live worker has no business staying in on the strength of a theory its own experiment refuted. The real reason is in sync_stream_within, and it was written there before any of this started: "In-flight kernels are orphaned and the caller's CUDA context should not be trusted afterwards, so the RefitAborted raised here is expected to end the run ... Recovering a frozen-but-alive rank on this transport stays out of scope." Aborting a communicator does not retire work already enqueued on a CUDA stream -- the same property sync_stream_within exists to bound. The orphaned kernels are on the TRAINERS' devices, so nothing done to the remote rank retires them, and no further bound can either. Job 6521181's py-spy dump shows where that lands: both trainers inside init_nccl_communicator, each with two frame-less threads still in ncclCommAbort 25 minutes on, unable to bootstrap a new communicator on a device holding a half-aborted one. So recovery-reshard-frozen was asserting a property the design places out of scope. The frozen expectation is now split, because the two transports genuinely differ: packed broadcast condemned suspect is recoverable -> the run CONTINUES, by attribution nccl_reshard bulk abort orphans trainer kernels -> the run ENDS attributably, < 900s The reshard branch still requires RefitAborted (so a run that dies of something else is not mistaken for a pass), still requires the victim to be genuinely frozen at the end, and adds an elapsed bound -- because wedging for the full harness timeout would otherwise satisfy a non-zero exit check and read as a pass, which is the failure this whole line of work replaced (job 6258553 sat in NCCL for 33 minutes). What the six bounds bought on this transport is not survival; it is a named cause in seconds. That distinction was never tested until now, and is the thing worth carrying. Design doc: section 8.5.7 rewritten (v14). Signed-off-by: asolergibert <asolergibert@nvidia.com>
Job 6524733 ran the full lane: 11/14 pass, three fail, for three different reasons.
1. recovery-reshard-refit REGRESSED from passing to a 1800s wedge, and it is the KILLED
variant, not the frozen one:
gen_fleet: shard 0 healthy -> dead (ActorDiedError: The actor died unexpectedly...)
refit: dispatching model_update_group rebuild ... world_size=3
rank=1 the previous refit communicator did not release within 30.0s
rank=0 the previous refit communicator did not release within 30.0s
The victim was SIGKILLed -- genuinely gone -- so the abort had no peer to wait on, and
it still did not return. Before release_within, that same abort() returned fine.
The difference is the thread. The CUDA device is thread-local and a fresh thread starts
on device 0, so the abort was running against the wrong device. This is the same trap
that cost job 6510914 a run when the refit first moved off the event loop, fixed there
by capturing torch.cuda.current_device() on the caller and re-setting it in the worker
thread; megatron_policy_worker asserts against exactly this drift after setup. The fix
went to await_off_loop and not to release_within.
It also settles the mechanism for good: a SIGKILLed peer closes its sockets, so the
abort never had anything peer-side to block on. Whatever remains is local, which is what
sync_stream_within has said since it was written.
2. recovery-frozen failed on an assertion, not on behaviour. The run recovered correctly --
exit 0, 420s -- but by way of "shard 0 suspect -> dead (TimeoutError)" rather than the
condemn. Both routes are correct and which one runs is a race: the condemn is only
needed when the shard never becomes absent on its own, and here its own probe got there
first. The check demanded one route's log line, so it had been passing by luck. It now
accepts either, while still requiring that the shard was attributed FROM THE LEDGER
rather than by dying.
3. recovery-reshard-frozen still wedges. Its bounded-failure assertion never ran, because
the harness's generic wedge check fires first -- correctly, a wedge is a failure. That
check now says what a wedge means on this variant, since the generic wording reads as
"did not recover" and recovery is out of scope here; what regressed is that the run no
longer ENDS. Whether (1) also fixes this is the open question the next run answers.
Signed-off-by: asolergibert <asolergibert@nvidia.com>
THE DEADLINE IS FOR A SILENT PEER, NOT A DEAD ONE, and firing it on a dead one is strictly harmful. Job 6405953 passed the reshard kill variant with RefitAborted appearing zero times, before any deadline existed: the victim's process was gone, its sockets closed, NCCL's own error path unblocked the survivors, and the run recovered off the pre-existing actor-death route. Once the deadline existed it started winning that race. It aborts at its timeout, sync_stream_within gives up on kernels already enqueued on the trainers' streams, and the CUDA context cannot be trusted afterwards -- so the rebuild that used to succeed cannot. recovery-reshard-refit has failed continuously since job 6512153, which is the run where the deadline first began firing on that path: 6027170 pass 6405953 pass <- last pass, deadline never fired 6512153 FAIL <- deadline starts firing here 6513879 FAIL 6524733 FAIL 6582457 FAIL That also corrects the record: I called this a regression from release_within, and it is not -- it broke well before release_within existed, and _settle_before_propagating, added to fix exactly this and written up in design section 8.5.6, never fixed it. Three runs then established that the abort itself never retires. Job 6521181's py-spy dump caught both trainers in init_nccl_communicator with frame-less ncclCommAbort threads 25 minutes on. Job 6523731 killed the frozen victim before the rebuild, so its sockets were closed, and they wedged identically. Job 6582457 pinned the release to the caller's CUDA device -- with a SIGKILLed peer -- and it still did not return in 30s. Not the peer, not the device: the orphaned work is local, exactly as sync_stream_within has always said. So the controller stands the deadline down the moment a probe reports an actor DEATH, which is conclusive in a way a timeout is not. RefitAbortWatchdog keeps a process-wide registry of armed guards; stand_down() sets the same event a clean exit sets, so the watch thread returns without aborting and fired stays False. The controller reaches the workers at all only because the refit runs off their event loop (await_off_loop). The frozen case is deliberately untouched. A frozen rank is alive, no death is ever recorded, the deadline still fires, and the run still ends attributably -- which remains the only outcome available on this transport once the bulk transfer has aborted. Keyed on death rather than silence is the whole design, so both halves are pinned: test_a_confirmed_death_stands_the_trainers_deadline_down and test_a_timeout_leaves_the_deadline_armed. Standing down on any probe failure would let a frozen rank wedge the run forever, and that mutation fails the second one. Signed-off-by: asolergibert <asolergibert@nvidia.com>
… wedging The limitation needs BOTH conditions, and job 6584636 measured its boundaries: nccl_reshard kill at a step boundary deadline 0 stuck 0 -> recovers null kill at a step boundary deadline 0 stuck 0 -> recovers null-refit kill mid-refit deadline 4 stuck 0 -> recovers null-refit-frozen freeze mid-refit deadline 4 stuck 0 -> recovers nccl_reshard-refit fault mid-refit deadline 4 stuck 2 -> UNRECOVERABLE The packed-broadcast transport takes the identical fault, fires the identical deadline, aborts, rebuilds and recovers without one stuck abort. Reshard recovers too when the fault lands at a step boundary. Only reshard-mid-bulk-transfer is lost, because only there does sync_stream_within give up on kernels already enqueued on the TRAINERS' streams -- both its call sites are inside _nccl_reshard_refit and nowhere else -- and aborting a communicator does not retire them. Afterwards ncclCommAbort never returns and nothing can bootstrap a communicator on that device, so entering the recovery does not fail: it WEDGES, for the full 1800s harness deadline, with no attribution. That is what jobs 6521181, 6523731, 6582457 and 6584636 all did, each after eliminating one more candidate explanation -- the frozen peer's sockets, the thread-local CUDA device, and racing Ray's actor-death detection. So stop trying to recover and report it instead. sync_stream_within tags its RefitAborted with REFIT_CONTEXT_LOST_TOKEN and the controller refuses the recovery on that tag, ending the run in seconds with a named cause. The token travels in the message for the same reason REFIT_ABORTED_TOKEN does -- vLLM's EngineCore RPC keeps the text and drops the type -- and it is applied in exactly one place, so the fail-fast scope cannot widen by accident. test_an_ordinary_abort_still_recovers guards that from the other side, since marking a plain abort would turn four passing variants into fail-fast. The cost, stated plainly: recovery-reshard-refit RECOVERED at job 6405953, before any deadline existed -- the shard died, NCCL's own error path unblocked the survivors, and the run carried on with RefitAborted appearing zero times. Adding the deadline removed that. Under this guard it fails fast instead. A real capability regression on one scenario, accepted so the other thirteen keep a bound that makes them attributable, and recorded in the design doc with the route back. The functional assertion now covers both fault kinds on that path and requires the guard to have been REACHED, not merely to be consistent with the exit code -- otherwise a run that died of anything else inside 900s would read as a pass. Design doc: section 8.5.8 (v15). Signed-off-by: asolergibert <asolergibert@nvidia.com>
14 upstream commits; four of them produced seven conflicts. Each resolution below. #3612 feat(sglang): megatron backend weight refit for sglang rollouts - weight_sync/factory.py: it rewrote the train_cluster/inference_cluster/ refit_buffer_size_gb docstrings (SGLang owns its own process group, so it needs neither cluster handle). Took its wording and kept our refit_timeout_s entry, which it never saw. refit_timeout_s still reaches NcclReshardWeightSynchronizer and CollectiveWeightSynchronizer; the new SGLang synchronizer does not take it, which is correct -- our watchdog bounds a JOINT communicator and SGLang does not build one. - base_policy_worker.py: it added _refit_transport_state and connect_sglang_rollout_engines at the same insertion point as our stand_down_refit_watchdog. Disjoint additions; kept both. - pyrefly.toml: it swapped http_weight_synchronizer for sglang_weight_synchronizer. Corroborated by the merge deleting http_weight_synchronizer.py outright. #3773 feat(sc): support PPO in single controller - single_controller_utils/setup.py: the SC path is no longer GRPO-only, so it renamed grpo_config to algo_cfg. Kept our nccl_reshard precondition guard and applied the rename to the val-period line inside it; grpo_config no longer appears anywhere. - L1_Functional_Tests_SingleController.sh: it added a ppo_async run_test and padded every non-fast entry to align with "run_test fast". Kept our annotation -- it says which of skip-vs-pass a green lane actually means, which its one-line version does not -- and adopted the alignment, including on our seven recovery entries, so the file does not end up half-converted. - pyrefly.toml: it re-sorted the list, moving vllm_remote_sparse_weight_synchronizer to its correct alphabetical slot. Our side had added membership.py AND held that entry in the old position, so taking our block verbatim would have duplicated it. Kept membership.py only; verified the result is sorted and has no duplicates. #3545 fix(vllm): support native BF16 FlashInfer TRTLLM refit - vllm_backend.py: its _nrl_layerwise_reload_* class attributes landed where our model_update_group declaration is. Disjoint; kept both. - tests/unit/models/generation/test_vllm_backend.py: its layerwise-reload suite against our init_collective release tests plus the _RecordingGroup fixture. Disjoint; kept both. 53 tests collect. #3768 feat: add MOPD to single-controller text path - Touched setup.py alongside #3773; no separate resolution needed. Submodule: the merge advances Megatron-Bridge to d352aced (#3824). Verified the STAGED pointer is upstream's and not our stale 8c46dc42 -- staging the local one is what breaks the fast-forward check and `uv lock --check` together. Gym is untouched by the merge. Verified after resolving: no conflict markers remain, all four lint hooks clean (the one pyrefly error is the pre-existing unrelated transfer_queue import), and 841 unit tests pass across single_controller, refit_watchdog, worker_refit_signatures and weight_sync -- up from 725, because #3773 brings a large new SC suite that passes alongside ours. Signed-off-by: asolergibert <asolergibert@nvidia.com>
Review follow-up. The parameter was added to these two constructors and to the factory signature, but only the factory's Args block gained an entry -- the same omission on both synchronizers, which is exactly what the review flagged. Worth the lines because misreading this parameter is what silently costs the hang protection: it reaches only the two NCCL transports, and None disarms the watchdog in every worker. Signed-off-by: asolergibert <asolergibert@nvidia.com>
Contributor
|
/ok to test 3d9ce21 |
terrykong
added a commit
that referenced
this pull request
Aug 28, 2026
… wedges Since 9a87c01 on #3591, any fault while the nccl_reshard bulk transfer is in flight ends the run -- the abort orphans kernels on the trainers' own devices, so a crash there is no more recoverable than a wedge. The crash cells in the grid and the failure table are now transport splits like the hang cell, the support matrix says 'between syncs only', and the wedged-engine section is renamed to cover both fault kinds. Signed-off-by: Terry Kong <terryk@nvidia.com>
kajalj22
approved these changes
Aug 28, 2026
asolergi-nv
added a commit
that referenced
this pull request
Aug 29, 2026
PR3 (#3591) was SQUASH-merged into main as b3b6713, so none of its commits are ancestors of main while PR4 still carries all of them. Git therefore sees PR3's whole diff as independently added on both sides, which is why all 16 conflicts name b3b6713 and why the PR showed CONFLICTING despite the content being identical. That made the classification, not the content, the work. For each conflicted file: is main's version byte-identical to PR3's head (3d9ce21), and does PR4 add anything beyond it? Three groups fell out. GROUP A -- pure squash artefacts, resolved by taking OURS (10 files) fleet_health.py, collective_weight_synchronizer.py, membership.py, nccl_reshard_weight_synchronizer.py, grpo_sc_generation_shard_recovery.sh, test_watchdog_pump.py, test_membership.py, test_reconcile_communicator.py, test_reshard_rebuild.py, test_weight_synchronizer.py main == PR3 exactly and no other PR touched them, so PR4's side is main's content plus PR4's delta. Taking ours loses nothing. GROUP B -- PR4 contributes nothing, resolved by taking THEIRS (2 files) single_controller_utils/setup.py (#3480, #3727, #3821 on top of PR3) tests/unit/single_controller/test_refit_recovery.py (#3480 on top of PR3) GROUP C -- genuine merges (4 files), one per upstream PR below. The six upstream PRs that contributed real content, and what each needed: #3480 recover replay buffer from native TQ checkpoints single_controller.py: rollout_recovery imports. Kept alongside ours. setup.py, test_refit_recovery.py, L1 harness: group B / additive. #3765 log toolcall and thinktag violation rate single_controller.py: VIOLATION_TAG_KEYS. Auto-merged, verified present. #3727 support non-colocated MInf single_controller.py: MegatronGeneration import, kept alongside ours. L1 harness: grpo_megatron_generation_gym_single_controller.sh entry. #3821 warm-start the value model from a critic-pretrain checkpoint config.py: the max_num_epochs validator. Ours only adds restart_dead_shards to FleetHealthConfig, so both survive; verified the field landed in the right class and the validator is intact. #3655 nemo-lens telemetry vllm_generation.py: the @trace_fn decorator on generate. Ours adds restart_shard in a different region; both kept. #3839 pause generation during in-flight refit vllm_generation.py: pause_generation_for_refit / resume_generation_after_refit. Auto-merged, verified present -- worth knowing it exists, since it pauses engines around a refit and this PR restarts them. Verified after resolving: no conflict markers; all four lint hooks clean (the single pyrefly error is the pre-existing unrelated transfer_queue import); 1122 unit tests pass; both submodule pointers and uv.lock/pyproject byte-identical to main. Both sides' work was checked individually rather than assumed: EngineSupervisor wiring, restart_dead_shards, restart_shard, recreate_worker, desired_membership and the report_refit call on our side; the six items above on main's. Note for anyone reproducing locally: #3655 adds a nemo-lens dependency that the pre-merge container image does not carry, so tests fail at import with ModuleNotFoundError: nemo until the venv is refreshed. Plain upstream/main fails the same way in that image; it is not a merge defect. Signed-off-by: asolergibert <asolergibert@nvidia.com>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
3 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Note
Mirror of #3472, originally opened by @asolergi-nv from the
asolergi-nv/RLfork.The branch was re-pushed to
NVIDIA-NeMo/RLunchanged (same head SHA) so the four partscan be linked as a proper PR stack —
gh stackcannot operate on fork branches.Please review and comment here. The original #3472 is left open for history.
Part 3/4 of #3454
What this fixes
With #3470 and #3471 in place a dead engine no longer wedges rollouts and no longer receives traffic — but the run still dies, and it dies in the worst possible way.
The refit communicator (
model_update_group) is built once at setup over every training and inference rank. A NCCL broadcast requires every rank in the communicator to take part, so when a generation rank is gone the next weight sync blocks forever inside NCCL: no exception, no progress, no CPU burn, and Ray still reporting every actor healthy. This is the failure the whole effort exists to remove.It is also ordered so that the hang comes first:
sync_weightsdoesray.get(futures_train)before theray.get(futures_inference)that would surface the dead actor, so the trainer blocks before anything can report the death.What it does
Reconcile before every refit, not on a death event.
reconcile_communicator()is called at the top of_sync_weights— the one point where the refit group is provably idle and every rank is synchronized, which matters because the operations that change membership are themselves collectives. Doing it every time is idempotent and converges after a missed or reordered health update, instead of needing replay.Rebuild over the survivors, so training continues on what is left. Trainers are never excluded, so rank 0 stays a trainer and the broadcast root is stable.
Both NCCL transports recover, by different routes.
nccl_reshardneeds more than the plain broadcast: both communicator families rebuilt and the refit plan regenerated.Four decisions:
"Absent" is deliberately not the complement of "serving". The obvious implementation reads
serving_shards(), and it is a bug.SUSPECT(failing probes, not yet condemned) andSTALE(reloaded, holding old weights) are both withheld from traffic while their processes are alive and join a refit normally. Using the serving set would abort a run on one failed probe — and would abort it precisely when a STALE shard is waiting to be refit, which is the recovery, not the failure. HenceGenerationFleetMonitor.absent_shards()over{DEAD, RESTARTING, RETIRED}.Rebuilding the communicator is only half a recovery. Every refit dispatch goes through
run_all_workers_*, which walks the whole worker group — so after a loss it kept calling the dead shard's Ray actor and the next refit failed withRayActorError. Both dispatches now address only surviving DP leaders. (This shipped broken in an intermediate commit here and is fixed within this PR; kept as separate commits because the fix is instructive.)nccl_reshardcannot simply be resized. Its bulk path is a mesh-to-mesh redistribute, not a broadcast:prepare_nccl_reshard_refit_infoderives each parameter's destination placements fromgen_world_size. Reusing a plan built for the old fleet does not error — a stale mesh is still a valid mesh — it just has survivors writing the slices the dead shard owned and leaving their own unwritten. So the plan is regenerated, andinit_communicatorand the rebuild share one_build(membership)so that arithmetic exists once and every normal run exercises the rebuild path.Survivors are compacted to contiguous prefixes, not left with a hole. The reshard destination mesh is
torch.arange(offset, offset + num_gpus), so a gap silently misaligns every parameter rather than erroring. The rank arithmetic lives in a pureweight_sync/membership.py, separate from Ray dispatch, because it is the part that must be exactly right and the part that cannot be exercised without ≥3 GPUs.Tests
StatelessProcessGroup.abort()verified on 2×A6000: with a peer SIGKILLed mid-broadcast, a survivor blocked in the collective was released 0.15 s after another thread calledabort().abort()and notdestroy()— NCCL documentsdestroyas an intra-node collective every rank must call or it hangs, which is exactly what a dead rank cannot do.tests/functional/grpo_sc_generation_shard_recovery.shis the gate, runs on both refit transports in the SingleController lane, and self-skips below 3 GPUs rather than passing vacuously.Issues
List issues that this PR closes (syntax):
Usage
# Add a code snippet demonstrating how to use thisBefore your PR is "Ready for review"
Pre checks:
Additional Information