fix(vllm): unify engine control-plane HTTP timeout as --vllm-engine-request-timeout-secs (use for /sleep+/wake_up) - #89
Conversation
There was a problem hiding this comment.
Code Review
This pull request replaces the hardcoded 30-second HTTP timeout with a configurable _weight_transfer_http_timeout() for the /sleep and /wake_up endpoints to prevent premature timeouts on larger models. The reviewer suggests using a tuple for the timeout parameter (e.g., (5.0, self._weight_transfer_http_timeout())) to keep a short connection timeout, preventing the Ray actor from hanging if the server is unreachable.
| response = requests.post( | ||
| f"{self._http_base()}/sleep", | ||
| params={"level": level}, | ||
| timeout=30, | ||
| timeout=self._weight_transfer_http_timeout(), | ||
| ) |
There was a problem hiding this comment.
Using a single float for timeout sets both the connection timeout and the read timeout to that value (which defaults to 900 seconds). If the vLLM server is completely dead or unreachable, the connection attempt will hang for 15 minutes, blocking the Ray actor. Specifying a tuple like (5.0, self._weight_transfer_http_timeout()) keeps a short, sensible connection timeout while still allowing the long-running sleep operation to complete.
| response = requests.post( | |
| f"{self._http_base()}/sleep", | |
| params={"level": level}, | |
| timeout=30, | |
| timeout=self._weight_transfer_http_timeout(), | |
| ) | |
| response = requests.post( | |
| f"{self._http_base()}/sleep", | |
| params={"level": level}, | |
| timeout=(5.0, self._weight_transfer_http_timeout()), | |
| ) |
| response = requests.post( | ||
| f"{self._http_base()}/wake_up", | ||
| params=wake_params, | ||
| timeout=30, | ||
| timeout=self._weight_transfer_http_timeout(), | ||
| ) |
There was a problem hiding this comment.
Using a single float for timeout sets both the connection timeout and the read timeout to that value (which defaults to 900 seconds). If the vLLM server is completely dead or unreachable, the connection attempt will hang for 15 minutes, blocking the Ray actor. Specifying a tuple like (5.0, self._weight_transfer_http_timeout()) keeps a short, sensible connection timeout while still allowing the long-running wake_up operation to complete.
| response = requests.post( | |
| f"{self._http_base()}/wake_up", | |
| params=wake_params, | |
| timeout=30, | |
| timeout=self._weight_transfer_http_timeout(), | |
| ) | |
| response = requests.post( | |
| f"{self._http_base()}/wake_up", | |
| params=wake_params, | |
| timeout=(5.0, self._weight_transfer_http_timeout()), | |
| ) |
…-engine-request-timeout-secs The /sleep and /wake_up control-plane calls hardcoded timeout=30, which a large engine exceeds (a 30B FP8 dp=2 colocate engine ReadTimeout'd on the first /sleep, because under vLLM data parallelism the front API server with api_server_count=dp coordinates every replica's sleep). They now use the tunable timeout that already governs the other trainer->engine control-plane calls (init_weight_transfer_engine, update_weights, start/finish_weight_update). That timeout was named --vllm-weight-transfer-timeout-sec, which is misleading now that it also covers /sleep + /wake_up. Rename it to --vllm-engine-request-timeout-secs (pairs with the existing --router-request-timeout-secs: one is requests to the router, the other to the vLLM engine). The old --vllm-weight-transfer-timeout-sec is kept as a deprecated alias mapping to the same dest, so existing configs keep working. Helper renamed _weight_transfer_http_timeout -> _engine_request_http_timeout; orchestration dest and unit tests updated (incl. an alias test). Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
9921165 to
59597b9
Compare
What
Two coupled changes to the trainer->vLLM-engine control-plane HTTP timeout:
/sleep+/wake_upno longer hardcodetimeout=30. A large engine exceeds 30s — a Qwen3-30B-A3B-FP8dp=2colocate engine ReadTimeout'd on the first/sleep, because under vLLM data parallelism the front API server (api_server_count=dp) coordinates every DP replica's sleep. They now use the same tunable timeout that already governs the other trainer->engine control-plane calls (init_weight_transfer_engine,update_weights,start/finish_weight_update).Rename that timeout flag
--vllm-weight-transfer-timeout-sec->--vllm-engine-request-timeout-secs(it now covers more than weight transfer). Pairs with the existing--router-request-timeout-secs— one is requests to the router, the other to the vLLM engine. The old--vllm-weight-transfer-timeout-secis kept as a deprecated alias mapping to the same dest (vllm_engine_request_timeout_secs), so existing configs keep working.Helper renamed
_weight_transfer_http_timeout->_engine_request_http_timeout; orchestration dest + unit tests updated (incl. a test that the deprecated alias still maps to the new dest).Why this name
Surveyed the reference RL frameworks: none use a
control_planeterm; the convention isrequest_timeout(AReaL) /*-request-timeout-secs(this repo's own--router-request-timeout-secs).--vllm-engine-request-timeout-secsmatches that and reads as the engine-side counterpart of the router-side timeout.Tests
tests/unit/backends/vllm_utils/— renamed/added timeout tests pass (incl. alias test). NOTE: 3 unrelatedtest_validate_args_*cases already fail on thefeature/multi_nodesbase (PR #85 changedvalidate_argsbut didn't update those stale tests); not touched by this PR.AI assistance (Claude Code) was used for this change.