Skip to content

fix(configs): reject launcher-owned fields instead of silently overwriting them - #3977

Open
faresobeid wants to merge 9 commits into
mainfrom
fix/reject-launcher-owned-fields
Open

faresobeid wants to merge 9 commits into
mainfrom
fix/reject-launcher-owned-fields

Conversation

@faresobeid

@faresobeid faresobeid commented Oct 10, 2026 •

Copy link
Copy Markdown
Contributor

Under rl, an explicit value for a launcher-derived field must now equal the derived value; a different value fails validation instead of being silently overwritten.

What

  • trainer.weight_broadcast / orchestrator.weight_broadcast: must match the shared [weight_broadcast], except host and inference_world_size, which the launcher fills per deployment.
  • orchestrator.pad_to_multiple_of: derived from trainer.model.cp.
  • orchestrator.num_train_workers: derived from the deployment when it is set by the launcher (more than 1 train GPU, or multi-node).
  • New helper set_derived in utils/validation.py.
  • validate_enough_devices_for_nccl now runs after the shared broadcast is copied in and reads the shared weight_broadcast, so [weight_broadcast] type = "nccl" on 1 GPU fails validation.

Equal values are allowed: the single-node SLURM launch re-parses the resolved rl.json, which contains every derived value (test_single_node_nccl_resolved_json_roundtrips). host is not compared because the single-node NIXL template overrides --weight-broadcast.host on that re-parse.

Not changed: dp_replicate from deployment.nodes_per_fsdp_group (#3992 deletes it); orchestrator.inference_metrics_roles (already filled only when unset). No shipped TOML under configs/ or examples/ sets any of these fields.

Validation

  • tests/unit/test_configs.py: parametrized conflict test, resolved-JSON roundtrip, test_rl_nccl_broadcast_needs_two_gpus (fails on main), test_load_configs over every shipped TOML.
  • Branch merged with main 1810d5b9a: pytest tests/unit/orchestrator tests/unit/test_configs.py tests/unit/utils tests/unit/eval -m "not gpu": 491 passed. ruff check, ruff format --check and uv lock --check are clean.

Overlaps


Note

Low Risk
Changes are config validation only at parse time; they prevent misconfiguration but do not alter runtime training or broadcast behavior when configs are valid.

Overview
RL config validation now treats launcher-derived fields as read-only: an explicit value is accepted only if it matches what the launcher would compute; otherwise validation fails with a clear error instead of silently replacing the user's setting.

This applies to orchestrator.pad_to_multiple_of, orchestrator.num_train_workers (when derived from deployment), and trainer / orchestrator.weight_broadcast (must align with shared [weight_broadcast], except host and inference_world_size, which stay launcher-filled). A new check_derived helper in validation.py implements the equality check so resolved rl.json round-trips still work.

NCCL GPU check runs after shared weight broadcast is copied onto sub-configs, so [weight_broadcast] type = "nccl" on a single-GPU layout fails validation even when set only at the top level.

Unit tests cover conflicting overrides, the NCCL two-GPU rule, and existing round-trip behavior.

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

@faresobeid
faresobeid marked this pull request as ready for review October 13, 2026 17:49
faresobeid added a commit that referenced this pull request Oct 28, 2026
`rl` and `sft` each defined their own deployment config with the same
`gpus_per_node` and the same single-node and multi-node fields, and
their defaults and checks had drifted. This PR gives them one base.

## Changes
- `shared.py` gets `BaseDeploymentConfig`,
`BaseSingleNodeDeploymentConfig` (`type`, `num_train_gpus`,
`num_infer_gpus`) and `BaseMultiNodeDeploymentConfig` (`type`, required
`num_train_nodes`). The `rl` and `sft` classes subclass them and keep
their own validators and extras: RL replicas and orchestrator placement;
SFT online-eval `num_infer_nodes`.
- SFT multi-node `num_train_nodes` is required, as for RL. A multi-node
SFT run without it no longer silently uses 2 nodes.
- `deployment.nodes_per_fsdp_group` is removed from `rl` and `sft`. It
only set `dp_replicate = num_train_nodes / nodes_per_fsdp_group` and
silently overwrote an explicit `dp_replicate`. Set
`trainer.model.dp_replicate` (`rl`) or `model.dp_replicate` (`sft`)
directly. No config in `configs/` or `examples/` uses it.
- SFT's `normalize_deployment` is removed. It silently dropped
`num_train_gpus` / `num_infer_gpus` under `type = "multi_node"`; they
are now rejected like any unknown field.
- The SFT `num_eval_gpus` / `num_eval_nodes` aliases are removed.

**Breaking:** `nodes_per_fsdp_group`, the `num_eval_*` aliases and
single-node fields under multi-node are rejected; SFT multi-node needs
`num_train_nodes`.

## Validation
- `pytest tests/unit/orchestrator tests/unit/test_configs.py
tests/unit/train/sft tests/unit/utils -m "not gpu"`: 538 passed.
`test_configs` validates every TOML in `configs/` and `examples/`.
- Checked directly: SFT `{"type": "multi_node"}` without
`num_train_nodes` is rejected; `num_train_gpus` under multi-node is
rejected; `nodes_per_fsdp_group` is rejected; a valid multi-node config
builds.
- `ruff check`, `ruff format --check`, `uv lock --check`: clean.

## Overlaps
#3977 (reject launcher-owned fields) does not guard `dp_replicate` and
merges cleanly with this PR.

<!-- CURSOR_SUMMARY -->
---

> [!NOTE]
> **Medium Risk**
> Breaking deployment TOML/CLI fields and stricter multi-node validation
can fail existing configs; FSDP layout must be set explicitly via model
`dp_replicate` instead of deployment.
> 
> **Overview**
> **Unifies `[deployment]` for RL and SFT** by moving shared single- and
multi-node fields into `shared.py` (`BaseDeploymentConfig`,
`BaseSingleNodeDeploymentConfig`, `BaseMultiNodeDeploymentConfig`). RL
and SFT only keep entrypoint-specific pieces (RL: infer replicas,
orchestrator placement; SFT: online-eval `num_infer_nodes`).
> 
> **Breaking config cleanup:** `deployment.nodes_per_fsdp_group` is
removed for both entrypoints—along with the logic that overwrote
`trainer.model.dp_replicate` / `model.dp_replicate`. Set `dp_replicate`
on the model block directly. SFT multi-node now **requires**
`num_train_nodes` (no silent default of 2). SFT drops `num_eval_gpus` /
`num_eval_nodes` aliases, removes `normalize_deployment` (single-node
GPU keys under multi-node are rejected like other unknown fields), and
uses the shared base for `num_infer_gpus` without the eval alias.
> 
> **Docs:** `docs/scaling.md` no longer documents `nodes_per_fsdp_group`
in the RL multi-node example.
> 
> <sup>Reviewed by [Cursor Bugbot](https://cursor.com/bugbot) for commit
93ce582. Bugbot is set up for automated
code reviews on this repo. Configure
[here](https://www.cursor.com/dashboard/bugbot).</sup>
<!-- /CURSOR_SUMMARY -->
…ed-fields

# Conflicts:
#	packages/prime-rl-configs/src/prime_rl/configs/rl.py

This branch has not been deployed

No deployments
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.

1 participant