[F] simplify flush_cache: drop retry loop (vLLM /reset_prefix_cache always returns 200) - #137
Conversation
There was a problem hiding this comment.
Code Review
This pull request adds logging and a 1-second delay when the vLLM prefix cache reset fails (non-200 status code). The review feedback suggests changing the log level from info to warning, truncating the response text to prevent log flooding, and refactoring the retry logic to avoid infinite retries on non-retriable 4xx client errors.
| response = requests.post(f"{self._http_base()}/reset_prefix_cache", params=params, timeout=60) | ||
| if response.status_code == 200: | ||
| return | ||
| logger.info("Error resetting vLLM prefix cache: HTTP %s %r", response.status_code, response.text) |
There was a problem hiding this comment.
Log Level and Potential Log Flooding
- Log Level: The message is logged as
logger.info, but it represents an error condition ("Error resetting vLLM prefix cache"). It should be logged aslogger.warningorlogger.errorto make it easier to monitor and filter in production logs. - Log Flooding / Truncation: Logging the entire
response.textwithout truncation can lead to severe log flooding or high memory usage if the server returns a very large HTML/JSON error response (e.g., from Nginx, a proxy, or a core dump). It is safer to truncateresponse.textto a reasonable limit (e.g., 1000 characters).
| logger.info("Error resetting vLLM prefix cache: HTTP %s %r", response.status_code, response.text) | |
| logger.warning("Error resetting vLLM prefix cache: HTTP %s %r", response.status_code, response.text[:1000]) |
| logger.info("Error resetting vLLM prefix cache: HTTP %s %r", response.status_code, response.text) | ||
| time.sleep(1) |
There was a problem hiding this comment.
Infinite Retry on Non-Retriable 4xx Client Errors
If the server returns a 4xx client error (such as 404 Not Found if the endpoint is unsupported/disabled, or 400 Bad Request), retrying 60 times is futile and unnecessarily blocks the execution for up to 60 seconds.
However, because of the generic except Exception as e: block on line 780, any standard exception raised inside the try block (like response.raise_for_status()) will be caught and retried anyway.
To improve robustness and fail fast, consider refactoring the exception handling to distinguish between retriable and non-retriable errors, or specifically check for 4xx status codes and propagate the error immediately without retrying.
519885f to
44e2ff2
Compare
…ng debt) (#140) These are the only two files in the repo that fail the pre-commit gate (ruff/autoflake/isort/black) on origin/main — confirmed repo-wide: - ruff: only vime/ray/rollout.py:8 (F401 unused `argparse.Namespace`) - autoflake:only vime/ray/rollout.py - black: vime/ray/rollout.py + vime/backends/vllm_utils/vllm_engine.py Apply the exact hook auto-fixes (ruff --fix, autoflake --remove-all-unused-imports, isort --profile=black, black -l119), no functional change: - rollout.py: drop unused `from argparse import Namespace` (zero refs); isort blank-line separation between third-party and first-party; order two deferred function-local imports (vllm_router before vime.*). - vllm_engine.py: two blank lines before `class VLLMEngine` (E302/black). Decoupled from the slime-sync PRs (#137/#138) on purpose: those two PRs each touch one of these files, and the auto-fixing gate would reformat them on any PR that does. Landing the debt once here keeps the sync PRs scoped to behavior. Verified in vime-vllm cpu image: all four hooks pass on both files afterward; py_compile clean; `Namespace` has no remaining references. Signed-off-by: aoshen02 <aoshen@inferact.ai> Co-authored-by: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
| response = requests.post(f"{self._http_base()}/reset_prefix_cache", params=params, timeout=60) | ||
| if response.status_code == 200: | ||
| return | ||
| logger.info("Error resetting vLLM prefix cache: HTTP %s %r", response.status_code, response.text) |
There was a problem hiding this comment.
logger.info("Error -> logger.warning is better?
d4e42e8 to
140f594
Compare
|
Dropping per maintainer call. #137's only change was logging a non-200 from |
140f594 to
651b946
Compare
…lways 200s) vLLM's /reset_prefix_cache always returns 200 and gives no busy signal, so the 60x retry loop, non-200 logging, and TimeoutError can never fire. Collapse to a single best-effort POST + raise_for_status(). Intentional divergence from slime #1953, whose loop relies on sglang's 400-while-busy as a drain barrier — that signal does not exist on vLLM. Matches SkyRL's single-call practice (same vLLM keep-mode flow). Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
651b946 to
d14bb64
Compare
…VER-translation) Restore slime-verbatim neutral log wording where vime had over-translated by inserting "vLLM" into strings slime kept engine-agnostic: - "Shutdown vLLM engine" -> "Shutdown engine" - "Simulating crash on vLLM engine" -> "Simulating crash on engine" - "vLLM rollout: resuming workers..." -> "rollout: resuming workers..." - "Failed to resume vLLM worker" -> "Failed to resume worker" - "vLLM rollout abort (pause) for workers" -> "Abort request for %s" - "Failed to pause/abort worker" -> "Failed to abort worker at %s" flush_cache log strings dropped from this PR: #137 removes that retry loop entirely (vLLM /reset_prefix_cache always 200s), so there is nothing left to re-word there. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
…VER-translation) (#153) Restore slime-verbatim neutral log wording where vime had over-translated by inserting "vLLM" into strings slime kept engine-agnostic: - "Shutdown vLLM engine" -> "Shutdown engine" - "Simulating crash on vLLM engine" -> "Simulating crash on engine" - "vLLM rollout: resuming workers..." -> "rollout: resuming workers..." - "Failed to resume vLLM worker" -> "Failed to resume worker" - "vLLM rollout abort (pause) for workers" -> "Abort request for %s" - "Failed to pause/abort worker" -> "Failed to abort worker at %s" flush_cache log strings dropped from this PR: #137 removes that retry loop entirely (vLLM /reset_prefix_cache always 200s), so there is nothing left to re-word there. Co-authored-by: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Summary
Originally this PR ported slime #1953 (log non-200 on
flush_cache+ back off). On review that change is inert on vLLM: vime'sflush_cacheposts to vLLM's/reset_prefix_cache, whose handler always returnsResponse(status_code=200)(it ignores the internal success/False bool) and gives no busy signal. So the 60× retry loop, the non-200 log, and theTimeoutErrorcan never fire.This PR instead drops the loop entirely and collapses
flush_cacheto a single best-effort POST +raise_for_status().Why this diverges from slime #1953 (intentional, maintainer-approved)
slime's loop is a real drain barrier: sglang
/flush_cachereturns 400 while the engine has pending requests, so slime retries until the engine drains. vLLM has no equivalent —/reset_prefix_cache200s unconditionally, and vime's pause usesmode="keep"(it never uses flush as a drain barrier; colocate drains via/sleep). SkyRL (same vLLM keep-mode flow) likewise does a singlereset_prefix_cachecall with no loop.Diff
flush_cache: 22-line retry loop → singlePOST /reset_prefix_cache?reset_running_requests=false&reset_external=false+raise_for_status().py_compile+ruffclean;timeimport still used elsewhere.