Skip to content

fix(orchestrator): make admin-plane timeouts env-overridable - #3597

Closed
JannikSt wants to merge 2 commits into
mainfrom
improvement/env-admin-plane-timeouts-rebase
Closed

JannikSt wants to merge 2 commits into
mainfrom
improvement/env-admin-plane-timeouts-rebase

Conversation

@JannikSt

@JannikSt JannikSt commented Sep 18, 2026 •

Copy link
Copy Markdown
Member

What dies today

Large-model hosted runs (GLM-5.2 790B) die at the first weight sync. The admin-plane POST timeouts are hardcoded module constants (src/prime_rl/orchestrator/clients.py):

  • UPDATE_WEIGHTS_TIMEOUT_S = 720 per-attempt read timeout for /update_weights
  • ADMIN_TIMEOUT_S = 300 per-attempt read timeout for /pause // /resume
  • _admin_post caps total wall-clock at stop_after_delay(2 * timeout_s), so /update_weights gets a ~24-min budget (and pause/resume ~10 min each) — ~34 min worst case per sync.

A 790B weight apply takes 72+ min observed. The 720 s read timeout fires 12 min in, tenacity re-POSTs /update_weights to an engine still mid-apply, and the 1440 s budget aborts ~24 min in. The timeout exception propagates up through WeightReceiver.receive → Watcher.apply_policy_update → the watcher background task dies → the orchestrator (and the run) exits at the first weight sync.

There is no config surface for these timeouts, and the orchestrator entrypoint did not apply OrchestratorConfig.env_vars in-process, so platform-hosted runs had no escape hatch.

What this fix does

Commit 1 — env overrides:

  • Adds env_timeout(name, default) in orchestrator/clients.py: reads the timeout from the environment at call time (not import time — entrypoints apply config env_vars after this module is imported). Invalid values raise instead of silently falling back.
  • Makes the admin-plane timeouts env-overridable:
    • PRL_ADMIN_TIMEOUT_S — /pause, /resume, /init_broadcaster (default: ADMIN_TIMEOUT_S = 300 s)
    • PRL_UPDATE_WEIGHTS_TIMEOUT_S — /update_weights (default: UPDATE_WEIGHTS_TIMEOUT_S = 720 s)
    • PRL_STARTUP_BROADCAST_TIMEOUT_S — online-eval startup broadcast rendezvous (eval/online.py, default: STARTUP_BROADCAST_TIMEOUT_S = 1200 s)
  • Applies os.environ.update(config.env_vars) in entrypoints/orchestrator.py, matching what entrypoints/inference.py already does, so a standalone orchestrator @ config.toml (e.g. a platform-hosted run) honors [env_vars].

Commit 2 — adversarial-review follow-ups:

  • DynamoAdminPlane.update_weights(transport="nccl") had its own hardcoded /pause / /update_weights / /resume timeouts that bypassed the overrides (the filesystem/NIXL paths go through super() and honor them). The Dynamo NCCL path now resolves PRL_ADMIN_TIMEOUT_S / PRL_UPDATE_WEIGHTS_TIMEOUT_S at call time like every other transport.
  • env_timeout rejects non-finite values (nan, inf, overflow such as 1e309) alongside negatives — a NaN or infinite deadline would silently invalidate the bounded read timeout and the tenacity stop_after_delay budget.
  • The orchestrator module entrypoint (python -m prime_rl.orchestrator.orchestrator @ config.toml, documented as a standalone alternative to the console script) now also applies config.env_vars in-process.
  • Tests use monkeypatch for env state (no dependence on inherited runner env) and cover the non-finite inputs.

Defaults unchanged

When the env vars are unset, behavior is identical to current main — every override falls back to the existing constant.

Recommended values for the GLM-5.2 (790B) run

[env_vars]
PRL_UPDATE_WEIGHTS_TIMEOUT_S = "7200"   # 2h per attempt; total budget 2x = 4h (790B apply = 72+ min observed)
PRL_ADMIN_TIMEOUT_S = "1800"             # pause/resume headroom for a busy pool
PRL_STARTUP_BROADCAST_TIMEOUT_S = "7200" # only if [orchestrator.eval] online evals are enabled

Test plan

  • uv run pytest tests/unit/orchestrator/test_clients.py → 17 passed (incl. 7 env_timeout tests).
  • uv run pytest tests/unit/eval tests/unit/inference/test_dynamo.py → 24 passed.
  • Pre-existing failures elsewhere in tests/unit/orchestrator (advantage/prefill_logprobs/qwen3_vl_e2e) and tests/unit/inference/test_serving_tokens.py are platform-only: torch/vllm are linux-only dependencies, unavailable on the macOS dev env; unrelated to this change.
  • Adversarial review (gpt-6-astra) found no blockers; the three SHOULD-FIX findings above are addressed by the second commit.

GLM-5.2 (790B) hosted runs die at the first weight sync: the admin-plane
per-attempt timeouts (300s pause/resume, 720s update_weights) are far
shorter than a 790B weight apply (72+ min observed), so /resume times
out and the orchestrator crashes mid-sync.

Read both timeouts from the environment at call time (not import time,
since entrypoints apply config env_vars after import):
- PRL_ADMIN_TIMEOUT_S (default 300.0) for /pause and /resume
- PRL_UPDATE_WEIGHTS_TIMEOUT_S (default 720.0) for /update_weights
- PRL_STARTUP_BROADCAST_TIMEOUT_S (default 1200) for the online eval
  startup broadcast rendezvous
Invalid values raise instead of silently defaulting.

Also apply OrchestratorConfig.env_vars in the orchestrator entrypoint,
mirroring entrypoints/inference.py: the rl/SLURM launchers inject them
into the subprocess, but a standalone orchestrator @ config.toml
(e.g. a platform-hosted run) has no launcher to do it. So a run TOML can
now raise the apply timeout with no platform change:

  [env_vars]
  PRL_UPDATE_WEIGHTS_TIMEOUT_S = "7200"
Review follow-ups on the env-overridable admin-plane timeouts:
- DynamoAdminPlane.update_weights(transport=nccl) now honors
  PRL_ADMIN_TIMEOUT_S / PRL_UPDATE_WEIGHTS_TIMEOUT_S like the other
  transports, instead of hardcoded ADMIN_TIMEOUT_S/UPDATE_WEIGHTS_TIMEOUT_S.
- env_timeout rejects non-finite values (nan, inf, overflow) alongside
  negatives; NaN/unbounded deadlines silently invalidated the read timeout.
- orchestrator module entrypoint (python -m prime_rl.orchestrator.orchestrator)
  applies config env_vars in-process, like the console entrypoint.
- tests use monkeypatch for env state and cover non-finite values.
@JannikSt

Copy link
Copy Markdown
Member Author

closing — reopening with corrected commit authorship

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