Skip to content

feat(sc): contain rollout failures in the SingleController path - #3470

Open
asolergi-nv wants to merge 16 commits into
NVIDIA-NeMo:mainfrom
asolergi-nv:feat/sc-resiliency-01-containment
Open

feat(sc): contain rollout failures in the SingleController path#3470
asolergi-nv wants to merge 16 commits into
NVIDIA-NeMo:mainfrom
asolergi-nv:feat/sc-resiliency-01-containment

Conversation

@asolergi-nv

@asolergi-nv asolergi-nv commented Aug 3, 2026

Copy link
Copy Markdown

Part 1/4 of #3454

What this fixes

Two failure modes in the SingleController async-GRPO path, both silent. Neither produces an exception, a log line, or an unhealthy Ray actor — which is why they have not been noticed.

1. The GRPO path silently corrupts training. AsyncRolloutImpl caught every exception from generation and committed the rollout anyway, with empty text and zero reward. Those rows enter the GRPO leave-one-out baseline as legitimate zero-reward samples, so a transient generation failure does not fail the run — it quietly biases the advantage estimate. This is a live data-correctness bug independent of any resiliency feature.

2. The NeMo-Gym path silently wedges. Gym's HTTP client is constructed with ClientTimeout() — every timeout disabled — and retries ClientOSError / ServerDisconnectedError in an uncapped loop. A dead vLLM endpoint therefore parks the rollout forever. Each parked rollout holds a max_inflight_prompts permit, so the rollout pump eventually blocks entirely while the train pump spins on an empty buffer. Ray reports every actor healthy throughout.

What it does

  • Stops swallowing generation errors (rollout_manager.py). The single highest-value change here: failures are classified and raised instead of being committed as zero-reward rows.
  • A failure taxonomy separating infrastructure failures (retry elsewhere) from data failures (retrying will not help). HTTP is classified by status, not exception type — a ClientResponseError carrying 503 is infrastructure, one carrying 400 is data.
  • Deadlines on rollouts, generation turns and environment steps, all defaulting toNone (disabled).
  • Re-dispatch with two separate budgets. A prompt that fails on infrastructure is re-dispatched to a different engine rather than dropped; a prompt that fails on its own data is retried a smaller number of times and then either fails the run or is skipped under an explicit budget. Two budgets because "the engine died" and "this prompt is malformed" deserve opposite treatment.
  • A watchdog that publishes rollout counters and reports stalls — the failures that otherwise produce no signal at all.
  • Partial NeMo-Gym re-dispatch: only the rows that never arrived are re-sent, rather than the whole prompt group.

Inert by default — all timeouts default to None. The two always-on changes are that errors stop being swallowed and retries are bounded; both convert silent failure into loud failure, which can surface pre-existing problems that were previously invisible. That is the intent.

Tests

  • Unit tests for the taxonomy, deadlines, re-dispatch budgets, watchdog, and partial gym re-dispatch, including a regression test for the stall condition above.
  • ruff and pyrefly clean.
  • End-to-end on 2xA6000: grpo_dp_single_controller.sh (all 5 metric checks) and grpo_async_gym_single_controller.sh (10 steps, median gen_kl_error 0.0010579, reward 0.5).

The chaos harness

tests/functional/grpo_dp_single_controller_chaos.sh SIGKILLs a generation worker and asserts the job fails fast and attributably rather than wedging. It found the stall-detector bug above, and it is registered in the SingleController L1 lane

Registered twice, because Ray retitles a worker for the duration of each call and the two states fail by different routes:

variant victim outcome
default (idle) ray::VllmAsyncGenerationWorker 222s, RolloutStall — the loss must be detected
VICTIM_STATE=serving ray::VllmAsyncGenerationWorker.generate_async 12s, GenerationUnavailable + RolloutRedispatchExhausted — an in-flight RPC dies

Victim selection is pinned rather than left to pgrep ... | head -1, which matched three distinct processes across five states and so silently chose a different scenario run to run. It never failed — every scenario ends in a bounded attributable failure, which is all the test asserted — so the divergence was only visible as wall-clock. Three consecutive idle runs now give an identical victim title and 222s.

Issues

List issues that this PR closes (syntax):

Usage

  • You can potentially add a usage example below
# Add a code snippet demonstrating how to use this

Before your PR is "Ready for review"

Pre checks:

  • Make sure you read and followed Contributor guidelines
  • Did you write any new necessary tests?
  • Did you run the unit tests and functional tests locally? Visit our Testing Guide for how to run tests
  • Did you add or update any necessary documentation? Visit our Document Development Guide for how to write, build and test the docs.

Additional Information

  • ...

asolergi-nv and others added 12 commits August 1, 2026 18:10
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 NVIDIA-NeMo#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>
@asolergi-nv
asolergi-nv requested review from a team as code owners August 3, 2026 13:28
@copy-pr-bot

copy-pr-bot Bot commented Aug 3, 2026

Copy link
Copy Markdown

This pull request requires additional validation before any workflows can run on NVIDIA's runners.

Pull request vetters can view their responsibilities here.

Contributors can view more details about this message here.

@asolergi-nv asolergi-nv changed the title feat(sc): contain rollout failures in the SingleController path (1/4 #3454) feat(sc): contain rollout failures in the SingleController path Aug 3, 2026
@asolergi-nv

Copy link
Copy Markdown
Author

/ok to test 97b309a

1 similar comment
@asolergi-nv

Copy link
Copy Markdown
Author

/ok to test 97b309a

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>
@asolergi-nv

Copy link
Copy Markdown
Author

/ok to test 85afdb2

@asolergi-nv asolergi-nv added the CI:L1 Run doctests, unit tests, and functional tests label Aug 4, 2026
@asolergi-nv

Copy link
Copy Markdown
Author

/ok to test 85afdb2

@asolergi-nv

Copy link
Copy Markdown
Author

/ok to test 8bb7148

Merging main brought in NVIDIA-NeMo#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 NVIDIA-NeMo#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 NVIDIA-NeMo#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>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

CI:L1 Run doctests, unit tests, and functional tests

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant