refactor: remove schema usage #110
Conversation
WalkthroughStudyConfig.from_file was changed to delegate to ConfigValidator(vllm_version).load_and_validate which reads YAML, expands environment variables, and validates raw_config; legacy schema/default-loading and multiple versioned YAML schemas were removed; parameter type inference and Optuna-related typings were updated. Changes
Sequence Diagram(s)sequenceDiagram
participant User
participant StudyConfig
participant ConfigValidator
participant ParamBuilder
User->>StudyConfig: StudyConfig.from_file(config_path, vllm_version?)
StudyConfig->>ConfigValidator: ConfigValidator(vllm_version)
StudyConfig->>ConfigValidator: load_and_validate(config_path)
ConfigValidator->>ConfigValidator: read YAML file
ConfigValidator->>ConfigValidator: expand_environment_variables(yaml_text)
ConfigValidator->>ConfigValidator: parse YAML -> raw_config (dict)
loop each parameter in raw_config
ConfigValidator->>ParamBuilder: _infer_parameter_type(parameter_config)
ParamBuilder-->>ConfigValidator: inferred_type
ConfigValidator->>ParamBuilder: _build_parameter_config(name, parameter_config)
ParamBuilder-->>ConfigValidator: ParameterConfig
end
ConfigValidator-->>StudyConfig: validated StudyConfig
StudyConfig-->>User: return StudyConfig
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Possibly related PRs
Suggested reviewers
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 |
remove parsing of schema for fallbacks on values
ephoris
force-pushed
the
refactor/remove_schmea_usage
branch
from
e04a0ce to
e35dd96
Compare
ephoris
force-pushed
the
refactor/remove_schmea_usage
branch
from
e35dd96 to
1a7b7e1
Compare
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
auto_tune_vllm/execution/trial_controller.py (1)
864-871: Redact sensitive env vars when logging.Avoid logging secrets (tokens, passwords). Redact values by pattern.
Apply this diff:
- if trial_env_vars: - env.update(trial_env_vars) - vllm_logger.info(f"Environment variables: {trial_env_vars}") + if trial_env_vars: + env.update(trial_env_vars) + import re as _re + secret_pat = _re.compile(r"(TOKEN|KEY|SECRET|PASSWORD|PWD|PASS)", _re.I) + redacted = { + k: ("***" if secret_pat.search(k) else v) + for k, v in trial_env_vars.items() + } + vllm_logger.info(f"Environment variables: {redacted}")auto_tune_vllm/core/study_controller.py (1)
351-384: Critical: grid generation mis-detects float ranges after data_type change.RangeParameter.data_type is now a type (float|int), but code at line 363 compares to string "float", forcing all float ranges down the integer path. This causes incorrect grid generation for all floating-point ranges.
Apply this diff:
- # Use integer-based generation for floats - # to avoid accumulation drift - if param_config.data_type == "float": + # Use integer-based generation for floats to avoid drift + is_float = ( + getattr(param_config, "data_type", None) is float + or getattr(param_config, "data_type", None) == "float" + ) + if is_float:
🧹 Nitpick comments (6)
auto_tune_vllm/core/config.py (4)
329-338: Use logging (not print) and guard against empty YAML defaults.Replace print with logger warnings and handle YAML loading returning None.
Apply this diff:
@@ -import yaml +import yaml +import logging +logger = logging.getLogger(__name__) @@ - with open(self.defaults_path) as f: - defaults_data = yaml.safe_load(f) + with open(self.defaults_path) as f: + defaults_data = yaml.safe_load(f) or {} @@ - except Exception as e: - print(f"Warning: Could not load vLLM defaults for version {version}: {e}") + except Exception as e: + logger.warning( + "Could not load vLLM defaults for version %s: %s", version, e + ) @@ - print( - f"Warning: Environment variable '{var_name}' not found, " - f"using empty string" - ) + logger.warning( + "Environment variable '%s' not found, using empty string", + var_name, + )Also applies to: 348-352, 421-425
519-534: Validate env parameter options are a non-empty list.Type-check and fail early if options isn’t a list or is empty.
Apply this diff:
- if "options" not in param_config: + if "options" not in param_config: raise ValueError( f"Environment parameter '{param_name}' " f"must specify options as a list" ) + options = param_config["options"] + if not isinstance(options, list) or len(options) == 0: + raise TypeError( + f"Environment parameter '{param_name}' options must be a non-empty list" + )
310-318: Fix truncated class docstring.Docstring reads incomplete (“against parameter”). Clarify intent.
Apply this diff:
-class ConfigValidator: - """Validates study configurations against parameter""" +class ConfigValidator: + """Validates study configurations, builds ParameterConfig objects, and assembles StudyConfig."""
702-715: Streamline builder and avoid unused temporary dict.Use user_config directly; no need for base dict.
Apply this diff:
- base = { - "name": name, - "enabled": user_config.get("enabled", True), - } - base.update(user_config) + enabled = user_config.get("enabled", True) @@ - if param_type is float or param_type is int: + if param_type is float or param_type is int: # TODO: Separate out into RangeIntParamter and RangeFloatParameter types return RangeParameter( - name=name, - min=user_config.get("min"), - max=user_config.get("max"), - step=base.get("step", None), - data_type=param_type, + name=name, + min=user_config.get("min"), + max=user_config.get("max"), + step=user_config.get("step"), + data_type=param_type, ) @@ - elif param_type is list: + elif param_type is list: return ListParameter( - name=name, - options=user_config.get("options"), + name=name, + options=user_config.get("options"), )auto_tune_vllm/execution/trial_controller.py (1)
345-353: Nit: simplify exception handling for Ray timeout.GetTimeoutError is a subclass of Exception; catching Exception alone suffices.
Apply this diff:
- except (Exception, GetTimeoutError) as _: + except Exception: return Falseauto_tune_vllm/core/parameters.py (1)
16-19: Optional: align base method signature with concrete overrides.Annotate ParameterConfig.generate_optuna_suggest with trial: optuna.Trial for consistency.
Apply this diff:
- def generate_optuna_suggest(self, trial) -> Any: + def generate_optuna_suggest(self, trial: optuna.Trial) -> Any: """Generate Optuna trial suggestion for this parameter."""Also applies to: 30-32
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (8)
auto_tune_vllm/core/config.py(6 hunks)auto_tune_vllm/core/parameters.py(2 hunks)auto_tune_vllm/core/study_controller.py(2 hunks)auto_tune_vllm/execution/trial_controller.py(26 hunks)auto_tune_vllm/schemas/parameter_schema_original.yaml(0 hunks)auto_tune_vllm/schemas/v0_10_0.yaml(0 hunks)auto_tune_vllm/schemas/v0_10_1_1.yaml(0 hunks)auto_tune_vllm/schemas/v0_11_0.yaml(0 hunks)
💤 Files with no reviewable changes (4)
- auto_tune_vllm/schemas/v0_10_1_1.yaml
- auto_tune_vllm/schemas/v0_11_0.yaml
- auto_tune_vllm/schemas/v0_10_0.yaml
- auto_tune_vllm/schemas/parameter_schema_original.yaml
🧰 Additional context used
🧠 Learnings (1)
📓 Common learnings
Learnt from: ephoris
PR: openshift-psap/auto-tuning-vllm#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.
🧬 Code graph analysis (2)
auto_tune_vllm/execution/trial_controller.py (2)
auto_tune_vllm/execution/backends.py (1)
is_cancelled(37-39)auto_tune_vllm/benchmarks/providers.py (9)
terminate_benchmark(42-138)set_logger(31-33)set_trial_context(35-40)start_benchmark(141-154)start_benchmark(188-249)start_benchmark(456-478)parse_results(157-165)parse_results(251-269)parse_results(480-506)
auto_tune_vllm/core/config.py (1)
auto_tune_vllm/core/parameters.py (4)
ParameterConfig(9-19)RangeParameter(22-46)ListParameter(49-57)BooleanParameter(60-67)
🔇 Additional comments (2)
auto_tune_vllm/core/study_controller.py (1)
605-609: LGTM: vLLM startup timeout parsing is straightforward.int(...) cast is appropriate; upstream validation ensures a numeric string is provided. Based on learnings.
If you want a safety net, add a config pre-check rather than try/except here.
auto_tune_vllm/core/parameters.py (1)
25-46: RangeParameter typing and Optuna integration look good.Switching data_type to a Python type and using identity checks is solid.
|
@ephoris I believe "defaults_path" is also not in use. |
Removed |
There was a problem hiding this comment.
Actionable comments posted: 3
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
auto_tune_vllm/core/config.py (1)
437-481: Environment parameter options: enforce list[str] and coerce; clearer errors.Current code assumes options is present but not its type/content. If options is not a list or contains non-strings, downstream code will misbehave when exporting env vars.
@@ - if "options" not in param_config: + if "options" not in param_config: raise ValueError( f"Environment parameter '{param_name}' " f"must specify options as a list" ) - - validated_param = EnvironmentParameter( + options = param_config["options"] + if not isinstance(options, list) or len(options) == 0: + raise ValueError( + f"Environment parameter '{param_name}' must provide a non-empty list of options" + ) + # Env vars are strings; coerce to str for consistency + options_str = [str(o) for o in options] + + validated_param = EnvironmentParameter( name=param_name, enabled=param_config.get("enabled", True), - options=param_config["options"], + options=options_str, data_type=param_config.get("data_type", "str"), description=param_config.get( "description", f"Environment variable {param_name}" ), )
🧹 Nitpick comments (3)
auto_tune_vllm/core/config.py (3)
353-369: Avoid print in env var expansion; use logging and optionally fail fast.Printing to stdout during config load is noisy. Use logger.warning and consider a strict mode to raise on missing required env vars (no default).
Apply these diffs:
@@ -import yaml +import yaml +import logging +logger = logging.getLogger(__name__) @@ - else: - # Log warning for missing required env vars without defaults - print( - f"Warning: Environment variable '{var_name}' not found, " - f"using empty string" - ) - return "" + else: + logger.warning( + "Environment variable '%s' not found, using empty string", + var_name, + ) + return ""Based on learnings.
421-435: Replace study-naming prints with logger calls.Use logger.info/debug instead of print for operational messages.
- print( - f"Using explicit study name: {name} (will fail if study already exists)" - ) + logger.info( + "Using explicit study name: %s (will fail if study already exists)", + name, + ) @@ - print(f"Generated study name: {auto_name} from prefix: {prefix}") + logger.info("Generated study name: %s from prefix: %s", auto_name, prefix) @@ - print(f"Auto-generated study name: {auto_name}") + logger.info("Auto-generated study name: %s", auto_name)
376-389: Reduce collision risk in auto-generated study names.time.time() % 1e6 can collide under concurrent runs. Prefer uuid4 (shortened) for safer uniqueness.
- import time - - timestamp_seconds = int(time.time()) - # Take last 6 digits to keep numbers reasonable - study_number = timestamp_seconds % 1000000 - - if prefix: - return f"{prefix}_{study_number}" - else: - return f"study_{study_number}" + import uuid + suffix = uuid.uuid4().hex[:8] + return f"{prefix}_{suffix}" if prefix else f"study_{suffix}"
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
auto_tune_vllm/core/config.py(6 hunks)scripts/generate_vllm_defaults.py(0 hunks)
💤 Files with no reviewable changes (1)
- scripts/generate_vllm_defaults.py
🧰 Additional context used
🧠 Learnings (1)
📓 Common learnings
Learnt from: thameem-abbas
PR: openshift-psap/auto-tuning-vllm#20
File: examples/study_config.yaml:4-5
Timestamp: 2025-09-02T19:23:04.775Z
Learning: The ConfigValidator.load_and_validate() method in auto_tune_vllm/core/config.py will implement environment variable expansion using Option 1: pre-processing YAML content with regex pattern matching for ${VAR} and ${VAR:-default} syntax before calling yaml.safe_load().
🧬 Code graph analysis (1)
auto_tune_vllm/core/config.py (1)
auto_tune_vllm/core/parameters.py (4)
ParameterConfig(9-19)RangeParameter(22-46)ListParameter(49-57)BooleanParameter(60-67)
🔇 Additional comments (2)
auto_tune_vllm/core/config.py (2)
16-22: EnvironmentParameter class and constructor are correctly used.All kwargs in the instantiation match valid field definitions:
nameandenabledare inherited from theParameterConfigparent classoptionsanddata_typeare defined inEnvironmentParameterdescriptionis inherited fromParameterConfigSince
ParameterConfigextends Pydantic'sBaseModel, the constructor accepts these fields as keyword arguments. No issues found.
301-306: Signature change verified complete; all call sites already migrated.Verification confirms the StudyConfig.from_file signature change is correct and all call sites across the codebase have already been updated to use the new signature (config_path + optional vllm_version only). No old-style calls with schema_path/defaults_path arguments found, and no stale parameters remain in the ConfigValidator implementation. The migration is consistent and complete.
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (3)
auto_tune_vllm/core/config.py (3)
309-309: Docstring is incomplete.The class docstring is truncated at "against parameter" - this was previously flagged and should be completed to clearly describe the class responsibility.
611-634: Require both min and max for range parameters.Line 614 uses
any(range_check)which proceeds if eitherminormaxis present, but then lines 615-620 attempt to validate both values. If only one bound is provided, the other will beNone, causing a misleadingTypeErrorat line 620. Range parameters must specify both bounds.Apply this diff to require and validate both bounds:
def _infer_parameter_type(self, parameter_config: dict[str, Any]): - range_check = ["max" in parameter_config, "min" in parameter_config] - list_check = "options" in parameter_config - if any(range_check): + has_min = "min" in parameter_config + has_max = "max" in parameter_config + list_check = "options" in parameter_config + + if has_min or has_max: + if not (has_min and has_max): + raise ValueError("Range parameter requires both 'min' and 'max'") min_value = parameter_config.get("min") max_value = parameter_config.get("max") - min_is_numeric = isinstance(min_value, (int, float)) - max_is_numeric = isinstance(max_value, (int, float)) - if not min_is_numeric or not max_is_numeric: + if not isinstance(min_value, (int, float)) or not isinstance(max_value, (int, float)): raise TypeError("'min' and 'max' must be numbers (int or float)") - # check or clause incase users do (0, 1.0) as a range if isinstance(max_value, float) or isinstance(min_value, float): return float else: return int elif list_check: options = parameter_config.get("options") if not isinstance(options, list): raise TypeError("'options' must be a list") if options == [True, False] or options == [False, True]: return bool return list - raise ValueError(f"Unable to parse parameter {parameter_config}")
656-656: Fix undefined variable reference.Line 656 references
base.get("step", None), butbaseis not defined in this method. This should beuser_config.get("step")orcommon.get("step").Apply this diff:
return RangeParameter( name=name, min=user_config.get("min"), max=user_config.get("max"), - step=base.get("step", None), + step=user_config.get("step"), data_type=param_type, )
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
auto_tune_vllm/core/config.py(6 hunks)
🧰 Additional context used
🧠 Learnings (1)
📓 Common learnings
Learnt from: thameem-abbas
PR: openshift-psap/auto-tuning-vllm#20
File: examples/study_config.yaml:4-5
Timestamp: 2025-09-02T19:14:51.253Z
Learning: The auto-tune-vllm configuration system currently does not support environment variable expansion in YAML files. The StudyConfig.from_file() method uses yaml.safe_load() directly without any preprocessing for environment variables like ${VAR} or ${VAR:-default}.
🧬 Code graph analysis (1)
auto_tune_vllm/core/config.py (1)
auto_tune_vllm/core/parameters.py (4)
ParameterConfig(9-19)RangeParameter(22-46)ListParameter(49-57)BooleanParameter(60-67)
🔇 Additional comments (5)
auto_tune_vllm/core/config.py (5)
298-305: LGTM!The simplified signature correctly removes schema and defaults path handling, aligning with the PR's goal to eliminate external schema dependencies.
311-315: LGTM!Constructor correctly simplified to accept only
vllm_version, consistent with removing schema/defaults dependencies.
437-437: LGTM!The updated signature correctly reflects the schema-less validation approach, accepting only raw_config.
446-478: LGTM!Environment variable parameter handling correctly validates that env vars can only use list options, not range configurations, with clear error messages.
516-522: LGTM!Study naming logic correctly handles missing study section by defaulting to an empty dict, allowing auto-generation to proceed.
ephoris
force-pushed
the
refactor/remove_schmea_usage
branch
from
7e8635c to
4726c7e
Compare
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (1)
auto_tune_vllm/core/config.py (1)
309-309: Complete the truncated docstring.The class docstring is incomplete ("against parameter"). Consider clarifying the class's purpose.
Apply this diff:
- """Validates study configurations against parameter""" + """Validates study configurations and builds ParameterConfig objects without external schemas."""
🧹 Nitpick comments (3)
auto_tune_vllm/core/config.py (3)
363-366: Consider using a logger instead of print() for warnings.Library code should use structured logging rather than print statements for better control and consistency with the rest of the codebase.
Apply this diff:
+import logging + +logger = logging.getLogger(__name__) ... if default_value: return default_value else: # Log warning for missing required env vars without defaults - print( + logger.warning( f"Warning: Environment variable '{var_name}' not found, " f"using empty string" )
611-635: Consider validating that both min and max are present for range parameters.The current logic uses
any(range_check)which allows partial ranges, but then the isinstance check will fail onNonevalues with a generic TypeError. Consider explicitly checking that both min and max are present and providing a clearer error message.Example improvement:
def _infer_parameter_type(self, parameter_config: dict[str, Any]): - range_check = ["max" in parameter_config, "min" in parameter_config] - list_check = "options" in parameter_config - if any(range_check): + has_min = "min" in parameter_config + has_max = "max" in parameter_config + has_options = "options" in parameter_config + + if has_min or has_max: + if not (has_min and has_max): + raise ValueError("Range parameters require both 'min' and 'max'") min_value = parameter_config.get("min") max_value = parameter_config.get("max") min_is_numeric = isinstance(min_value, (int, float)) max_is_numeric = isinstance(max_value, (int, float)) if not min_is_numeric or not max_is_numeric: raise TypeError("'min' and 'max' must be numbers (int or float)") # check or clause incase users do (0, 1.0) as a range if isinstance(max_value, float) or isinstance(min_value, float): return float else: return int - elif list_check: + elif has_options: options = parameter_config.get("options") if not isinstance(options, list): raise TypeError("'options' must be a list") if options == [True, False] or options == [False, True]: return bool return list raise ValueError(f"Unable to parse parameter {parameter_config}")
636-664: Add validation for range parameters to prevent runtime issues.The method lacks several important validations:
- No check that
min <= max- No check that
step > 0when providedcommon.update(user_config)at line 649 merges all user fields, potentially passing unexpected fields to parameter constructorsConsider adding explicit validation:
def _build_parameter_config( self, name: str, user_config: dict[str, Any] ) -> ParameterConfig: """Build parameter config from user config.""" try: param_type = self._infer_parameter_type(user_config) except ValueError as _: raise ValueError(f"Unable to parse {name}: {user_config}") common = { "name": name, "enabled": user_config.get("enabled", True), + "description": user_config.get("description"), } - common.update(user_config) + if param_type is float or param_type is int: + min_v = user_config.get("min") + max_v = user_config.get("max") + if min_v is None or max_v is None: + raise ValueError(f"Range parameter '{name}' requires both 'min' and 'max'") + if float(min_v) > float(max_v): + raise ValueError(f"'min' must be <= 'max' for '{name}'") + + step_v = user_config.get("step") + if step_v is not None and float(step_v) <= 0: + raise ValueError(f"'step' for '{name}' must be > 0") + # TODO: Separate out into RangeIntParamter and RangeFloatParameter types return RangeParameter( - name=name, - min=user_config.get("min"), - max=user_config.get("max"), - step=common.get("step", None), + **common, + min=min_v, + max=max_v, + step=step_v, data_type=param_type, ) elif param_type is list: - return ListParameter(name=name, options=user_config.get("options")) + options = user_config.get("options", []) + if not options: + raise ValueError(f"List parameter '{name}' requires non-empty 'options'") + return ListParameter(**common, options=options) elif param_type is bool: - return BooleanParameter(name=name) + return BooleanParameter(**common) else: raise ValueError(f"Unknown parameter type: {param_type}")
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
auto_tune_vllm/core/config.py(6 hunks)
🧰 Additional context used
🧬 Code graph analysis (1)
auto_tune_vllm/core/config.py (1)
auto_tune_vllm/core/parameters.py (4)
ParameterConfig(9-19)RangeParameter(22-46)ListParameter(49-57)BooleanParameter(60-67)
🔇 Additional comments (3)
auto_tune_vllm/core/config.py (3)
298-305: LGTM! Clean refactoring to remove schema dependencies.The simplified signature and delegation to
ConfigValidatoraligns well with the PR objectives. The removal ofschema_pathanddefaults_pathparameters makes the interface cleaner.
317-330: LGTM! Well-structured configuration loading flow.The method clearly separates file reading, environment variable expansion, YAML parsing, and validation. Good error handling with
FileNotFoundError.
437-609: LGTM! Comprehensive validation logic.The refactored validation method correctly handles environment variables, static parameters, and delegates parameter config building appropriately. The flow is clear and maintainable.
Summary
Currently we rely on a hand-written schema file to check compatibility amongst parameters. This change removes this usage as we had to keep parity with the schema file across versions. Reminder that vLLM handles things by default and will do internal calculations on what gets set, so there is no reason for us to rely on reading the schema.
This PR should address #107
Notable
Summary by CodeRabbit
New Features
Bug Fixes
Refactor
Chores