Skip to content

Conversation

@aas008
Copy link
Collaborator

@aas008 aas008 commented Nov 10, 2025

  • Update TrialConfig to handle boolean False values by adding --no- prefix to CLI parameters (e.g., --no-enable-chunked-prefill)
  • Remove hardcoded --no-enable-prefix-caching from trial_controller as it's now handled by the boolean parameter logic
  • Fixes issue where boolean False values were not properly passed to vLLM CLI

Summary by CodeRabbit

  • New Features

    • Added explicit support for disabling boolean CLI options via --no- flags, matching vLLM conventions.
  • Chores

    • Adjusted vLLM server startup configuration by removing the prefix-caching toggle from startup arguments.

- Update TrialConfig to handle boolean False values by adding --no- prefix
  to CLI parameters (e.g., --no-enable-chunked-prefill)
- Remove hardcoded --no-enable-prefix-caching from trial_controller
  as it's now handled by the boolean parameter logic
- Fixes issue where boolean False values were not properly passed to vLLM CLI
@aas008 aas008 self-assigned this Nov 10, 2025
@coderabbitai
Copy link
Contributor

coderabbitai bot commented Nov 10, 2025

Walkthrough

Adds generation of negated boolean CLI flags (--no-<param>) for False vLLM arguments, and removes an explicit --no-enable-prefix-caching flag from the vLLM server startup command.

Changes

Cohort / File(s) Summary
Boolean flag negation support
auto_tune_vllm/core/trial.py
Generate --no-<param> CLI flags when boolean parameters are False; preserves existing --<param> for True. Non-boolean handling unchanged.
Trial controller startup configuration
auto_tune_vllm/execution/trial_controller.py
Remove explicit --no-enable-prefix-caching from the vLLM server startup arguments.

Sequence Diagram(s)

(Skipped — changes are limited to CLI argument generation and a startup flag removal; no control-flow changes warranting a sequence diagram.)

Estimated code review effort

🎯 2 (Simple) | ⏱️ ~10 minutes

  • Check auto_tune_vllm/core/trial.py for correct construction of --no-<param> (hyphenation, param name normalization) and ensure True/False branching doesn't affect non-boolean args.
  • Verify auto_tune_vllm/execution/trial_controller.py change doesn't inadvertently alter intended caching behavior or restart semantics.

Poem

🐰 I hop through flags both yes and no,
I tuck a "--no-" where false winds blow,
Startups trimmed and tidy, neat and bright,
Small tweaks, big harmony — code takes flight! ✨

Pre-merge checks and finishing touches

✅ Passed checks (3 passed)
Check name Status Explanation
Description Check ✅ Passed Check skipped - CodeRabbit’s high-level summary is enabled.
Title check ✅ Passed The title clearly summarizes the main change: adding support for boolean False values with --no- prefix, which aligns with the primary modifications across both modified files.
Docstring Coverage ✅ Passed Docstring coverage is 100.00% which is sufficient. The required threshold is 80.00%.
✨ Finishing touches
  • 📝 Generate docstrings
🧪 Generate unit tests (beta)
  • Create PR with unit tests
  • Post copyable unit tests in a comment

📜 Recent review details

Configuration used: CodeRabbit UI

Review profile: CHILL

Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between 8de59c4 and ef08ee1.

📒 Files selected for processing (1)
  • auto_tune_vllm/core/trial.py (1 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
  • auto_tune_vllm/core/trial.py

Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out.

❤️ Share

Comment @coderabbitai help to get the list of available commands and usage tips.

Copy link
Contributor

@coderabbitai coderabbitai bot left a comment

Choose a reason for hiding this comment

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

Actionable comments posted: 1

📜 Review details

Configuration used: CodeRabbit UI

Review profile: CHILL

Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between 7b6e8a5 and 8de59c4.

📒 Files selected for processing (2)
  • auto_tune_vllm/core/trial.py (1 hunks)
  • auto_tune_vllm/execution/trial_controller.py (0 hunks)
💤 Files with no reviewable changes (1)
  • auto_tune_vllm/execution/trial_controller.py
🧰 Additional context used
🪛 GitHub Actions: lint-ci
auto_tune_vllm/core/trial.py

[error] 94-94: E501 Line too long (100 > 88)

🪛 GitHub Check: ruff
auto_tune_vllm/core/trial.py

[failure] 94-94: Ruff (E501)
auto_tune_vllm/core/trial.py:94:89: E501 Line too long (100 > 88)

🔇 Additional comments (1)
auto_tune_vllm/core/trial.py (1)

89-98: Boolean handling logic is correct.

The implementation correctly handles boolean parameters:

  • True values generate --{param} flags
  • False values generate --no-{param} flags (aligning with vLLM's convention)
  • Non-boolean values generate --{param} {value} pairs

This follows the standard vLLM CLI pattern as documented in the comment.

args.append(f"--{cli_param}")
else:
# When False, add --no- prefix
# Evidence from vLLM help: --enable-chunked-prefill, --no-enable-chunked-prefill
Copy link
Contributor

Choose a reason for hiding this comment

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

⚠️ Potential issue | 🟡 Minor

Fix line length violation.

Line 94 exceeds the project's 88-character limit (currently 100 characters), causing the lint-ci pipeline to fail.

Apply this diff to fix the line length:

-                # Evidence from vLLM help: --enable-chunked-prefill, --no-enable-chunked-prefill
+                # Evidence from vLLM help: --enable-chunked-prefill,
+                # --no-enable-chunked-prefill
📝 Committable suggestion

‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.

Suggested change
# Evidence from vLLM help: --enable-chunked-prefill, --no-enable-chunked-prefill
# Evidence from vLLM help: --enable-chunked-prefill,
# --no-enable-chunked-prefill
🧰 Tools
🪛 GitHub Actions: lint-ci

[error] 94-94: E501 Line too long (100 > 88)

🪛 GitHub Check: ruff

[failure] 94-94: Ruff (E501)
auto_tune_vllm/core/trial.py:94:89: E501 Line too long (100 > 88)

🤖 Prompt for AI Agents
In auto_tune_vllm/core/trial.py around line 94, the comment "# Evidence from
vLLM help: --enable-chunked-prefill, --no-enable-chunked-prefill" exceeds the
88-character limit; split or wrap this comment into two shorter lines (e.g.,
keep the prefix on the first line and place the two flags on the following
indented comment line) so each line stays under 88 characters, preserving the
exact flag text and comment meaning.

@aas008 aas008 marked this pull request as draft November 10, 2025 22:46
style: fix ruff line length in trial.py
@aas008 aas008 marked this pull request as ready for review November 11, 2025 20:24
Copy link
Collaborator

@ephoris ephoris left a comment

Choose a reason for hiding this comment

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

LGTM! 🚀

@aas008 aas008 linked an issue Nov 11, 2025 that may be closed by this pull request
Copy link
Member

@thameem-abbas thameem-abbas left a comment

Choose a reason for hiding this comment

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

LGTM

@aas008 aas008 merged commit b6d0eab into openshift-psap:main Nov 11, 2025
2 of 3 checks passed
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.

Boolean parameters with False value are silently ignored

3 participants