fix(vllm): unify weight-transfer HTTP handling - #47
Conversation
There was a problem hiding this comment.
Code Review
This pull request refactors the vLLM engine backend by introducing a helper function _response_json_or_fallback to safely parse JSON responses and handle decoding failures or non-dictionary shapes consistently. It also consolidates the timeout retrieval logic into a helper method _weight_transfer_http_timeout and adds corresponding unit tests. The reviewer recommended making the environment variable parsing within this timeout helper more robust by gracefully handling potential ValueError exceptions and falling back to a default value.
| return float( | ||
| os.environ.get( | ||
| "SLIME_VLLM_WEIGHT_TRANSFER_UPDATE_TIMEOUT_SEC", | ||
| os.environ.get("SLIME_VLLM_WEIGHT_TRANSFER_HTTP_TIMEOUT_SEC", "900"), | ||
| ) | ||
| ) |
There was a problem hiding this comment.
Parsing environment variables directly with float() can raise a ValueError if the variable is set to an empty string or a malformed value (e.g., by a user configuration error). It is safer to use defensive programming to handle empty or invalid values gracefully and fall back to the default timeout of 900.0 seconds while logging a warning.
val = os.environ.get("SLIME_VLLM_WEIGHT_TRANSFER_UPDATE_TIMEOUT_SEC")
if not val:
val = os.environ.get("SLIME_VLLM_WEIGHT_TRANSFER_HTTP_TIMEOUT_SEC", "900")
try:
return float(val)
except ValueError:
logger.warning(
"Invalid timeout value %r for weight transfer. Falling back to default 900.0s.",
val,
)
return 900.0|
LGTM. Rebase to solve the conflicts |
6458b62 to
d6af552
Compare
2e56cf5 to
2141840
Compare
|
|
||
|
|
||
| def _response_json(response: requests.Response) -> dict: | ||
| """Return a JSON response using the same fail-loud behavior as SGLangEngine.""" |
There was a problem hiding this comment.
remove “as SGLangEngine.”
Route vLLM weight-transfer control-plane responses through a single slime-style JSON helper: raise HTTP errors with response.text attached, then parse the JSON body directly. This removes the previous fallback that converted JSON parse failures into {"ok": True, "raw": ...}.
Replace the weight-transfer timeout env knobs with the vime config flag --vllm-weight-transfer-timeout-sec (default 900s), and make update_weights, start/finish_weight_update, init_weight_transfer_engine, and init_weights_update_group all read the same helper.
Tests cover the new config flag, action-table exclusion, config-driven timeout use, and fail-loud JSON/HTTP error behavior.
Signed-off-by: aoshen02 <aoshen@inferact.ai>
2141840 to
84f35cd
Compare
|
LGTM |
Route vLLM weight-transfer control-plane responses through a single slime-style JSON helper: raise HTTP errors with response.text attached, then parse the JSON body directly. This removes the previous fallback that converted JSON parse failures into {"ok": True, "raw": ...}.
Replace the weight-transfer timeout env knobs with the vime config flag --vllm-weight-transfer-timeout-sec (default 900s), and make update_weights, start/finish_weight_update, init_weight_transfer_engine, and init_weights_update_group all read the same helper.
Tests cover the new config flag, action-table exclusion, config-driven timeout use, and fail-loud JSON/HTTP error behavior.
Signed-off-by: aoshen02 <aoshen@inferact.ai>
Follow-up to #22 and rebased over #49 / current
main. PR #49 merged the R3 routing-replay work and partially centralized the vLLM weight-transfer timeout path, but currentmainstill had two issues in the vLLM control plane:{"ok": True, "raw": ...}in several endpoints, which can hide bad/plain-text responses as success.init_weight_transfer_engine/init_weights_update_groupstill used a separate timeout path from the rest of the weight-transfer calls.Why this is not duplicating #49
#49 was a routing replay feature PR. It did touch
vllm_engine.py, and it leftstart_weight_update/finish_weight_updateusing a timeout helper, but it did not make response parsing fail loud and did not move all weight-transfer control-plane calls to a config-backed timeout. This PR is now scoped to those remaining control-plane fixes.Changes
_response_json(response)helper:raise_for_status()first.response.textto HTTP errors withException.add_note, matching the SGLang engine pattern.ok: True.--vllm-weight-transfer-timeout-sec(default900.0) as the vime config knob for vLLM weight-transfer HTTP control-plane calls.vllm serve.update_weights,start_weight_update,finish_weight_update,init_weight_transfer_engine, andinit_weights_update_groupthrough the same timeout helper.SLIME_VLLM_WEIGHT_TRANSFER_*_TIMEOUT_SECenv-var path from this code.Tests
AI assistance
AI assistance was used to inspect related PRs/reference implementations and update this PR. The human submitter should review every changed line and validate the behavior before merge.