Skip to content

feat: first-class sft+opd training modes - #2546

Merged
mikasenghaas merged 52 commits into
mainfrom
mika/student-eval-pool-for-sft-distill
May 19, 2026
Merged

mikasenghaas merged 52 commits into
mainfrom
mika/student-eval-pool-for-sft-distill

Conversation

@mikasenghaas

@mikasenghaas mikasenghaas commented May 18, 2026

Copy link
Copy Markdown
Member

Note

Stacks on top of #2476 (open). Once #2476 merges to main, this PR will rebase cleanly. Until then, the diff vs main includes #2476's commits as well.

Summary

  • Unify OrchestratorConfig model fields: rename modelstudent and teacher_modelteacher with backward-compat aliases (AliasChoices). Both old TOML names still parse.
  • orchestrator.training_mode is the single source of truth (rl / opd / sft). Set it under [orchestrator]; the orchestrator stamps every TrainingSample.training_mode from this field.
  • Batch-driven loss dispatch: TrainingSample.training_mode flows through MicroBatch into compute_loss, which looks up the right loss fn per batch:
    • rldefault_loss_fn (uses trainer.loss knobs — adv_tau, dppo_mask_*, kl_tau)
    • opdopd_loss_fn (self-contained; taus + dppo + kl knobs inlined as literals, matching DefaultLossConfig's defaults)
    • sftsft_loss_fn (masked NLL on teacher tokens)
      Packer enforces same-mode packing; same-mode batches alone share a micro batch.
  • trainer.loss: DefaultLossConfig | CustomLossConfig (discriminated union, drop the old SFTLossConfig). Applies only to rl-mode batches; opd and sft don't read it. CustomLossConfig overrides the rl-mode loss fn.
  • Two-pool orchestrator: replace the "one pool + teacher-client overlay" with explicit student_inference: InferencePool (always required, weight-sync + eval target) and teacher_inference: InferencePool | None (set in opd/sft; rollout source for sft, logprob source for opd). Scheduler resolves the rollout pool internally from training_mode. The legacy overlay path that mapped student-pinned clients onto a separate teacher client set is gone; fixes a latent bug where SFT-mode LoRA updates would overwrite scheduler.model_name.
  • ClientConfig.headers_from_env — env-var-resolved HTTP headers (analogue of api_key_var). Auto-injects X-Prime-Team-ID → PRIME_TEAM_ID when the base URL targets pinference.ai.
  • Debug configs for the training modes at configs/debug/training_modes/{rl,opd,opd_lora,sft,sft_lora,sft_external}.toml against PrimeIntellect/Qwen3-0.6B-Reverse-Text-SFT (student). Local-teacher variants use Qwen3-0.6B-Reverse-Text-RL on :8001; sft_external points SFT at openai/gpt-5-mini via PI inference. Each config has its teacher-start command as a comment at the top, plus a README at configs/debug/training_modes/README.md.
  • docs/training_modes.md — grid of what student/teacher do in each mode, what must be configured, and the key implications (e.g. OPD teacher must be a vLLM because compute_teacher_logprobs uses vLLM's /inference/v1/generate; SFT teacher can be any OAI-compatible endpoint). Merged the standalone OPD doc into this page.
  • Drop check_gpus_available runtime check in the RL entrypoint — it raised RuntimeError when other processes held GPUs, which blocked iterating with a long-lived teacher inference server on a sibling GPU.
  • Legacy-layout shims: OrchestratorConfig now accepts both legacy [orchestrator.client.*] and flat [orchestrator.model.<k>] syntaxes via one before-validator, so existing TOMLs (elastic, ci/integration LoRA configs, examples) parse without modification.

Notable design decisions

  • Loss dispatch is per-batch, not per-config: the trainer is mode-agnostic — setup_loss_fns always returns all three keys (rl, opd, sft). At the trainer level, mixed-mode batches in one run dispatch correctly. (Orchestrator-side mixed-mode would need per-sample mode selection — separate concern.)
  • opd_loss_fn is self-contained: takes only LossInputs, no loss_config. The DPPO/KL knobs (0.2 / 0.2 / 1e-3) and the tau values (0.0 / 1.0) are inlined as literals. Users can't tune them from trainer.loss (which is rl-only by design). If you need to customize, edit opd_loss_fn directly.
  • SFTLossConfig deleted: zero-field discriminator that was already implied by training_mode = "sft". trainer.loss collapsed to DefaultLossConfig | CustomLossConfig.
  • Headers auto-config triggers only on pinference.ai: explicit allowlist; other PI URLs / external endpoints still require the user to set headers_from_env explicitly.
  • logprobs stripped at runtime in sft mode: external reasoning-model endpoints (openai/gpt-5*) reject logprobs=True and the trainer doesn't read it in sft anyway, so the orchestrator pops it from each TrainEnv.sampling_args at startup.

Behavior change

SFT now requires [inference] (or an explicitly-set orchestrator.student.client.base_url pointing at a running student server). Previously SFT could run without a student inference server (teacher-only rollouts, no online evals, no weight sync). After this refactor the orchestrator unconditionally sets up student_inference and waits for it to be ready, so every SFT run needs a student vLLM. If you previously relied on num_infer_gpus = 0 SFT runs, add [inference] (or point orchestrator.student.client.base_url at an externally-started student server). The rl entrypoint logs a warning when [inference] is missing in any mode.

Configs

Orchestrator-side blocks per training mode. The rest of the TOML (model, trainer, inference, etc.) is unchanged.

RL — orchestrator config is unchanged from before this PR.

[orchestrator]
training_mode = "rl"
batch_size = 128
rollouts_per_example = 16

[orchestrator.train.sampling]
max_completion_tokens = 128

[[orchestrator.train.env]]
id = "my-env"

OPD — adds [orchestrator.teacher.*] pointing at a vLLM-backed teacher. opd_loss_fn runs automatically (taus + dppo/kl knobs are inlined; no [trainer.loss] needed).

[orchestrator]
training_mode = "opd"
batch_size = 128
rollouts_per_example = 16

[orchestrator.train.sampling]
max_completion_tokens = 128

[[orchestrator.train.env]]
id = "my-env"

[orchestrator.teacher.model]
name = "PrimeIntellect/Qwen3-0.6B-Reverse-Text-RL"

[orchestrator.teacher.client]
base_url = ["http://localhost:8001/v1"]   # must expose vLLM's /inference/v1/generate

SFT — adds [orchestrator.teacher.*] pointing at any OAI-compatible endpoint (PI inference, OpenAI, local vLLM…). Set use_renderer = false (validator-required for sft).

[orchestrator]
training_mode = "sft"
batch_size = 128
rollouts_per_example = 4
use_renderer = false

[orchestrator.train.sampling]
max_completion_tokens = 128

[[orchestrator.train.env]]
id = "my-env"

[orchestrator.teacher.model]
name = "qwen/qwen3-30b-a3b-instruct-2507"

[orchestrator.teacher.client]
base_url = ["https://api.pinference.ai/api/v1"]
api_key_var = "PRIME_API_KEY"
# X-Prime-Team-ID auto-injected from $PRIME_TEAM_ID for pinference.ai URLs

Verification

Ran each training mode end-to-end against the reverse-text env with WANDB_MODE=offline, 3 steps. All three exited cleanly (exit code 0, SUCCESS RL training finished! in the log).

Setup: GPU 0 hosts the auto-launched student vLLM (PrimeIntellect/Qwen3-0.6B-Reverse-Text-SFT); GPU 1 hosts a manually started teacher vLLM (PrimeIntellect/Qwen3-0.6B-Reverse-Text-RL) on port 8001 — only needed for opd / sft (the local-teacher variants):

CUDA_VISIBLE_DEVICES=1 uv run inference \
  --model.name PrimeIntellect/Qwen3-0.6B-Reverse-Text-RL \
  --server.port 8001 --gpu-memory-utilization 0.5 --model.enforce-eager
Screenshot 2026-05-18 at 7 07 11 PM
Mode Command Outcome
rl WANDB_MODE=offline uv run rl @ configs/debug/training_modes/rl.toml --clean-output-dir --max-steps 3 --wandb.shared false 3 train steps + eval at step 1; clean exit.
opd WANDB_MODE=offline uv run rl @ configs/debug/training_modes/opd.toml --clean-output-dir --max-steps 3 --wandb.shared false 3 train steps; teacher logprobs computed each step (Computing teacher logprobs for 128 training examples); clean exit.
sft (local teacher) WANDB_MODE=offline uv run rl @ configs/debug/training_modes/sft.toml --clean-output-dir --max-steps 3 --wandb.shared false 3 train steps; rollouts dispatched to teacher pool, student inference used for evals + weight sync; clean exit.
sft (external teacher, openai/gpt-5-mini via PI inference) uv run rl @ configs/debug/training_modes/sft_external.toml --clean-output-dir rollouts dispatched to PI inference, X-Prime-Team-ID auto-injected from PRIME_TEAM_ID, train+eval working. Reasoning-model budget tuned via max_completion_tokens = 2048 + reasoning_effort = "minimal"; logprobs stripped at runtime so the OAI endpoint accepts the request.

Also: uv run pytest tests/unit/ -m "not gpu" -q → 355 passed.


Note

High Risk
High risk because it refactors core training/orchestration wiring (config schema, inference pool setup, weight sync, and loss computation) and removes/renames several user-facing config fields, which can break existing runs if shims miss edge cases.

Overview
Adds a first-class orchestrator.training_mode (rl/opd/sft) that is stamped onto each TrainingSample.training_mode and used end-to-end to drive behavior.

Refactors config and runtime wiring to match: OrchestratorConfig renames modelstudent and consolidates teacher configuration into teacher (with back-compat aliases/shims for common legacy layouts), while removing orchestrator.use_sft_loss and orchestrator.teacher_rollout_model. The orchestrator now always brings up a student inference pool (evals + weight sync) and optionally a teacher pool (rollouts for sft, logprobs for opd), and Scheduler selects the rollout pool based on mode.

Updates the trainer to be mode-agnostic: replaces the sft_loss boolean with training_mode, deletes SFTLossConfig and DefaultLossConfig.teacher_tau, adds an opd_loss_fn, and dispatches loss per micro-batch via setup_loss_fns(); packing prevents mixing modes in a micro-batch. Also adds ClientConfig.headers_from_env for env-resolved HTTP headers, refreshes docs (new docs/training_modes.md, removes legacy OPD doc), and adds debug configs for all modes under configs/debug/training_modes/.

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

tim0120 and others added 21 commits May 12, 2026 02:11
When teacher_rollout_model is configured for SFT distillation, the
orchestrator now supports a separate student inference pool for online
evals and weight sync. Previously, configuring [inference] alongside
teacher_rollout_model was forbidden — evals either ran on the teacher
or were skipped entirely.

Changes:
- Relax RLConfig validator to allow [inference] + teacher_rollout_model
- Create eval_inference_pool from config.client when teacher_rollout_model
  is set, pointing at the student vLLM server
- Route eval calls and weight updates to the student pool
- Add eval_inference_pool param to Scheduler for weight sync targeting
- All existing RL/soft-distill paths are unchanged (eval_inference_pool
  defaults to inference_pool when no teacher_rollout_model is configured)

Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
Consolidates 3 training modes (rl, opd, sft) under a unified config
structure: `student` (always present) and `teacher` (optional, role
determined by `training_mode`). Removes scattered flags (`use_sft_loss`,
`update_student_inference_weights`) in favour of a single discriminator.

Backward-compat aliases (`model` → `student`, `teacher_model` → `teacher`)
keep existing TOML configs parsing without changes.

Also adds `configs/reverse_text/debug_{rl,opd,sft}.toml` for local
mode debugging.

Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
qwen/qwen3-4b-instruct doesn't exist on PI inference;
use Qwen/Qwen3-4B-Instruct-2507 instead.

Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
Maps header names to env var names, resolved at client setup time.
Analogous to api_key_var but for arbitrary headers. Useful for e.g.
X-Prime-Team-ID when hitting the PI inference API.

Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
When ClientConfig.base_url targets pinference.ai, auto-add a
headers_from_env mapping for X-Prime-Team-ID -> PRIME_TEAM_ID so team
billing works without each config repeating the boilerplate.

Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
…rence

PI inference is OpenAI-compatible but doesn't expose vLLM's
/inference/v1/generate prefill endpoint that compute_teacher_logprobs
needs - so OPD against PI inference 404s. SFT mode kept working
because that path only hits /chat/completions.

Switch debug_opd.toml to use the existing teacher_inference auto-setup:
set num_teacher_gpus = 1, and an empty [orchestrator.teacher] block to
satisfy the orchestrator validator (which runs before
auto_setup_teacher_inference can populate it). The validator chain
spins up a local vLLM teacher on port 8001 using the same reverse-text
student model.

Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
- Add docs/training_modes.md with the rl/opd/sft role grid, implications
  (e.g. OPD needs local vLLM teacher, SFT teacher is any OAI-compatible
  endpoint), and minimal config per mode. Linked from mint.json nav.
- Add validate_training_mode_loss_consistency on RLConfig enforcing:
  sft mode <-> sft loss type, opd mode requires teacher_tau > 0,
  non-sft modes forbid sft loss type. These cross orchestrator and
  trainer fields so they live on RLConfig (per-component validators in
  OrchestratorConfig already cover the within-orchestrator constraints).
- Fix debug_sft.toml to set trainer.loss.type = "sft" (was relying on
  default loss which is now explicitly rejected).
- Make training_mode explicit in debug_rl.toml and debug_opd.toml.

Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
…t local vLLM

- Add shared training_mode field to RLConfig (rl | opd | sft). A mode="before"
  validator propagates it into orchestrator.training_mode and (for sft)
  trainer.loss.type before nested validation runs. Must be a before-validator
  because OrchestratorConfig.validate_training_mode would otherwise reject
  configs like training_mode = "opd" + [orchestrator.teacher] - the
  orchestrator-level default of "rl" would conflict with the teacher block
  before the after-validator could propagate the shared value.

- Point debug_opd and debug_sft at a manually-deployed teacher vLLM
  (Qwen3-0.6B-Reverse-Text-RL on localhost:8001). Add a comment with the
  exact teacher start command at the top of each config. Drop the
  num_teacher_gpus auto-launch from debug_opd since we now use the
  external teacher.

- Set student inference gpu_memory_utilization = 0.5 in all three debug
  configs (matches the teacher's mem budget, leaves room when both are
  co-located).

- Add configs/reverse_text/README.md with the start commands.

Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
- Remove check_gpus_available in rl entrypoint - it raised RuntimeError
  when other processes held GPUs, which blocked iterating with a
  long-lived teacher inference server on a sibling GPU.
- Update debug_opd / debug_sft / README to start the teacher via
  uv run inference (the prime-rl variant) instead of raw vllm serve.
  Same port (8001), same model, same flags.

Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
@mikasenghaas mikasenghaas changed the title feat(configs): training modes + shared training_mode + debug configs feat: first-class sft+opd training modes May 18, 2026
mikasenghaas and others added 8 commits May 18, 2026 23:06
Pre-refactor, orchestrator.model was a ModelConfig directly (name, lora,
trust_remote_code, vlm). After the student/teacher rename it's a
RolloutModelConfig wrapping ModelConfig + ClientConfig, so existing
[orchestrator.model.lora] TOML sections stopped parsing because they
landed on the wrong nesting level.

Add a mode="before" validator on RolloutModelConfig that detects a flat
ModelConfig dict (any of {name, trust_remote_code, vlm, lora} at the top
level, no nested model/client keys) and re-nests it under "model". This
lets the three existing LoRA configs revert to their original syntax.

Also fix examples/alphabet_sort/sft_distill_hard.toml: hoist training_mode
to the top-level shared field so trainer.loss.type = "sft" gets auto-set
(previously the orchestrator-only flag triggered the new sft/loss
consistency validator).

Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
…nce pools

Replace the "one pool plus optional teacher-client overlay" model with two
explicit pools:

- student_inference: InferencePool | None
  - Required for rl/opd, optional for sft (set iff [inference] is configured)
  - Target for evals, weight broadcast, and inference metrics
  - When None, online evals and policy updates are skipped automatically
- teacher_inference: InferencePool | None
  - Set whenever orchestrator.teacher is configured (opd or sft)
  - Source of teacher logprobs in opd
  - Source of train rollouts in sft
  - Always MITO (chat completions) for simplicity; external OAI-compatible
    teachers (PI inference, OpenAI) work as drop-in endpoints

Scheduler now takes both pools and resolves the rollout target internally
(rollout_inference = teacher_inference if sft else student_inference).
The old _resolve_rollout_request_target overlay is gone - the rollout pool
serves rollouts directly. Fixes a latent bug where SFT-mode LoRA updates
would overwrite scheduler.model_name and route subsequent teacher requests
with the student's LoRA name.

Also:
- Drop setup_external_rollout_model (logic is now inline + clearer)
- Rename setup_rollout_inference_pool -> setup_student_inference_pool;
  always handles only the student pool, teacher is plain setup_inference_pool
- Log the resolved training_mode + one-line description at orchestrator start
- Update tests: cover both sft (rollout != student) and rl (rollout = student)
  LoRA paths; drop the now-obsolete teacher-overlay scheduler test

Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
…training_modes

- Add _accept_top_level_client validator on OrchestratorConfig: legacy
  [orchestrator.client...] re-nests under [orchestrator.student.client...]
  matching the existing _accept_flat_model_layout shim pattern. Revert
  configs/elastic/rl.toml back to the original [orchestrator.client.elastic]
  syntax.
- Revert configs/ci/integration/reverse_text_multi_run/orchestrator.toml
  [model.model] -> [model]; the flat-layout shim on RolloutModelConfig
  already handles it (verified).
- Merge docs/on_policy_distillation.md into docs/training_modes.md. The
  training_modes page is now the single entry point; OPD-specific details
  (external teacher, pure distillation, monitoring, VLM, params) live as
  subsections. Delete the standalone OPD page.
- Drop the redundant "Initializing static inference pool" / WandbMonitor /
  PrimeMonitor init logs - they duplicate the immediately-preceding
  per-pool init line.

Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
In OPD mode, send a tiny probe request to the teacher's /inference/v1/generate
endpoint after the pool is ready. If the endpoint 404s or doesn't return
prompt_logprobs, raise an informative error pointing at docs/training_modes.md
instead of crashing mid-training with a 404.

Verified that PI inference fails this probe on all three plausible routes:
- /inference/v1/generate -> 404
- /v1/chat/completions with logprobs=true -> logprobs:null in response
- /v1/completions with echo=true,logprobs=1 -> 404

Also add configs/reverse_text/debug_sft_thinking.toml: SFT from
qwen3-30b-a3b-thinking-2507 via PI inference (sft path needs only
chat completions, so PI inference is fine here).

Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
Drop the server-tokenized TITO path (``openai_chat_completions_token`` /
``/v1/chat/completions/tokens``). The orchestrator now picks between
renderer-backed TITO (``use_renderer = true``, default) and MITO
(``use_renderer = false``, fallback for VLMs and external teacher rollouts).

- Remove ``OrchestratorConfig.use_token_client`` field and its validators
- Drop the TITO branch in ``setup_rollout_inference_pool``
- Drop the obsolete TITO warning in ``setup_inference_pool`` and flip the
  default ``train_client_type`` to ``openai_chat_completions`` in
  ``StaticInferencePool`` / ``ElasticInferencePool``
- Strip ``use_token_client = false`` from configs/examples/docs that
  carried the no-op fallback line
- CHANGELOG entry documenting the breaking removal

Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Drop the ``/v1/chat/completions/tokens`` endpoint and the
``OpenAIServingChatWithTokens`` / ``ChatCompletionRequestWithTokens``
wrappers now that no client routes to them. Also drop the obsolete
``base()`` helper and unused imports from ``server.py``.

Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Student inference was already required for rl/opd and optional for sft;
making it required everywhere simplifies the orchestrator + scheduler:

- Drop has_student_inference / enable_policy_updates gating
- student_inference: InferencePool (no longer Optional) in Scheduler.__init__
- Weight broadcast init, weight sync, metrics collector, eval routing, and
  shutdown all unconditionally use student_inference
- Drop the now-stale OPD probe (will revisit in a follow-up PR)
- Add is_vlm property on BaseModelConfig; replace local is_vlm vars in the
  orchestrator with config.student.model.is_vlm
- Merge the two legacy student-layout shims on OrchestratorConfig into one
  before-validator (_accept_legacy_student_layout) covering both
  [orchestrator.client.*] and flat [orchestrator.model.<k>] paths
- Drop redundant pool / WandbMonitor / PrimeMonitor init log lines
- Reorder OrchestratorConfig fields: training_mode > student > teacher
- Drop "OAI" from RolloutModelConfig.client description
- Update docs/training_modes.md: SFT student inference is now required;
  evals and weight sync are unconditional

Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
Comment thread src/prime_rl/orchestrator/orchestrator.py
mikasenghaas and others added 5 commits May 19, 2026 03:11
…sft)

After the always-require-student refactor, sft runs need a student
inference pool too (orchestrator unconditionally creates it for evals +
weight sync). The old "sft mode, using teacher for rollouts" info log
was correct under the old semantics but now silently puts the user on a
path where the orchestrator hangs waiting for a non-existent student
server at the default localhost:8000. Replace with a single warning
that prints the configured student base_url and flags the hang risk.

Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
…acher_tau=1)

