-
Notifications
You must be signed in to change notification settings - Fork 420
feat(configs): launcher-owned env-server address overrides #3216
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -537,6 +537,9 @@ class OrchestratorConfig(BaseConfig): | |
| env_server_base_port: int = Field(5000, ge=1, le=65535) | ||
| """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.""" | ||
|
|
||
| batch_size: int | None = Field(None, ge=1) | ||
| """Samples to train on per step (rollout-based batching). Set this OR ``token_batch_size``.""" | ||
|
|
||
|
|
@@ -746,11 +749,32 @@ def env_sources(self) -> list[tuple[str, EnvConfig]]: | |
| @property | ||
| def env_addresses(self) -> dict[tuple[str, str], str]: | ||
| """Where each source's env server lives, keyed by ``(split, resolved_name)``: | ||
| an ``env_server_addresses`` override when the launcher set one, else | ||
| ``tcp://127.0.0.1:<port>`` with ports from ``env_server_base_port`` in | ||
| ``env_sources`` order. The launcher binds env servers at exactly these addresses | ||
| and the orchestrator connects to them, so both sides agree from the config | ||
| alone.""" | ||
| return { | ||
| (split, source.resolved_name): f"tcp://127.0.0.1:{self.env_server_base_port + index}" | ||
| (split, source.resolved_name): self.env_server_addresses.get(f"{split}/{source.resolved_name}") | ||
| or f"tcp://127.0.0.1:{self.env_server_base_port + index}" | ||
| for index, (split, source) in enumerate(self.env_sources) | ||
| } | ||
|
|
||
| @model_validator(mode="after") | ||
| def validate_env_server_addresses(self): | ||
| """Reject override keys that match no source — a typo would otherwise silently | ||
| fall back to the derived loopback address and the run would hang polling a | ||
| server nobody runs. Skipped when no sources are present: external render | ||
| paths (e.g. rl-k8s's validator) validate with the source sections stripped.""" | ||
| if not self.env_server_addresses: | ||
| return self | ||
| known = {f"{split}/{source.resolved_name}" for split, source in self.env_sources} | ||
| if not known: | ||
| return self | ||
| unknown = sorted(set(self.env_server_addresses) - known) | ||
| if unknown: | ||
| raise ValueError( | ||
| f"env_server_addresses keys {unknown} match no train/eval source " | ||
| f"(known: {sorted(known)}); overrides are keyed '<split>/<resolved_name>'" | ||
| ) | ||
| return self | ||
|
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Bench mode rejects eval overridesMedium Severity
Additional Locations (1)Reviewed by Cursor Bugbot for commit 07ad9b6. Configure here. |
||


There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
does the
EnvConfigon 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 elegantThere was a problem hiding this comment.
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

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