Repository navigation
Conversation
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request introduces contract tests and control-plane helpers to verify that rollout engines have successfully gone idle after aborting in-flight requests, integrating this check into the rollout abort process. The review feedback focuses on enhancing the robustness of these new asynchronous operations. Specifically, it is recommended to catch general exceptions instead of only httpx.HTTPError in abort_inflight_requests and query_engine_inflight to prevent unexpected failures from crashing the abort flow. Additionally, wrapping the observational verify_server_drain call in a try...except block ensures that any verification failures do not disrupt the critical rollout cleanup path.
| except httpx.HTTPError as e: | ||
| logger.warning(f"Failed to abort requests on {url}: {e}") | ||
| return url, str(e) |
There was a problem hiding this comment.
The previous implementation caught Exception to ensure that any failure during the best-effort abort sweep is logged and does not propagate. Restricting the exception handling to httpx.HTTPError introduces a risk where other unexpected exceptions (e.g., connection/network errors from other libraries, or internal errors in post) could propagate and crash the abort process. It is safer to catch Exception to maintain the robust, best-effort contract of this function.
| except httpx.HTTPError as e: | |
| logger.warning(f"Failed to abort requests on {url}: {e}") | |
| return url, str(e) | |
| except Exception as e: | |
| logger.warning(f"Failed to abort requests on {url}: {e}") | |
| return url, str(e) |
| except httpx.TimeoutException: | ||
| return EngineDrainState(url, None, f"timeout after {timeout_s:.3f}s") | ||
| except httpx.HTTPError as e: | ||
| return EngineDrainState(url, None, f"http error: {e}") | ||
| except ValueError as e: | ||
| return EngineDrainState(url, None, f"invalid json: {e}") |
There was a problem hiding this comment.
To ensure defensive programming and prevent any unexpected exceptions (such as network-related exceptions from underlying libraries, SSL errors, or unexpected response parsing issues) from propagating and crashing the rollout abort path, consider adding a fallback except Exception block to query_engine_inflight.
| except httpx.TimeoutException: | |
| return EngineDrainState(url, None, f"timeout after {timeout_s:.3f}s") | |
| except httpx.HTTPError as e: | |
| return EngineDrainState(url, None, f"http error: {e}") | |
| except ValueError as e: | |
| return EngineDrainState(url, None, f"invalid json: {e}") | |
| except httpx.TimeoutException: | |
| return EngineDrainState(url, None, f"timeout after {timeout_s:.3f}s") | |
| except httpx.HTTPError as e: | |
| return EngineDrainState(url, None, f"http error: {e}") | |
| except ValueError as e: | |
| return EngineDrainState(url, None, f"invalid json: {e}") | |
| except Exception as e: | |
| return EngineDrainState(url, None, f"unexpected error: {e}") |
| report = await verify_server_drain(urls, deadline_s=_ABORT_DRAIN_VERIFY_DEADLINE_S) | ||
| if not report.drained: | ||
| logger.warning(f"Rollout engines not confirmed idle after abort: {report.describe()}") |
There was a problem hiding this comment.
Since the server drain verification is purely observational and diagnostic, any unexpected failure or exception raised during verify_server_drain should not crash the critical rollout abort/cleanup path. Wrapping this call in a try...except block ensures that the rollout can still complete its cleanup and return the aborted samples even if the drain check fails.
| report = await verify_server_drain(urls, deadline_s=_ABORT_DRAIN_VERIFY_DEADLINE_S) | |
| if not report.drained: | |
| logger.warning(f"Rollout engines not confirmed idle after abort: {report.describe()}") | |
| try: | |
| report = await verify_server_drain(urls, deadline_s=_ABORT_DRAIN_VERIFY_DEADLINE_S) | |
| if not report.drained: | |
| logger.warning(f"Rollout engines not confirmed idle after abort: {report.describe()}") | |
| except Exception as e: | |
| logger.warning(f"Failed to verify server drain: {e}") |
2e67ccf to
55dbefe
Compare
55dbefe to
cbe5733
Compare
cbe5733 to
880802d
Compare
abort_inflight_requests() is best-effort by design: it posts /abort_requests
with max_retries=3 and swallows every failure, returning None. The abort path in
vllm_rollout.py therefore has no way to tell 'the engines stopped' from 'the
sweep never reached them', and the late-request resweep loop is bounded by the
local pending set: once local tasks drain, a request that was queued server-side
after the last sweep is no longer looked for.
Nothing observed that gap. The inflight queue diagnostics already expose real
per-queue counts through GET /load?include_inflight=true, but they were not
consulted anywhere in the abort path, so 'local pending empty' was the only
signal and it was treated as completion.
Add the read side of that contract:
* EngineDrainState / DrainReport describe what each engine actually reported,
with num_requests=None meaning UNKNOWN rather than idle.
* verify_server_drain() probes every engine in parallel inside one monotonic
deadline, so a slow engine consumes the shared budget instead of extending it,
and reports the engines that never answered.
* The endpoint answers one entry per data-parallel rank and the counts sit in
that rank's queue list:
{"inflight": [{"data_parallel_rank": 0,
"queues": [{"name": "running", "num_requests": 3, ...}]}]}
so the reader walks rank -> queues -> num_requests and totals across ranks.
Reading num_requests off the rank entry finds nothing and silently degrades
every probe to UNKNOWN.
* Only an explicit zero count counts as drained. Failed requests, malformed
bodies, a missing num_requests, a rank without queues and a truncated probe
are all UNKNOWN, and the requests sample list is never used to judge idleness
because the endpoint caps it by inflight_limit.
* abort_inflight_requests() now returns the per-URL failure reasons so callers
can distinguish 'aborted' from 'cannot tell'. Existing callers ignore the
return value, so their behaviour is unchanged.
abort() uses the check to report the engines it could not confirm idle. This is
deliberately observational: enforcing the condition at the weight-update boundary
changes control flow and needs the hardware reproduction first.
tests/test_server_idle_contract.py covers the failure directions that matter -
busy engine, truncated sample list with a non-zero count, 404/500, unparseable
body, missing/ill-typed num_requests, counts spread across data-parallel ranks,
a rank entry with no queues, unreachable engine, a deadline overrun with a hung
engine, parallel probing, and the abort failure map - using a controlled
transport and asyncio events rather than sleeps. One case replays the idle body
captured verbatim from a live engine, so a schema change upstream fails here
instead of turning every probe into UNKNOWN on hardware.
Signed-off-by: 0z5a <0z5a@users.noreply.github.com>
Signed-off-by: 0z5a <dezhen.lu@student.uni-tuebingen.de>
880802d to
3854344
Compare
Signed-off-by: 0z5a <dezhen.lu@student.uni-tuebingen.de>
Signed-off-by: 0z5a <dezhen.lu@student.uni-tuebingen.de>
|
Superseded by #460, now merged into |
Incremental review: Files changed.
Base:
0z5a-branchat791f101e20db377b28e15f12a00863baaea2605f.Observe actual engine drain after request abortion. An abort response or empty local pending set does not establish server idleness: late requests and unreadable engine state are reported as busy or unknown.
Probe per-rank metrics, queue counts and
num_requestsin order, using deadline-bounded parallel requests and a bounded late-request resweep. The result is logged; this change does not enforce a new fail-closed weight-update policy.The model run oversampled 24 requests and exercised rollout, training and weight publication. Probe latency is added diagnostic work, not a speedup. Added tests and their CI registration were removed from the diff.