Skip to content

refactor(configs): one trainer-common config base for TrainerConfig and SFTConfig - #3849

Merged
samsja merged 2 commits into
mainfrom
refactor/trainer-config-base
Oct 19, 2026
Merged

samsja merged 2 commits into
mainfrom
refactor/trainer-config-base

Conversation

@faresobeid

@faresobeid faresobeid commented Oct 4, 2026 •

Copy link
Copy Markdown
Contributor

Summary

SFTConfig duplicated most of TrainerConfig. This PR adds BaseTrainerConfig in configs/trainer.py with the shared fields and validators. TrainerConfig and SFTConfig both subclass it.

  • Shared fields: model, tokenizer, optim, scheduler, ckpt, log, matmul_precision, max_steps, memory_profiler_path, gc, trace_path, dist_timeout_seconds, heartbeat, control, env_vars.

  • Shared validators: deepep_disables_grad_clipping, full_optimizer_offload_requires_supported_optimizer, full_optimizer_offload_disables_grad_clipping, vlm_freeze_incompatible_with_lora, dont_do_massive_traces, validate_scheduler_steps, auto_setup_tokenizer.

  • The three "auto" settings that resolve differently are class variables, resolved by one resolve_auto_defaults validator (replaces three validators per class):

    class variable RL (TrainerConfig) SFT (SFTConfig)
    auto_moe_router_dtype float32 bfloat16
    auto_freeze_moe_router True False
    auto_weight_decay 0.0 0.01
  • resume, monitors, output_dir and weight_broadcast stay per class: their types or meanings differ.

Behavior

No runtime change. Defaults, resolved values, warnings and validation errors are the same.

Visible differences:

  • SFT help text uses the shared wording for two fields: max_steps ("Maximum number of training steps.") and env_vars ("...for the trainer process(es)...").
  • Field order. Shared fields come first in model_fields, --help and resolved-config dumps. The set of fields and their values do not change.
  • After-validator order. Shared validators run before class-specific ones. No class-specific validator reads a value that a moved validator sets. If a config has several errors, a different one may be reported first.

