Skip to content

chore(orchestrator): fold GenerationSource into TrainEnv - #3838

Merged
faresobeid merged 1 commit into
mainfrom
chore/fold-generation-source
Oct 8, 2026
Merged

faresobeid merged 1 commit into
mainfrom
chore/fold-generation-source

Conversation

@faresobeid

@faresobeid faresobeid commented Oct 4, 2026 •

Copy link
Copy Markdown
Contributor

Summary

Fold GenerationSource into TrainEnv, its only owner. Move connect_frozen_client from algo/base.py to clients.py, so algo no longer imports clients lazily. Give TrainEnv setup()/aclose() so orchestrator setup and teardown no longer reach into its parts.

What changed

  • orchestrator/generation_source.py is deleted. TrainEnv (envs.py) now holds clients, renderer_config, connected (the frozen source clients it connected) and uses_live_policy. Dropping logprobs for frozen sources happens in TrainEnv.__init__.
  • TrainEnv.setup() connects the frozen generation source and runs algorithm.setup() concurrently. TrainEnv.aclose() closes connected and algorithm.connected, never the shared policy clients.
  • connect_frozen_client is a plain function in clients.py, next to check_inference_ready. algo/base.py imports it at module level. It is no longer re-exported from prime_rl.orchestrator.algo.
  • dispatcher.py reads the TrainEnv directly at the four former generation_source call sites. orchestrator.py calls env.setup() and env.aclose().
  • docs/algorithms.md and the algo/__init__.py docstrings refer to TrainEnv.

Behavior change

None.

Checks run

  • pytest tests/unit/orchestrator tests/unit/test_configs* -m "not gpu": 297 passed.
  • ruff check, ruff format --check: clean.
  • Equivalence script, old GenerationSource (origin/main) vs new TrainEnv, on 4 configs: policy, policy with top_p=0.9, sft with a frozen sampling.source, opd with a frozen teacher. Identical sampling_args, requires_sampling_masks and uses_live_policy. After setup(), the same generation clients (policy object, or the frozen endpoint with the same train client config) and the same algorithm clients. Teardown closes the same clients.
  • GPU smoke on one node (vLLM 0.31): reverse-text RL (15 steps), sft with a frozen sampling.source (5 steps), opd with a frozen teacher (5 steps). All three ran to "Training finished" and exited 0.

Note

Low Risk
Internal orchestrator refactor with stated behavioral equivalence and no changes to training algorithms or dispatch semantics beyond indirection.

Overview
Refactors train rollout sourcing by deleting GenerationSource and moving its responsibilities onto TrainEnv: inference clients, live-vs-frozen policy detection (uses_live_policy), sampling-arg tweaks (e.g. dropping logprobs for frozen endpoints), and frozen-pool connection in setup().

TrainEnv.setup() now connects frozen sampling.source endpoints and runs algorithm.setup() together; TrainEnv.aclose() closes only env-owned clients (connected + algorithm pools), not the shared policy pool. The orchestrator and dispatcher call these surfaces instead of reaching into a nested generation_source.

connect_frozen_client moves from algo/base.py to clients.py (next to check_inference_ready); it is no longer re-exported from prime_rl.orchestrator.algo. Docs and package docstrings now describe TrainEnv as owning the sampling half of the algorithm config.

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

@faresobeid
faresobeid marked this pull request as ready for review October 4, 2026 12:28
@samsja samsja closed this Oct 4, 2026
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 -->
@faresobeid faresobeid reopened this Oct 7, 2026
@faresobeid
faresobeid force-pushed the chore/fold-generation-source branch from 2693eb4 to e89ca1a Compare October 7, 2026 18:58
@faresobeid
faresobeid merged commit fec3acd into main Oct 8, 2026
19 of 20 checks passed
@faresobeid
faresobeid deleted the chore/fold-generation-source branch October 8, 2026 10:52
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.

3 participants