Fix/n startup sampler - #121
Conversation
- 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
openshift-psap#115) This commit fixes the broken link between the n_startup_trials config parameter and the actual sampler creation, ensuring user-defined values are properly used. Changes: - Set explicit default n_startup_trials=10 in OptimizationConfig (matching Optuna default) - Updated _create_sampler() to read and pass n_startup_trials to supported samplers - Added validation to ensure n_startup_trials < n_trials (prevents all-random trials) - Now passes n_startup_trials to: TPESampler, GPSampler, and BoTorchSampler - Added informative logging showing sampler configuration Fixes openshift-psap#115
openshift-psap#115) This commit fixes the broken link between the n_startup_trials config parameter and the actual sampler creation, ensuring user-defined values are properly used. Changes: - Set explicit default n_startup_trials=10 in OptimizationConfig (matching Optuna default) - Updated _create_sampler() to read and pass n_startup_trials to supported samplers - Added validation to ensure n_startup_trials < n_trials (prevents all-random trials) - Now passes n_startup_trials to: TPESampler, GPSampler, and BoTorchSampler - Added informative logging showing sampler configuration Fixes openshift-psap#115
WalkthroughDefault startup trials set to 10; study controller validates startup < total trials and passes startup count to samplers; boolean CLI flags now emit Changes
Sequence Diagram(s)sequenceDiagram
participant Config as Config
participant Controller as StudyController
participant Sampler as Optuna Sampler
participant TrialExec as TrialExecution
Config->>Controller: Provide OptimizationConfig (n_startup_trials=10)
Controller->>Controller: Validate n_startup_trials < n_trials
alt valid
Controller->>Sampler: Create sampler (n_startup_trials=10)
Sampler->>Sampler: Initialize with startup trials
TrialExec->>TrialExec: Build CLI flags (--param / --no-param)
TrialExec->>TrialExec: Start vLLM server (without --no-enable-prefix-caching)
else invalid
Controller->>Controller: Raise ValueError
end
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes
Poem
Pre-merge checks and finishing touches✅ Passed checks (3 passed)
✨ Finishing touches
🧪 Generate unit tests (beta)
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. Comment |
Removed detailed comments on boolean parameter handling in vllm_args property.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
auto_tune_vllm/core/config.py(1 hunks)auto_tune_vllm/core/study_controller.py(1 hunks)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
🧠 Learnings (2)
📓 Common learnings
Learnt from: ephoris
Repo: openshift-psap/auto-tuning-vllm PR: 104
File: auto_tune_vllm/core/study_controller.py:605-609
Timestamp: 2025-10-20T20:40:13.235Z
Learning: In the auto-tuning-vllm codebase, configuration validation is performed at entry points before reaching execution paths like trial submission in study_controller.py. Defensive try-except blocks around config value parsing (e.g., VLLM_STARTUP_TIMEOUT) are unnecessary and considered bloat since validation has already occurred upstream.
📚 Learning: 2025-10-20T20:40:13.235Z
Learnt from: ephoris
Repo: openshift-psap/auto-tuning-vllm PR: 104
File: auto_tune_vllm/core/study_controller.py:605-609
Timestamp: 2025-10-20T20:40:13.235Z
Learning: In the auto-tuning-vllm codebase, configuration validation is performed at entry points before reaching execution paths like trial submission in study_controller.py. Defensive try-except blocks around config value parsing (e.g., VLLM_STARTUP_TIMEOUT) are unnecessary and considered bloat since validation has already occurred upstream.
Applied to files:
auto_tune_vllm/core/study_controller.py
🪛 GitHub Actions: lint-ci
auto_tune_vllm/core/config.py
[error] 72-72: Ruff: E501 Line too long (117 > 88)
🪛 GitHub Check: ruff
auto_tune_vllm/core/config.py
[failure] 72-72: Ruff (E501)
auto_tune_vllm/core/config.py:72:89: E501 Line too long (117 > 88)
🔇 Additional comments (5)
auto_tune_vllm/core/study_controller.py (3)
401-412: Validation logic is sound and user-friendly.The check ensures that startup trials don't consume all available trials, which would result in purely random sampling. The error message provides actionable suggestions for resolution.
414-418: Clear and informative logging.The log message helps users understand the sampler configuration being used.
420-431: The sampler constructors do acceptn_startup_trialsas a parameter in Optuna's API. The code correctly passes this parameter to TPESampler, GPSampler, and BoTorchSampler with the exact parameter name required by the library.auto_tune_vllm/core/config.py (1)
72-72: The default value of Optuna's TPESampler'sn_startup_trialsis 10, confirming the code comment is accurate.auto_tune_vllm/core/trial.py (1)
92-94: I need to verify whether vLLM's argument parser supports--no-prefix for ALL boolean parameters, or if there are any exceptions. Let me search for vLLM's actual argument handling:The web search confirms vLLM uses
argparse.BooleanOptionalAction, which is Python's standard mechanism for boolean flags with automatic--no-prefix support. However, I need to verify how the parameter names are being generated in trial.py and whether they align with vLLM's actual parameter names.Based on my verification using both shell scripts and web searches, vLLM uses
argparse.BooleanOptionalActionfor boolean arguments, which automatically supports the--no-prefix negation syntax. For example,--enable-prefix-cachingcan be negated as--no-enable-prefix-caching.The code change in trial.py correctly implements this pattern by emitting
--no-<param>for False boolean values. The existing codebase invllm_cli_parser.pyalready recognizes--no-as a valid boolean flag indicator, and the change aligns with vLLM's CLI conventions.
All boolean parameters in vLLM support the
--no-prefix syntax throughargparse.BooleanOptionalAction; the implementation is correct.
Updated comment for n_startup_trials for clarity.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
auto_tune_vllm/core/config.py (1)
72-72: Good change; remove trailing whitespace.Making
n_startup_trialsnon-optional with a sensible default of 10 simplifies downstream logic and aligns with Optuna's TPESampler default.Remove the trailing whitespace after the comment to keep the code clean:
- n_startup_trials: int = 10 # Number of random startup trials + n_startup_trials: int = 10 # Number of random startup trials
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
auto_tune_vllm/core/config.py(1 hunks)
🧰 Additional context used
🧠 Learnings (1)
📚 Learning: 2025-10-20T20:40:13.235Z
Learnt from: ephoris
Repo: openshift-psap/auto-tuning-vllm PR: 104
File: auto_tune_vllm/core/study_controller.py:605-609
Timestamp: 2025-10-20T20:40:13.235Z
Learning: In the auto-tuning-vllm codebase, configuration validation is performed at entry points before reaching execution paths like trial submission in study_controller.py. Defensive try-except blocks around config value parsing (e.g., VLLM_STARTUP_TIMEOUT) are unnecessary and considered bloat since validation has already occurred upstream.
Applied to files:
auto_tune_vllm/core/config.py
2 participants
Summary by CodeRabbit
Configuration Changes
Improvements