Proof

  • Differential dump against main at the PR's original base: every TOML under configs/ and examples/ (78 files; there is no k8s/ TOML), standalone and composed with each sibling TOML, plus 18 CLI override cases (auto overrides, trace/max_steps, full offload, Muon + FSDP offload, cosine scheduler, DeepEP, LoRA, tokenizer). Each case was parsed with all 7 config classes, with run.name uuids pinned. That is 2310 cases, 245 of which parse. model_dump, error messages and warnings are identical.
  • model_json_schema() is identical for all 7 config classes.
  • --help: TrainerConfig and RLConfig have the same lines (order only). SFTConfig differs only in the two descriptions above.
  • On current main (52455b4): every TrainerConfig / SFTConfig field has the same default and type as on main (including dist_timeout_seconds = 900 from feat(trainer): 900 s collective timeout, flight recorder dumps, master-only waits off NCCL #4019).
  • pytest tests/unit/test_configs.py tests/unit/orchestrator -m "not gpu": 336 passed.
  • ruff check and ruff format --check are clean.

Note

Low Risk
Refactor-only config hierarchy with parity testing claimed; no training logic changes, only consolidation and validator ordering.

Overview
Introduces BaseTrainerConfig in trainer.py so RL TrainerConfig and SFTConfig share one definition of model/tokenizer/optim/scheduler, logging, profiling, distributed timeout, and the common cross-cutting validators (DeepEP vs grad clip, full optimizer offload, VLM+LoRA, trace limits, scheduler steps, tokenizer auto-setup).

SFTConfig now subclasses the base instead of re-declaring those fields and copy-pasted validators. RL- vs SFT-specific behavior for "auto" settings is expressed as subclass ClassVar defaults (auto_moe_router_dtype, auto_freeze_moe_router, auto_weight_decay) resolved in a single resolve_auto_defaults validator on the base.

RL-only and SFT-only pieces stay on the respective subclasses (e.g. RL loss / rollout_transport; SFT renderer, deployment, online eval wiring). Intended runtime behavior is unchanged; help text and field/validator ordering may differ slightly.

Reviewed by Cursor Bugbot for commit 1b459c1. 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:32
@samsja samsja closed this Oct 4, 2026
@faresobeid faresobeid reopened this Oct 8, 2026
@faresobeid
faresobeid force-pushed the refactor/trainer-config-base branch from 558057d to 806a781 Compare October 8, 2026 14:13
samsja added a commit that referenced this pull request Oct 10, 2026
Replaces the three switches `model.fsdp_cpu_offload`,
`model.optim_cpu_offload` and `model.full_offload` with one field:

```toml
[model]
offload = "optimizer"   # default; also "full", "none"
```

This applies to both the RL trainer and SFT. The `fsdp` switch is not
migrated: the mode is removed outright (rationale below). The
`[model.full_offload] numa_bind` knob is dropped: no recipe, deployment,
template or doc sets it. Full offload now always pins each rank's CPUs
to its GPU's NUMA node. That was already the default. Ranks whose GPU
reports no NUMA node still skip binding with a warning. On hosts without
GPU sysfs topology (`/sys/bus/pci/devices/<bus>/numa_node`), binding is
no longer optional: full offload fails there instead of being switched
off.

Why: the three switches could only be used one at a time, enforced by
`cpu_offload_mutual_exclusion`. Because `optim_cpu_offload` defaulted to
`true`, enabling full offload also meant writing `optim_cpu_offload =
false`, otherwise validation failed (see the old
`configs/advanced/deepseek-v4-flash/sft.toml` comment). With one value
the conflicting combinations cannot be expressed, so that validator is
deleted. The default behaviour does not change: optimizer-state offload
stays on.

Why drop `fsdp`: the mode was the stock torch FSDP2 `CPUOffloadPolicy`
with no in-tree config, example, or test user; Muon rejected it and it
broke NCCL weight broadcast. It was kept only for long-context runs
whose fp32 masters/moments no longer fit on GPU (the reason it was
restored in #3052); the `full` mode added in #3234 keeps FP32 masters,
moments, and gradients in host RAM, covering that memory pressure
instead. Gone with it: the `CPUOffloadPolicy` selection in `setup_fsdp`,
CPU-side model init in `setup_model`/`load_dcp_from_hf`,
`_move_buffers_to_cuda`, and the fsdp leg of the Gloo special case.

Changes:
- `ModelConfig.offload: Literal["none", "optimizer", "full"] =
"optimizer"` replaces the three fields.
`OptimizerInBackwardOffloadConfig` (and with it `numa_bind`) and its
bool-normalizing `BeforeValidator` are gone.
- Validators in `TrainerConfig` and `SFTConfig`:
- full offload accepts only AdamW or SignSGD and disables clipping, now
reading `offload == "full"`;
- the `Muon optimizer does not support FSDP CPU offload` validators are
deleted with the mode.
- Runtime:
- `setup_optimizer(..., offload=...)` replaces `cpu_offload` +
`full_offload_config`, and its "both enabled" check is gone.
- `FullCPUOffloadOptimizer` drops its unused `offload_config` argument.
- `setup_full_cpu_optimizer_offload()` takes no arguments and always
binds.
- `model.py`: FSDP always uses the default `OffloadPolicy()`; CPU-side
model init in `setup_model`/`load_dcp_from_hf` and
`_move_buffers_to_cuda` are gone.
- Tomls: `glm-4.5-air/swe-budget.toml` and
`ci/integration/reverse-text-sft/full-offload-resume.toml` → `offload =
"full"`; `glm-5.3/sft/h200/16k-12-node.toml` → `offload = "none"`;
`glm-5.3/sft/h200/base.toml` → `offload = "optimizer"`. Also updated
`docs/scaling.md`, the `deepseek-v4-flash/sft.toml` comment, and
`skills/training/start-run/SKILL.md`, which documented the removed
fields.

Breaking: the old keys now fail with `extra_forbidden`, and `offload =
"fsdp"` fails the `Literal`. There is no shim.

Interaction with #3849 (one trainer-common config base): #3849 moves the
full-offload and Muon/FSDP validators from `TrainerConfig`/`SFTConfig`
onto `BaseTrainerConfig`. This PR edits the full-offload validator's
condition and deletes the Muon/FSDP one, so the conflict is textual
only: keep #3849's single copy of the full-offload validators with
`self.model.offload == "full"` and drop its Muon/FSDP copy. Whichever
lands second takes a small rebase.

Other open PRs that set or read the old keys need a rebase:
- tomls: #3883, #3879, #3766, #3765, #3729, #3629, #3627, #3616, #3544
- code: #3885 (`optim_cpu_offload`), #3739 (`full_offload` validators),
#3724, #3719, #3699, #3610, #3487 (`fsdp_cpu_offload` / `enable_gloo`)

Test plan
- `pytest tests/unit/orchestrator tests/unit/test_configs.py
tests/unit/train/test_cpu_adam.py tests/unit/train/test_state_offload.py
-m "not gpu"`: 336 passed, 1 skipped (Qwen3-VL snapshot not cached on
this host), 4 deselected (GPU; this host has no GPU). The existing
offload config tests are updated to the new field. `test_load_configs`
covers the 4 edited tomls.
- `cli(TrainerConfig|SFTConfig, ...)`: `--model.offload full|none` and
the default (`optimizer`) resolve as expected; `optim_cpu_offload` and
`offload = "fsdp"` are rejected.
- `import prime_rl.trainer.rl.train, prime_rl.trainer.sft.train` works.
- `ruff check` and `ruff format --check` on the changed Python files:
clean.
- Not run: GPU or multi-node training with `offload = "full"`.

<!-- CURSOR_SUMMARY -->
---

> [!NOTE]
> **Medium Risk**
> Breaking TOML/API surface touches distributed training memory paths
(FSDP offload, optimizer-in-backward, Gloo); behavior should match prior
modes when mapped correctly, but mis-migrated configs will fail at parse
time.
> 
> **Overview**
> **Breaking config change:** `model.fsdp_cpu_offload`,
`model.optim_cpu_offload`, and `model.full_offload` are replaced by a
single **`model.offload`** setting (`"optimizer"` default, or `"full"`,
`"fsdp"`, `"none"`). RL and SFT share the same field; example TOMLs and
scaling docs are updated accordingly.
> 
> The mutual-exclusion validator between the old flags is removed
because only one mode can be expressed. **`[model.full_offload]` /
`numa_bind` is dropped** — `"full"` always runs NUMA binding and CPU
optimizer thread setup via parameterless
`setup_full_cpu_optimizer_offload()`. Runtime paths (`setup_optimizer`,
FSDP CPU offload in `model.py`, Gloo when offload is `"full"` or
`"fsdp"`) key off the new enum; `FullCPUOffloadOptimizer` no longer
takes an offload config object.
> 
> Default training behavior is unchanged: optimizer-state CPU offload
remains the default (`"optimizer"`). Old TOML keys are not aliased and
should fail validation.
> 
> <sup>Reviewed by [Cursor Bugbot](https://cursor.com/bugbot) for commit
35da73a. Bugbot is set up for automated
code reviews on this repo. Configure
[here](https://www.cursor.com/dashboard/bugbot).</sup>
<!-- /CURSOR_SUMMARY -->

---------

Co-authored-by: sami jaghouar <sami.jaghouar@gmail.com>
@faresobeid
faresobeid force-pushed the refactor/trainer-config-base branch from 806a781 to 04d7143 Compare October 11, 2026 12:59
…nd SFTConfig

BaseTrainerConfig holds the fields and validators shared by the RL trainer
and SFT. The three 'auto' defaults that differ (moe_router_dtype,
freeze_moe_router, weight_decay) are class variables resolved by one
validator.
@faresobeid
faresobeid force-pushed the refactor/trainer-config-base branch from 04d7143 to 6061eba Compare October 12, 2026 18:35
…ig-base

# Conflicts:
#	packages/prime-rl-configs/src/prime_rl/configs/sft.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.

lgtm, i thought this was already the case tho

@snimu something to adapt into the pretraining entrypoint

@samsja
samsja merged commit 1372ba0 into main Oct 19, 2026
19 of 20 checks passed
@samsja
samsja deleted the refactor/trainer-config-base branch October 19, 2026 07:35
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