Repository navigation
feat(orchestrator): opt-in background group finalization - #3802
Closed
faresobeid wants to merge 2 commits into
Closed
faresobeid wants to merge 2 commits into
faresobeid wants to merge 2 commits into
Conversation
faresobeid
force-pushed
the
feat/background-group-finalize
branch
from
October 3, 2026 12:22
1d75cf3 to
07e846f
Compare
Algorithms can set `finalize_in_background = True` to run `finalize_group` as a background task instead of inline in the orchestrator loop. Every other algorithm keeps the inline path unchanged. - completed tasks put the group id on the sink-owned `finalized_q`; the main loop settles those groups (admission, samples, batching) before reading new dispatcher results - train dispatch pauses while an env has `max_finalizing_groups` groups finalizing - a step advance cancels finalizing groups already past max_off_policy_steps, accounted like a stale cancellation - drain and shutdown cancel background finalizations - an exception in a background finalize_group crashes the main loop
…least one batch of the env's groups)
faresobeid
force-pushed
the
feat/background-group-finalize
branch
from
October 4, 2026 10:52
d6e2665 to
d12bb49
Compare
samsja
pushed a commit
that referenced
this pull request
Oct 4, 2026
…alize_group (#3845) Two small cleanups in `orchestrator/algo/`. Item 4 of the orchestrator review. - `action_loss_type` is now declared once, on the algorithm config classes. Before, it was declared on the config classes and again on the runtime classes (`algo/base.py`, `sft.py`, `opd.py`, `opsd.py`), and `build_algorithm` asserted that the two agreed. Now `Algorithm.__init__` copies it from the config, so `train_sink`'s `env.algorithm.action_loss_type` is unchanged. - `Algorithm.finalize_group` is removed. It only called `score_group`, and `train_sink` now calls `score_group` directly. `finalize_episode` is kept: its "has sampled tokens" guard matters for `echo`, whose `score_episode` does not filter on the mask. - `docs/algorithms.md` is updated to match. Behavior change: none. Every algorithm type gets the same loss routing as before, because the old assert guaranteed that the config and runtime values were equal. ## Checks - `pytest tests/unit/orchestrator tests/unit/test_configs* -m "not gpu"`: 291 passed (`test_algorithms` covers the per-type `action_loss_type`). - `ruff check`, `ruff format --check`: clean. GPU checks: none needed. ## Conflicts `algo/base.py` and `algo/__init__.py`: #3802, #3803 (small). #3838 (fold `GenerationSource`) also edits `algo/base.py` and `algo/__init__.py` (`connect_frozen_client`), which needs a trivial rebase. `train_sink.py:245`: #3652 rewrites `train_sink.py` (a one-line change there). <!-- CURSOR_SUMMARY --> --- > [!NOTE] > **Low Risk** > Refactor-only: loss routing and group scoring call paths are unchanged in effect; no auth, data, or trainer hot-path logic changes. > > **Overview** > **Single source of truth for loss routing:** runtime `Algorithm` instances now take `action_loss_type` from the algorithm config in `__init__` instead of duplicating it on each subclass and asserting agreement in `build_algorithm`. Per-algorithm class variables on `OPD`, `OPSD`, and `SFT` are removed; `train_sink` still reads `env.algorithm.action_loss_type` when stamping samples. > > **Thinner scoring API:** `Algorithm.finalize_group` is dropped and the train sink calls `score_group` directly when a group has trainable survivors. `finalize_episode` remains as the guarded entry point for rollout-local `score_episode` (skips episodes with no trainable sampled tokens). > > **Docs:** `algorithms.md` is updated to describe this pipeline wiring and config-owned `action_loss_type`. > > No intended training behavior change—loss routing values are the same as before when config and runtime matched. > > <sup>Reviewed by [Cursor Bugbot](https://cursor.com/bugbot) for commit 1ebee7f. Bugbot is set up for automated code reviews on this repo. Configure [here](https://www.cursor.com/dashboard/bugbot).</sup> <!-- /CURSOR_SUMMARY -->
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
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.
What
No behavior change unless an algorithm opts in. This adds an opt-in
Algorithm.finalize_in_background: ClassVar[bool] = False. An algorithm that does not opt in runs the same inlineprocess_grouppath asmain: same order, same accounting, same metrics. Only the inline code was split intoprocess_group(pop the pending state) and_settle_group(the unchanged body), so both paths can share it.When an env's algorithm opts in (gar in #3803), a completed group with trainable survivors runs
finalize_groupas a background asyncio task:TrainSink.finalized_q(an unboundedasyncio.Queue[str]; it is notdispatcher.out_q).out_q, the orchestrator main loop settles each queued id withTrainSink.finalized(group_id). This runs the rest of the oldprocess_group: accounting, curriculum admission,trace_to_samples, pruning,pending_batchinsert, the insertion stale sweep, and_maybe_batch.finalized()returnsNonefor ids that were already dropped (stale cancel, drain, stop).Why
The gar
score_groupawaits a grader call of 10-30 minutes. If that call is inline, the whole orchestrator stops for that time.Background-path behavior
Algorithm.max_finalizing_groups(default 64; an instance can override it from config). When any env has that many groups finalizing,update_dispatch_gate()closes the orchestrator's existing train dispatch gate (dispatcher.dispatch_allowed). Train scheduling then stops, while evals and in-flight episodes keep flowing. The gate reopens when the count falls below the cap. Only groups already in flight can still complete and start finalizing, sofinalizingis bounded by the cap plus the groups in flight.out_qbecause it does not stall evals, and it also boundsout_qwhen no inflight ceiling is configured.max_off_policy_steps: right afterprogress.stepadvances,cancel_stale_finalizing()cancels finalizing groups that would already be stale on insertion. They are accounted exactly like astalegroup cancellation: episodes cancelled and not admitted, the zero-output tally advanced, failures and cancelled attempts booked. A group whose task has already finished is not cancelled; the insertion sweep_drop_stalehandles it and remains the hard guarantee.start_drainingcallstrain_sink.stop(). Finalizing groups whose batches would be dropped anyway are cancelled; drain does not wait for them.Orchestrator.stop()also callstrain_sink.stop().finalize_groupis stored by the task's done-callback._raise_if_component_stopped()re-raises it at the top of the next loop iteration (at most ~0.5 s later), with the original traceback. Errors are never swallowed.ProgressandTrainSource(curriculum) state. The curriculum sees a group only when it is settled, so finalizing groups are lost on restart like in-flight rollouts. No changes were needed inckpt.py.(+N buffered)pipeline count includes the episodes and failures of finalizing groups.out_qpoll timeout. That is negligible next to minute-scale finalization.docs/algorithms.mdnow says that background-finalized algorithms runfinalize_groupconcurrently and complete in any order, so stateful ones must tolerate interleaved groups.What I ran
pytest tests/unit/orchestrator: 92 passed.ruff checkandruff format --checkonsrcare clean. (On the earlier revision, the fulltests/unitrun had the same failure set asorigin/main; all of those are trainer/GPU tests, and no GPU is available here.)origin/main'sTrainSinkand this branch's. The stream mixes interleaved groups, errored traces, dispatch failures, overload and stale cancellations, policy-version bumps, and a curriculum that rejects some groups. It uses realGRPOAlgorithmand covers 12 seeds × {trace, token batching} × {constant_trainer_batch_size on/off} × max_off_policy_steps ∈ {1, 2, 8}.stale_drops, the zero-output tally, the final pending state, and the order of curriculum calls.cancel_stale_finalizing()cancels only the stale group, with exact stale accounting.finalizing_fullfollowsmax_finalizing_groups.stop()cancels the rest and leaves the queue empty.finalized()on a dropped id returnsNone.raise_if_failed()re-raises the error.GPU / e2e checks not run here
off_policy/*metrics, including aconstant_trainer_batch_sizerun.max_steps, the final checkpoint, and resume from it.finalizing(pipeline log+N buffered) and orchestrator RSS over many steps; the dispatch pause and resume atmax_finalizing_groups.Landing order
verifiers + prime-envs -> prime-rl PR1 (this PR, background finalization) -> prime-rl PR4 (gar, #3803)