Skip to content

feat(configs): launcher-owned env-server address overrides - #3216

Closed
eexwhyzee wants to merge 2 commits into
mainfrom
fix/env-server-address-overrides
Closed

feat(configs): launcher-owned env-server address overrides#3216
eexwhyzee wants to merge 2 commits into
mainfrom
fix/env-server-address-overrides

Conversation

@eexwhyzee

@eexwhyzee eexwhyzee commented Aug 7, 2026

Copy link
Copy Markdown
Collaborator

#3162 made env-server addresses fully derived (tcp://127.0.0.1:<base + index>) on the grounds that addresses are wiring, not intent - correct, but it bakes in the assumption that the launcher and the orchestrator always share a host. Kubernetes launchers wire across pods: the prime-rl-fft chart currently has no choice but to spawn every env server inside the orchestrator pod, because the orchestrator will only ever dial loopback. That forecloses per-source pods (isolated resources, per-env log streams, independent restarts) entirely.

Add OrchestratorConfig.env_server_addresses: a launcher-owned map keyed '/<resolved_name>'. A listed source is externally managed - the local/sbatch launchers neither write its env-server TOML nor spawn a server for it, and the orchestrator connects to the given address; the env-server side binds wherever that launcher told it to. Unlisted sources keep the derived loopback address, with indices still counted across ALL sources so overriding one source never shifts another's port. Sources themselves stay deployment-agnostic: this is the launcher recording where it chose to run each server, not user intent - the same framing #3162 established, extended to launchers that cannot use loopback.

Default-empty means zero behavior change for every existing deployment: rl.py and the sbatch templates render identically when the field is unset.

A model validator rejects override keys matching no source (a typo would otherwise silently fall back to loopback and the run would hang polling a server nobody runs), skipped when no sources are present so external render paths that validate with source sections stripped (e.g. the k8s validator service) still pass.


Note

Medium Risk
Changes orchestrator connectivity and launcher env-server lifecycle; mis-keyed overrides are now rejected, but wrong addresses could still cause connection failures at runtime.

Overview
Adds OrchestratorConfig.env_server_addresses, a launcher-owned map keyed by <split>/<resolved_name> so the orchestrator can connect to env servers that are not on loopback (e.g. separate Kubernetes pods). env_addresses now prefers an override when present; port indices still follow the full train-then-eval source list so overriding one source does not change others’ derived ports.

The RL launcher (env_servers() and SLURM template wiring) skips overridden sources: it does not write env-server TOML or spawn a local server for them. Unset overrides keep today’s loopback behavior.

A validator rejects override keys that do not match any train/eval source (avoids silent typos and hangs); validation is skipped when there are no sources so stripped configs (e.g. k8s validator) still load.

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

#3162 made env-server addresses fully derived (tcp://127.0.0.1:<base +
index>) on the grounds that addresses are wiring, not intent - correct,
but it bakes in the assumption that the launcher and the orchestrator
always share a host. Kubernetes launchers wire across pods: the
prime-rl-fft chart currently has no choice but to spawn every env
server inside the orchestrator pod, because the orchestrator will only
ever dial loopback. That forecloses per-source pods (isolated
resources, per-env log streams, independent restarts) entirely.

Add OrchestratorConfig.env_server_addresses: a launcher-owned map keyed
'<split>/<resolved_name>'. A listed source is externally managed - the
local/sbatch launchers neither write its env-server TOML nor spawn a
server for it, and the orchestrator connects to the given address; the
env-server side binds wherever that launcher told it to. Unlisted
sources keep the derived loopback address, with indices still counted
across ALL sources so overriding one source never shifts another's
port. Sources themselves stay deployment-agnostic: this is the launcher
recording where it chose to run each server, not user intent - the
same framing #3162 established, extended to launchers that cannot use
loopback.

Default-empty means zero behavior change for every existing deployment:
rl.py and the sbatch templates render identically when the field is
unset.

A model validator rejects override keys matching no source (a typo
would otherwise silently fall back to loopback and the run would hang
polling a server nobody runs), skipped when no sources are present so
external render paths that validate with source sections stripped
(e.g. the k8s validator service) still pass.

Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
@eexwhyzee
eexwhyzee requested a review from mikasenghaas August 7, 2026 22:58

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

Want reviews to match your repository better? Bugbot Learning can learn team-specific rules from PR activity. A team admin can enable Learning in the Cursor dashboard.

Reviewed by Cursor Bugbot for commit 07ad9b6. Configure here.

f"env_server_addresses keys {unknown} match no train/eval source "
f"(known: {sorted(known)}); overrides are keyed '<split>/<resolved_name>'"
)
return self

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Bench mode rejects eval overrides

Medium Severity

auto_setup_bench clears eval before validate_env_server_addresses runs. With bench=True, any env_server_addresses keys for eval sources look unknown and raise, even though those overrides are simply unused after eval is disabled. That breaks the common --bench path whenever a launcher has injected eval addresses.

Additional Locations (1)
Fix in Cursor Fix in Web

Reviewed by Cursor Bugbot for commit 07ad9b6. Configure here.

Comment thread tests/unit/test_configs.py Outdated
"""First port of the env-server port range: the source at position ``i`` (train, then eval) is served at ``tcp://127.0.0.1:<base + i>``. Give concurrent runs on one host distinct bases (e.g. one per multi-run orchestrator)."""

env_server_addresses: dict[str, str] = Field(default_factory=dict)
"""Launcher-owned overrides of where each source's env server lives, keyed ``<split>/<resolved_name>`` (e.g. ``train/wordle``). A listed source is externally managed: the launchers neither write its env-server TOML nor spawn a server for it, and the orchestrator connects to the given address instead of the derived loopback one. Unlisted sources keep the derived ``tcp://127.0.0.1:<env_server_base_port + index>`` address (indices stay positional across ALL sources, so overriding one source never shifts another's port). Sources themselves stay deployment-agnostic — this block is the launcher recording where it chose to run each server, not user intent: launchers whose orchestrator and env servers cannot share a host (e.g. the k8s chart running env servers in separate pods) inject their addresses here."""

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.

does the EnvConfig on orch not have a address field which we can set and avoid the autosetup? i might be wrong, but if we do have it, would be more elegant

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

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

so that was removed in #3162
Screenshot 2026-08-07 at 4 12 12 PM

do we want to add it back in? from this line in the PR summary, it looks like there was intent behind it:

Addresses are derived, not configured per source (matching how transport wiring works): OrchestratorConfig.env_addresses maps each (split, name) source to tcp://127.0.0.1:<base + i> in config order (train → eval). There is no address field on the source's serve block — the launcher and the orchestrator independently derive the same answer from the config alone.

Co-Authored-By: Claude Fable 5 <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.

2 participants