Skip to content

fix(vllm): accept --disable-log-requests as no-op for vLLM 0.17+ compat#783

Open
lilyz-ai wants to merge 2 commits intomainfrom
lilyzhu/vllm-disable-log-requests-compat
Open

fix(vllm): accept --disable-log-requests as no-op for vLLM 0.17+ compat#783
lilyz-ai wants to merge 2 commits intomainfrom
lilyzhu/vllm-disable-log-requests-compat

Conversation

@lilyz-ai
Copy link
Collaborator

@lilyz-ai lilyz-ai commented Mar 19, 2026

Summary

  • --disable-log-requests was removed from vLLM's arg parser in v0.17+
  • model-engine hardcodes this flag when launching vllm_server.py, causing startup failures with unrecognized arguments: --disable-log-requests
  • Fix: check if the flag already exists before adding it, so this is a no-op when vLLM already handles it and a backward-compat shim when it doesn't

Test plan

  • Deployed qwen3-5-27b and qwen3-5-0-8b endpoints with vLLM 0.17.0-rc1 image — both confirmed 3/3 Running

🤖 Generated with Claude Code

Greptile Summary

This PR fixes a startup crash introduced when vLLM v0.17+ removed --disable-log-requests from its argument parser, while model-engine still unconditionally passes the flag (e.g. from llm_model_endpoint_use_cases.py when sensitive_log_mode is set, and in vllm_batch.py). The fix adds a guard in parse_args that injects a no-op argument definition only when vLLM has not already registered the flag — preserving forward compatibility on older vLLM versions while preventing unrecognized arguments errors on 0.17+.

  • The guard logic is correct: checks vLLM's own parser first, adds the shim only when needed.
  • The fix uses parser._actions (a private argparse attribute) for the existence check; parser._option_string_actions would be a more direct and equally idiomatic alternative.
  • The comment labels this "older model-engine versions pass --disable-log-requests", but the current model-engine codebase still passes this flag unconditionally (e.g. llm_model_endpoint_use_cases.py:997, vllm_batch.py:179), so the wording is mildly misleading about the longevity of this shim.
  • There are no new tests added, but the PR description notes manual endpoint verification with vLLM 0.17.0-rc1.

Confidence Score: 4/5

  • Safe to merge — the guard logic is correct and the fix directly addresses the vLLM 0.17+ startup failure.
  • The change is small, well-scoped, and logically correct. The only minor concern is using the private parser._actions attribute instead of the more direct parser._option_string_actions, which is a style nit rather than a correctness issue. The sensitive_log_mode behavioral regression (noted in a previous review thread) is the more substantive open question but is pre-existing and outside the scope of this fix.
  • No files require special attention beyond the already-discussed sensitive_log_mode behavioral gap.

Important Files Changed

Filename Overview
model-engine/model_engine_server/inference/vllm/vllm_server.py Adds a backward-compat shim to accept --disable-log-requests as a no-op when vLLM 0.17+ has removed the flag from its parser; uses private parser._actions for the existence check.

Flowchart