Mirror default_loss_fn's structure literally: declare the two tau locals at
the top, then use them in the same `adv_tau * advantages + teacher_tau *
teacher_kl.detach()` expression. The "this is the default loss with those
specific knobs baked in" relationship is now visible in the code rather than
hidden in a substituted expression.

Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
…(rl-only)

- DefaultLossConfig regains adv_tau (= 1.0). default_loss_fn reads it.
- trainer.loss is back to a discriminated union of DefaultLossConfig |
  CustomLossConfig (drop SFTLossConfig stays dropped). type discriminator
  restored.
- trainer.loss only applies to rl-mode batches. opd and sft batches dispatch
  unconditionally to opd_loss_fn / sft_loss_fn - they don't read trainer.loss.
- opd_loss_fn is self-contained: the DPPO/KL knobs (dppo_mask_*, kl_tau)
  are baked in as module-level _OPD_* constants matching DefaultLossConfig's
  defaults. No loss_config parameter.
- Drop the trainer.custom_loss sibling field (subsumed by the union).

Also add debug_opd_lora.toml + bump lr to 1e-4 in both lora debug configs.

Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
Drop the module-level _OPD_* constants and the local adv_tau/teacher_tau
bindings - inline the literal values into the math. Keeps the parallel
structure to default_loss_fn visible: `0.0 * advantages + 1.0 * teacher_kl`
in the same place default_loss_fn has `loss_config.adv_tau * advantages`.

Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
@mikasenghaas
mikasenghaas requested a review from tim0120 May 19, 2026 03:51
Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
@mikasenghaas
mikasenghaas marked this pull request as ready for review May 19, 2026 03:58
Comment thread packages/prime-rl-configs/src/prime_rl/configs/trainer.py
- OrchestratorConfig auto-sets use_renderer=False when training_mode='sft'
  via an after-validator placed before the renderer validators in
  declaration order, so the user doesn't have to set it. Drop the rejecting
  branch in validate_training_mode + the explicit use_renderer=false lines
  in the three sft debug configs.
