Skip to content

chore(orchestrator): send all engine admin POSTs through _admin_post - #3839

Closed
faresobeid wants to merge 1 commit into
mainfrom
chore/admin-post
Closed

faresobeid wants to merge 1 commit into
mainfrom
chore/admin-post

Conversation

@faresobeid

@faresobeid faresobeid commented Oct 4, 2026 •

Copy link
Copy Markdown
Contributor

Summary

orchestrator/clients.py had three ways to POST to an engine. Now every admin POST goes through _admin_post.

What changed

  • AdminPlane.init_broadcaster(host, port, timeout, inference_world_size, session_id="default") replaces AdminPlane.initialize_nccl and the free function init_nixl_broadcast. NCCL and NIXL receivers both call it. DynamoAdminPlane overrides init_broadcaster instead of initialize_nccl and passes session_id through to its collective RPC (it was hard-coded to "default").
  • AdminPlane.load_lora_adapter(lora_name, lora_path) replaces the free function. It uses _admin_post(..., retry_not_found=True). _is_retryable_lora_error, the tenacity decorator and LORA_LOAD_READ_TIMEOUT_S/LORA_LOAD_TOTAL_TIMEOUT_S are gone. The 404 retry is kept on purpose: vLLM returns 404 (LoRAAdapterNotFoundError) while the adapter files are not yet visible over NFS.
  • _admin_post / _is_retryable_admin_error take retry_not_found: bool = False.
  • PrefillScorer (a lazy AsyncOpenAI holder) is folded into InferenceClient.score / aclose.
  • Tests: the 4 _is_retryable_lora_error tests become one parametrized _is_retryable_admin_error test. The LoRA and NCCL-init tests now target the new methods.

Behavior change

  • NCCL init no longer silently skips a 404 on /init_broadcaster. It now raises. It also now retries 5xx and transport errors, with a per-attempt read timeout of max(300s, timeout) (it had no timeout before). NIXL init already worked this way.
  • LoRA load: now also retries 502/503/other 5xx (before, only 500 and 404). Per-attempt read timeout goes from 30s to 60s. The total budget stays 120s.
  • NIXL init now logs the same info line as NCCL init. The NCCL payload now includes session_id: "default". The server already defaults to that value.
  • Dynamo + NIXL: init_broadcaster now goes through Dynamo's /collective_rpc override instead of a plain /init_broadcaster POST to the Dynamo worker. This combination is not exercised anywhere.

Checks run

  • pytest tests/unit/orchestrator tests/unit/test_configs* tests/unit/inference/test_dynamo.py tests/unit/transports -m "not gpu": 307 passed
  • ruff check ., ruff format --check .: clean
  • Differential check against origin/main clients.py, using mocked clients:
    • init_nixl_broadcast and init_broadcaster send identical POSTs (path, timeout, JSON) for (world size, servers) = (8,2), (16,4), (1,1).
    • The NCCL payloads match apart from session_id.
    • The LoRA JSON payloads match.
    • The retry predicates match for transport errors and 400/404/409/500. They differ only for LoRA on 502/503 (now retried).

GPU checks still needed

  • NCCL weight broadcast RL run (multi-server, e.g. rank_offset > 0)
  • NIXL weight broadcast run
  • LoRA RL run with filesystem broadcast (adapter hot-swap)
  • OPD/OPSD teacher prefill scoring (InferenceClient.score)
  • Dynamo NCCL example (examples/extra/dynamo)

Conflicts

None of #3652, #3811, #3802, #3803, #3807, #3820, #3819, #3836 touch these files.


Note

Medium Risk
Changes NCCL broadcast init from fail-open on 404 to fail-closed with retries, affecting multi-server weight broadcast; LoRA and init_broadcaster retry semantics also shifted.

Overview
Routes all inference engine admin POSTs through shared _admin_post, consolidating LoRA load, weight-broadcast setup, pause/resume, and related calls behind one retry path.

AdminPlane.init_broadcaster replaces initialize_nccl and the standalone init_nixl_broadcast; NCCL, NIXL, and Dynamo receivers call it, with optional session_id (Dynamo passes it into collective RPC instead of hard-coded "default"). AdminPlane.load_lora_adapter replaces the old module-level helper and uses _admin_post(..., retry_not_found=True) for NFS propagation 404s. PrefillScorer is folded into InferenceClient.score.

Behavior: NCCL /init_broadcaster no longer silently skips 404—it fails like NIXL—and gains bounded retries/timeouts on 5xx and transport errors. LoRA loads also retry other 5xx (not only 500) with a 60s per-attempt read timeout. Weight transports call the new AdminPlane methods directly.

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

One AdminPlane.init_broadcaster for NCCL and NIXL, AdminPlane.load_lora_adapter
through the shared retry helper (opt-in 404 retry), and fold PrefillScorer into
InferenceClient.
@faresobeid
faresobeid marked this pull request as ready for review October 4, 2026 12:28
@samsja samsja closed this Oct 4, 2026
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