%%{init: {'theme': 'neutral'}}%%
flowchart TD
    A[vllm_server.py starts] --> B[FlexibleArgumentParser created]
    B --> C[make_arg_parser populates parser\nwith vLLM's known flags]
    C --> D{Is --disable-log-requests\nalready in parser._actions?}
    D -- Yes\nvLLM < 0.17 → vLLM handles it natively --> E[Skip — no-op shim not needed]
    D -- No\nvLLM 0.17+ removed the flag --> F[Add --disable-log-requests\nas no-op argument]
    E --> G[parser.parse_args\naccepts --disable-log-requests]
    F --> G
    G --> H[vllm_server launches successfully]

    I[llm_model_endpoint_use_cases.py\nif sensitive_log_mode:\n  vllm_args.disable_log_requests = True] -->|passes --disable-log-requests\nat runtime| A
Loading
Prompt To Fix All With AI
This is a comment left during a code review.
Path: model-engine/model_engine_server/inference/vllm/vllm_server.py
Line: 29

Comment:
**Accessing private `_actions` attribute**

`parser._actions` is a private, underscore-prefixed attribute of Python's `ArgumentParser`. While it is effectively stable across CPython versions and widely used for introspection, it is not part of the public API and could break with future argparse or `FlexibleArgumentParser` changes.

A more robust way to check if the flag is already registered is to inspect `parser._option_string_actions` (which is a dict keyed by flag string and is equally stable in practice) or to use a try/except approach:

```suggestion
    if "--disable-log-requests" not in parser._option_string_actions:
```

`_option_string_actions` is a direct `O(1)` dict lookup that maps each flag string (e.g. `"--disable-log-requests"`) to its `Action`, which is both more efficient and slightly more explicit than iterating over `_actions`.

How can I resolve this? If you propose a fix, please make it concise.

Last reviewed commit: "Merge branch 'main' ..."

Context used:

  • Rule used - Add comments to explain complex or non-obvious log... (source)

Learnt From
scaleapi/scaleapi#126958

…d compat

vLLM 0.17.0 removed --disable-log-requests from its arg parser, but
model-engine hardcodes this flag when launching vllm_server. Accept
the flag as a no-op when it's not present in the vLLM parser to avoid
startup failures with newer vLLM images.

Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
@lilyz-ai lilyz-ai requested a review from a team March 19, 2026 02:31
Comment on lines +26 to +36
# Backward compatibility: older model-engine versions pass --disable-log-requests
# which was removed from vLLM's arg parser in v0.17+. Accept it as a no-op.
if not any(
"--disable-log-requests" in getattr(a, "option_strings", []) for a in parser._actions
):
parser.add_argument(
"--disable-log-requests",
action="store_true",
default=False,
help="(deprecated, no-op) Kept for backward compatibility with older model-engine versions.",
)
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

P1 Silent no-op may break sensitive_log_mode compliance guarantees

The comment says "older model-engine versions pass --disable-log-requests", but the current model-engine code (llm_model_endpoint_use_cases.py:996-997) still hardcodes this flag whenever hmi_config.sensitive_log_mode is True:

if hmi_config.sensitive_log_mode:
    vllm_args.disable_log_requests = True

By silently accepting --disable-log-requests as a no-op in vLLM 0.17+, request logging will not be disabled for endpoints that have sensitive_log_mode set. If sensitive_log_mode is used for privacy or compliance reasons, this silent behavioral regression could expose request-level logs that were previously suppressed.

It would be worth either:

  1. Confirming that vLLM 0.17+ disables request logging by default (or via a different mechanism) and documenting that here, or
  2. Identifying the replacement mechanism in vLLM 0.17+ (e.g., a new flag or VLLM_CONFIGURE_LOGGING env var) and wiring sensitive_log_mode through that path alongside this no-op shim.
Prompt To Fix With AI
This is a comment left during a code review.
Path: model-engine/model_engine_server/inference/vllm/vllm_server.py
Line: 26-36

Comment:
**Silent no-op may break `sensitive_log_mode` compliance guarantees**

The comment says "older model-engine versions pass `--disable-log-requests`", but the *current* model-engine code (`llm_model_endpoint_use_cases.py:996-997`) still hardcodes this flag whenever `hmi_config.sensitive_log_mode` is `True`:

```python
if hmi_config.sensitive_log_mode:
    vllm_args.disable_log_requests = True
```

By silently accepting `--disable-log-requests` as a no-op in vLLM 0.17+, request logging will **not** be disabled for endpoints that have `sensitive_log_mode` set. If `sensitive_log_mode` is used for privacy or compliance reasons, this silent behavioral regression could expose request-level logs that were previously suppressed.

It would be worth either:
1. Confirming that vLLM 0.17+ disables request logging by default (or via a different mechanism) and documenting that here, or
2. Identifying the replacement mechanism in vLLM 0.17+ (e.g., a new flag or `VLLM_CONFIGURE_LOGGING` env var) and wiring `sensitive_log_mode` through that path alongside this no-op shim.

How can I resolve this? If you propose a fix, please make it concise.

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.

1 participant