- CHANGELOG: add a "first-class training_mode + batch-driven loss dispatch"
  entry covering the breaking removals (orchestrator.use_sft_loss,
  orchestrator.teacher_rollout_model, trainer.loss.type='sft' /
  SFTLossConfig, trainer.loss.teacher_tau) with migration notes.
- _accept_legacy_student_layout: use set(ModelConfig.model_fields) instead
  of hardcoding the field list, so new ModelConfig fields are picked up
  automatically.
- Trim the orchestrator startup log to just '(<training_mode>)'; the
  prose description was redundant with docs/training_modes.md.

Co-Authored-By: Claude Sonnet 4.6 <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 90e75d9. Configure here.

Comment thread packages/prime-rl-configs/src/prime_rl/configs/orchestrator.py

@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.

can we remove the debug config ?

Comment thread docs/training_modes.md Outdated
Comment thread docs/training_modes.md Outdated
train: TrainConfig = TrainConfig()
# Training mode: drives validation and runtime wiring
training_mode: Annotated[
Literal["rl", "opd", "sft"],

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.

still not convinced sft is the right term tho

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

hm yea, you have a better suggestion? also i think @willccbb wanted to ship some env features that makes it more like traditional sft

@mikasenghaas

Copy link
Copy Markdown
Member Author

can we remove the debug config?

would strongly prefer to keep these for internal documentation and dev. can maybe ship into configs/debug/ tho?

Comment on lines +1127 to +1132
@model_validator(mode="before")
@classmethod
def _accept_legacy_student_layout(cls, data: Any) -> Any:
"""Backward-compat shims for the pre-refactor student layout.

Pre-refactor OrchestratorConfig had top-level `model: ModelConfig` and

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.

hmmmm wondering if its not better to failed here, I think that its fine to break backward compat because this feature was not the most used ?

not sure about removing it

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

no this we prob need to keep -- otherwise model.name doesn't work anymore which imo is the more natural way to configure an RL run (which doesn't have a teacher)

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

going to adjust the name and docstring tho to make this clearer

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.

huhuhuhuhhuhuhuuhuhh but isn't it an easier way to do it hmmmmmmmmmmmmmmm

@mikasenghaas mikasenghaas May 19, 2026

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

im all ears

Comment thread packages/prime-rl-configs/src/prime_rl/configs/shared.py Outdated
mikasenghaas and others added 6 commits May 19, 2026 04:25
Add a mode="before" validator on RLConfig that stubs an empty
[orchestrator.teacher] block when deployment.num_teacher_gpus is set and
the user didn't write one. Without this, OrchestratorConfig.validate_training_mode
("opd requires orchestrator.teacher") fires during nested validation - before
auto_setup_teacher_inference can wire the teacher from teacher_inference.

Repro that now passes:

  RLConfig.model_validate({
      "deployment": {"num_teacher_gpus": 1},
      "trainer": {},
      "orchestrator": {"training_mode": "opd"},
      "inference": {},
  })

-> auto_setup_teacher_inference fills in orchestrator.teacher.client.base_url
   = ["http://localhost:8001/v1"] and model.name from teacher_inference.

Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
…her note

Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
…ining_modes/

The reverse-text debug configs are about exercising the training modes, not
about reverse-text specifically. Move to configs/debug/training_modes/ and
drop the debug_ prefix from each filename (the directory name carries it).

  configs/reverse_text/debug_rl.toml          -> configs/debug/training_modes/rl.toml
  configs/reverse_text/debug_opd.toml         -> configs/debug/training_modes/opd.toml
  configs/reverse_text/debug_opd_lora.toml    -> configs/debug/training_modes/opd_lora.toml
  configs/reverse_text/debug_sft.toml         -> configs/debug/training_modes/sft.toml
  configs/reverse_text/debug_sft_lora.toml    -> configs/debug/training_modes/sft_lora.toml
  configs/reverse_text/debug_sft_external.toml -> configs/debug/training_modes/sft_external.toml
  configs/reverse_text/README.md              -> configs/debug/training_modes/README.md

Update path references in each TOML's header comment, the README, and
docs/training_modes.md.

Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
…cut shim

- ClientConfig.auto_setup_pinference_team_header dropped. The hostname-sniff
  was implicit magic. Configs that need the X-Prime-Team-ID header against
  PI inference set it explicitly under [...client.headers_from_env] now.
  Update configs/debug/training_modes/sft_external.toml accordingly and
  drop the auto-injection note from the file header.
- Rename OrchestratorConfig._accept_legacy_student_layout (with leading
  underscore + "backward-compat" framing) to fold_student_shortcuts. The
  shim is more than a back-compat: it's the ergonomic path for rl configs
  too, letting users write [orchestrator.model.name] / [orchestrator.client]
  rather than [orchestrator.student.model.name] / [orchestrator.student.client].
  Docstring reframed accordingly. Teacher has no equivalent shortcut on
  purpose: rl mode forbids teacher, so the same shortcut routing to two
  roles would be ambiguous.

Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
test_reverse_text_multi_run.start_orchestrator passes --client.base-url
and --model.lora.name as CLI flags to `uv run orchestrator`. After the
student rename, those tyro args don't exist anymore (they're now
--student.client.base-url and --student.model.lora.name). tyro rejects
them before the fold_student_shortcuts before-validator can re-nest the
dict, so the orchestrator process exits immediately and the test times
out waiting for "Step 11" to appear in the log.

Rename both CLI args to their new student-scoped form.

Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
@mikasenghaas
mikasenghaas requested a review from samsja May 19, 2026 06:28
@mikasenghaas
mikasenghaas merged commit eaf9599 into main May 19, 2026
16 checks passed
mikasenghaas added a commit that referenced this pull request May 19, 2026
Resolves conflicts in:
  - packages/prime-rl-configs/src/prime_rl/configs/rl.py: keep the before-
    validator approach (auto_setup_shared_configs) from this branch and drop
    main's mode='after' propagation validators that it replaces.
  - tests/unit/test_configs.py: keep both branches' new tests.

Adapts to main's #2546 ("first-class sft+opd training modes"):
  - Drops validate_teacher_model / validate_external_rollout_inference from
    rl.py — both referenced fields (trainer.loss.teacher_tau,
    orchestrator.teacher_rollout_model) that no longer exist after #2546
    restructured the loss and teacher configs around training_mode. The
    semantic checks they enforced are now expressed via training_mode.
  - Fixes auto_setup_session_headers on OrchestratorConfig to use
    student.client (path post-#2546) instead of self.client.
  - Updates the propagation tests to assert on orchestrator.student.model.name
    (the post-#2546 path) and to opt out of orchestrator.use_renderer's
    MODEL_RENDERER_MAP check when using fake model names.

Also drops _dump_preserving_discriminators from auto_setup_shared_configs:
once pydantic-config PR #8 is in (already pinned in pyproject.toml + the
submodule), cli() runs the user's validators exactly once on the merged
TOML + CLI dict, so the validator only ever sees plain dict-of-dicts input
and the discriminator-preserving dump-and-rewrap dance is no longer needed.

Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
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