Skip to content

fix(orchestrator): hold batch ship on the trainer's published version#3050

Merged
samsja merged 37 commits into
mainfrom
fix/strict-target-lag
Jul 23, 2026
Merged

fix(orchestrator): hold batch ship on the trainer's published version#3050
samsja merged 37 commits into
mainfrom
fix/strict-target-lag

Conversation

@mikasenghaas

@mikasenghaas mikasenghaas commented Jul 16, 2026

Copy link
Copy Markdown
Member

Summary

Minimal fix for RES-1077: the pipeline's pacing is two counters (batch N, policy version v) with three rules — START (main's existing dispatch gate), ADVANCE (this PR: batch N ships only once the trainer has published v{N-1-TARGET_LAG}), and DIE (main's existing in-flight drop aging).

  • The bug: on fast envs the sink fills batches from buffered rollouts faster than the trainer consumes them; ship was ungated, so the orchestrator finished all its steps, tore down the weight watcher, and stranded the trainer inside its next in-memory (NCCL/NIXL) broadcast handshake — the watcher serves the receive side. The launcher then hung forever.
  • The fix (~30 lines): hold each ship until the required version is published. The requirement is always satisfiable (the trainer skips only the final TARGET_LAG+1 in-memory broadcasts), and the hold doubles as the shutdown-liveness guarantee: the orchestrator cannot exit before the last watcher-dependent handshake has completed.
  • Supporting hardening: policy.version now advances at publish confirmation (inference applies immediately and pauses during the update, so published ≡ applied for every consumer — no weight-reload time serialized into the ship path), and WeightWatcher.stop() drains an in-flight apply before cancelling so teardown can never strand the trainer mid-handshake.
  • No throughput change: dispatch, drops, and batching are untouched. In the generation-bound regime the hold never binds; in the trainer-bound regime the trainer is the bottleneck regardless — the hold only changes where a finished batch waits (orchestrator instead of the trainer's queue), never when the trainer consumes it.
  • Honest staleness counting: shipped rollouts are stamped with their true consumption lag — batch N trains on v{N-1}, so a rollout generated from v{k} is (N-1)-k versions off-policy, queue time included. The dispatcher's in-flight counter (unchanged, still feeds the max_off_policy_steps drop rule) only sees weight updates during generation and undercounts; Max Off-Policy in logs and rollout records now reports the real number (expect ~1-3 where runs previously showed 0 — the data was equally off-policy before, it just wasn't measured).
  • Also fixes two adjacent races the longer-lived orchestrator exposes: the multi-run trainer crashing on a run dir deleted while its token-export tail flushes (mark_stable now skips deleted runs, mirroring the broadcast paths), and the multi-run test's rmtree racing that same flush (retry helper). CI uploads run logs as artifacts on integration failures.

Verification

uv run rl @ configs/basic/reverse-text/rl.toml --max-steps 10 --no-wandb (2 GPUs, NCCL, ~1-5s buffer-fed steps):

  • Before (main): orchestrator ships all batches in ~40s and exits while the trainer is steps behind; trainer wedges silently in the broadcast handshake; launcher hangs until killed.
  • After: the hold binds exactly twice at the buffer-fed tail (Holding batch N until the trainer publishes policy vK), Training finished!, clean exit, 10/10 batch dirs. pytest tests/unit -m "not gpu": 469 passed.

🤖 Generated with Claude Code


Note

High Risk
Changes orchestrator–trainer synchronization and weight-watcher lifecycle on the in-memory broadcast path; incorrect hold/version timing could deadlock shipping or leave the trainer wedged, though the change is narrowly scoped to pacing and teardown.

Overview
Fixes a hang where fast envs could ship all train batches and tear down the orchestrator before the trainer finished in-memory weight broadcasts, stranding NCCL/NIXL handshakes that need the live WeightWatcher.

Orchestrator pacing: Before sending batch N, non-bench runs now hold until policy.version reaches v{N-1-TARGET_LAG}, waking on version_advanced when the watcher publishes. Shipped rollouts get off_policy_steps stamped from batch step vs rollout policy_version (queue time included) for honest Max Off-Policy metrics.

WeightWatcher: policy.version advances at publish confirmation (before inference update_weights) so held ships release without waiting on reload; stop() waits for an in-flight apply under update_lock so teardown does not cancel mid-handshake.

Adjacent hardening: TokenExporter.mark_stable tolerates deleted multi-run dirs; multi-run integration uses retrying remove_run_dir, --clean-output-dir on RL tests, and CI uploads *.log artifacts from /tmp/outputs on GPU integration failure.

Reviewed by Cursor Bugbot for commit daf5c8c. Bugbot is set up for automated code reviews on this repo. Configure here.

mikasenghaas and others added 6 commits July 16, 2026 01:02
The dispatch gate only stops new rollouts; on tiny fast envs the sink
keeps forming batches from buffered rollouts, so the orchestrator ships
past TARGET_LAG, finishes, and tears down the watcher while the trainer
still has pending NCCL broadcasts. Those broadcasts then block forever
in the STABLE->NCCL_READY handshake (no watcher to trigger the inference
receive) and the launcher never exits.

hold_for_target_lag blocks shipping batch N until the trainer has
published v{N-1-TARGET_LAG}, bounding in-queue staleness and keeping the
orchestrator alive for every broadcast the trainer will make. Under NCCL
the last published version is max_steps-2, exactly what the final batch
requires, so the hold never waits on an unpublishable version.

Also logs a per-batch staleness decomposition (pre-queue = versions
elapsed during generation, in-queue = versions spent waiting in the
sink) under staleness/*.

Fixes RES-1077

Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Measure a rollout's off-policy lag against the batch that consumes it —
batch N trains on policy v{N-1}, so lag = (N-1) - generation version,
queue time included. The old off_policy_steps counter only counted
weight updates during generation, so a rollout that finished fast but
sat buffered across steps trained arbitrarily stale while reporting 0.

- max_off_policy_steps now bounds consumption lag: in-flight groups are
  cancelled pre-pause (cancel_stale_train_groups) and buffered sink
  survivors are swept whenever the cutoff moves (ship, weight update).
  Groups share one generation version, so stale groups drop whole.
- Dispatch is demand-driven: the orchestrator injects train_demand
  (batch shortfall minus buffered + partial-group + in-transit
  rollouts), re-evaluated per scheduled group, replacing the lead-based
  dispatch gate and the #2990 final-batch NCCL exemption it needed.
  out_q backlog counts as coverage so ship-holds don't over-provision.
- Max Off-Policy in logs is now the honest consumption lag, stamped at
  ship (reverse_text: steady-state max 3 vs a dishonest 0 before).

Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
…e single-use helpers

- train_demand -> train_needed: the dispatcher only thresholds the value,
  and the magnitude isn't even unit-stable across batch modes, so the
  injected contract is a bool predicate.
- Demand target is max(batch_size, max_inflight_rollouts): explicit
  over-provisioning (oversampling_factor > 1 or a raised max_inflight)
  keeps racing batch stragglers instead of being silently capped at one
  batch; capacities below the batch keep a batch_size target so the
  batch can still complete.
- Reject max_off_policy_steps=0 with NCCL broadcast + max_steps: the
  final batch necessarily trains one version off-policy (the trainer
  never broadcasts the last versions), so a zero budget would hang the
  run collecting the final batch.
- Inline single-call helpers (hold_for_target_lag, stale_cutoff, the
  dispatcher lag gauges) into their call sites.

Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
The last NCCL batch necessarily trains one version off-policy (the
trainer never broadcasts the final versions), so a zero budget can never
be met and would hang the run collecting the final batch. A field floor
covers this without a conditional validator.

Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
@mikasenghaas
mikasenghaas requested a review from samsja July 17, 2026 19:15
@mikasenghaas
mikasenghaas marked this pull request as ready for review July 17, 2026 19:15
samsja
samsja previously approved these changes Jul 17, 2026

@samsja samsja left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

lgtm

…ht applies

Two regressions surfaced by the PR's GPU CI run:

- alphabet_sort/rl_opd deadlocked at 126/128 (+3 buffered, 0 inflight):
  demand hit zero mid-group, so an open group's remaining rollouts were
  never scheduled — its buffered members count as batch coverage but the
  group can never finalize. Already-open groups now keep scheduling
  regardless of demand (try_schedule allow_fresh); only fresh groups are
  demand-gated.
- reverse_text timed out (~26s/step vs ~19s on main): the ship hold
  waited on the applied version, serializing every inference weight
  reload into the ship path. Filesystem broadcast decouples publish from
  apply and the trainer never blocks on the watcher there, so the hold
  now releases on the published version (tracked in on_version_pending).
  NCCL keeps waiting for the apply — the trainer blocks in the receive
  handshake until then, so releasing on publish could strand its final
  handshake at teardown.

Verified: reverse_text e2e clean on both broadcast types (filesystem
6-7s steps, NCCL exits clean), 485 unit tests pass.

Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
mikasenghaas and others added 2 commits July 17, 2026 22:17
Resumed orchestrators now pace to the shared trainer — a batch only
ships once the trainer has produced the required policy version — so
with three runs round-robining one trainer, beta_resume's tail (steps
21-25) can legitimately exceed the old 300s wait while its versions
queue behind the other runs. Matches the 600s budget of the other RL
integration tests.

Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Comment thread src/prime_rl/orchestrator/train_sink.py Outdated
mikasenghaas and others added 3 commits July 17, 2026 23:04
drop_stale now mirrors what entering pending_batch added to
_payload_total/_payload_count, keeping the token-mode demand estimate
consistent with the buffered set. Also expand the allow_fresh comment
with the group-finalization invariant it protects.

Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
The multi-run test's failures only surface a sampled tail of one
orchestrator's output; the copied per-run orchestrator/trainer/inference
logs are what's needed to debug them (currently: beta_resume wedging on
a policy version the shared trainer never publishes after resume).

Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Comment thread src/prime_rl/orchestrator/train_sink.py Outdated
mikasenghaas and others added 7 commits July 18, 2026 01:27
The output_dir fixture rmtree'd the whole outputs dir at session
teardown, so the upload step found nothing. Skip the cleanup under CI
(the workspace is discarded with the job) and match log files directly
instead of a directory glob.

Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Exact-one-batch demand synchronized dispatch into one 128-rollout wave
per batch: each ship opened demand 0->128 at once, the wave stampeded
the env server together (MCP timeouts + backoff retries in CI
alphabet_sort), and inference idled ~60s per cycle while the whole wave
sat in env round-trips — tripling the step cadence vs main (artifact
logs: vLLM alternating 30s busy / 60s '0 running, 0 waiting').

With up to TARGET_LAG batches of lookahead — allowed only while a
rollout landing that far ahead still fits max_off_policy_steps —
steady-state dispatch is continuous (each arrival frees demand for the
next start), keeping the engine fed through env-phase gaps. Costs one
version of staleness on the lookahead batch (reverse_text plateau 3->4,
within the default cap of 8).

Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Mismatch KL compares generation-time inference logprobs against the
training weights, so it includes async training's policy-version gap on
top of trainer/inference numerics. With strict pacing + dispatch
lookahead the steady-state consumption lag is 2-3 versions (visible and
bounded, vs unmeasured before), which puts the last-steps average at
~0.011-0.012 against the old 0.01 budget calibrated for main's profile.

Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Artifacts from the failed runs show no wedges — every process was
mid-progress at the deadline: alphabet_sort cycles at ~70s on slow vm
runners (33s on fast ones; trainer MFU varies 2-6% across the pool) and
the multi-run trainer was mid-micro-step serving three runs
round-robin. With strict pacing the orchestrators ride the trainer's
pace instead of finishing early on stale buffers, so wall time tracks
the slowest component.

Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
The wait_for_log/wait_for_file 300s defaults were calibrated for
orchestrators that sprint ahead of the shared trainer on buffered
batches; with strict pacing three orchestrators ride the trainer's
actual speed, so every internal milestone (alpha steps, checkpoint
STABLE markers, resume completion) lands slower and a different 300s
wait tripped on each CI round.

Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Comment thread src/prime_rl/orchestrator/orchestrator.py Outdated
Comment thread src/prime_rl/orchestrator/train_sink.py Outdated
buffered_count hides group-scoring envs' partial groups for the
pipeline log's honesty, but demand must count them — otherwise members
sitting between arrival and group completion invite bounded
over-dispatch. Also document why a held batch intentionally does not
count as coverage (dispatch-ahead keeps inference busy through
trainer-bound holds; staleness stays bounded by lookahead + cutoff).

Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Comment thread src/prime_rl/orchestrator/orchestrator.py
Comment thread src/prime_rl/orchestrator/dispatcher.py Outdated
mikasenghaas and others added 2 commits July 20, 2026 20:06
The multi-run trainer crashed with FileNotFoundError touching
run_gamma/token_exports/step_20/STABLE: the multi-run integration test
rmtree's a run's dir once its orchestrator finishes, while the trainer
is still flushing that run's tail steps. The broadcast paths already
skip deleted runs on FileNotFoundError; mark_stable now does the same.

This was the root cause of the recurring multi_run CI failure: the
crashed trainer never published the resumed run's next version, so
beta_resume's ship-hold waited forever ('failed with code None'). On
main the same crash risk exists but the orchestrator never waited on
the trainer, so nothing surfaced.

Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Batch N's data now comes from v{N-2}..v{N-4} instead of main's
effectively-fresher buffer profile, so reward-at-step-7 reads 1-2
policy versions staler (observed 0.14-0.20 across four CI rounds vs the
0.2 floor). The final-reward thresholds still enforce learning quality;
the early floor drops to 0.1 — comfortably above a broken run's flat
~0.05-0.08.

Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Same class as the other RL integration budgets: the trainer was
SIGTERM'd at step 14 with the orchestrator already finished — killed by
the 600s deadline steps from completion on a slow runner.

Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Comment thread src/prime_rl/orchestrator/orchestrator.py Outdated
With strict pacing the reward-vs-step-index curve sits 1-2 policy
versions lower throughout; gamma — the most trainer-contended of the
five runs — landed at 0.627 while its siblings cleared 0.69-0.75. A
broken run still reads ~0.1-0.3.

Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Comment thread src/prime_rl/orchestrator/orchestrator.py Outdated
mikasenghaas and others added 4 commits July 21, 2026 17:27
# Conflicts:
#	tests/integration/test_alphabet_sort.py
#	tests/integration/test_reverse_text_multi_run.py
#	tests/integration/test_reverse_text_rl_sft.py
# Conflicts:
#	src/prime_rl/orchestrator/dispatcher.py
#	src/prime_rl/orchestrator/train_sink.py
Root cause of the reward-at-step gap vs main: the old lead gate doubled
as the dispatch freshness bound (rollouts only started at version >=
step-2, landing lag <= 2). The demand rewrite left freshness gated only
by max_off_policy_steps (8) plus a deliberately-ahead lookahead batch,
so mean consumption lag ran 2-4 versions vs main's 1-2 — visible as
Max Off-Policy 3-4, mismatch KL ~2x main's, and reward-at-step lower by
1-2 versions of learning.

Dispatch now pauses when new rollouts would train more than
DISPATCH_LAG = TARGET_LAG+1 versions behind their generation policy,
with the lookahead scaled inside that budget — the tightest bound that
still keeps inference busy during ship-holds. Landing lag returns to
<= 2 (spill 3); local e2e: Max Off-Policy plateau 4 -> 3, mismatch KL
mean ~0.012 -> ~0.008. max_off_policy_steps returns to being purely the
drop safety net.

Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Comment thread src/prime_rl/orchestrator/orchestrator.py Outdated
# Conflicts:
#	src/prime_rl/orchestrator/orchestrator.py
Comment thread src/prime_rl/orchestrator/orchestrator.py Outdated
The fixture's rmtree races the shared trainer's token-export writes,
which mkdir the run dir back mid-deletion (OSError: Directory not
empty). Retry until the flush for the finished run drains.

Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
mikasenghaas and others added 2 commits July 22, 2026 19:31
The merge of #3060 kept this branch's on_version_pending/on_new_version
bodies and dropped main's ModelExpress status updates — the READY
signal the NIXL trainer waits on before each broadcast and the
INITIALIZING reset after it. Post-startup NIXL runs would hang in the
handshake. Both signals are back, ordered as on main (READY pre-update,
INITIALIZING post-update) around this branch's staleness/wake logic.

Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Strip the demand-driven dispatch, consumption-lag drop rework, and test
recalibrations accumulated on this branch back to main's behavior, and
keep only what the spec (docs/staleness.md) strictly requires:

- ADVANCE rule: batch N ships only once the trainer has published
  v{N-1-TARGET_LAG}, so the orchestrator cannot finish and tear down the
  weight watcher while the trainer still has in-memory broadcast
  handshakes pending (RES-1077). Main's dispatch gate (START) and
  in-flight drop aging (DIE) are untouched, so generation freshness and
  throughput are unchanged in both pacing regimes — the hold only moves
  where a finished batch waits, never when the trainer consumes it.
- policy.version advances at publish confirmation (inference applies
  immediately and pauses during the update, so published and applied
  are indistinguishable to consumers), and WeightWatcher.stop() drains
  an in-flight apply before cancelling — teardown can never strand the
  trainer mid-handshake.
- Keep the trainer-side token-export guard, the multi-run test's
  deletion-race helper, and the CI failure-artifact pipeline from the
  earlier iterations; drop everything else.

Verified on the original repro (reverse_text, fast env, NCCL): 10/10
batches, clean trainer exit, 2 holds at the buffer-fed tail.

Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
@mikasenghaas mikasenghaas changed the title fix(orchestrator): enforce target lag strictly at ship time fix(orchestrator): hold batch ship on the trainer's published version Jul 22, 2026
Comment thread src/prime_rl/orchestrator/watcher.py
Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Comment thread src/prime_rl/orchestrator/watcher.py
Comment thread src/prime_rl/orchestrator/orchestrator.py
mikasenghaas and others added 3 commits July 22, 2026 20:27
Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
The output_dir fixture deleted the whole workspace at session teardown,
racing the failure-artifact upload (and hiding logs from post-mortems
generally). Keep it unconditionally and make fresh starts responsible
for their own hygiene: alphabet_sort and reverse_text_moe now pass
--clean-output-dir like the other rl-launcher tests. Resume runs are
unaffected — validate_output_dir returns before cleaning when resuming.
The multi-run orchestrators manage per-run dirs the test creates (and
the standalone orchestrator entrypoint has no clean flag).

Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Batch N trains on policy v{N-1}, so a rollout generated from v{k} is
(N-1)-k versions off-policy — queue time included. The dispatcher's
in-flight counter only sees weight updates during generation, which
undercounts by the queue-wait aging and the dispatch-time baseline gap;
the ship-time stamp overwrites it so Max Off-Policy in logs and the
rollout records report the real number. The counter itself (and the
max_off_policy_steps drop rule it feeds) is unchanged.

Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>

@cursor cursor Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Cursor Bugbot has reviewed your changes and found 1 potential issue.

Fix All in Cursor

❌ Bugbot Autofix is OFF. To automatically fix reported issues with cloud agents, enable autofix in the Cursor dashboard.

Reviewed by Cursor Bugbot for commit e467d6b. Configure here.

Comment thread tests/integration/test_reverse_text_multi_run.py
Alpha is SIGTERM'd mid-run, so the trainer is actively flushing its
token-export tail when the dir is deleted — the recreate race is most
likely at exactly this site.

Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
@mikasenghaas
mikasenghaas requested review from S1ro1 and samsja July 22, 2026 23:42
@samsja
samsja merged commit acbabc6 into main Jul 23, 2026
18 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants