[Bugfix] Fail instead of ignoring when CompilationConfig gets invalid args#30708
Conversation
Signed-off-by: mgoin <mgoin64@gmail.com>
|
Codex usage limits have been reached for code reviews. Please check with the admins of this repo to increase the limits by adding credits. |
There was a problem hiding this comment.
Code Review
This pull request addresses an issue where invalid arguments for CompilationConfig were silently ignored. By adding config=ConfigDict(extra="forbid") to the Pydantic dataclasses PassConfig, DynamicShapesConfig, and CompilationConfig, the code now correctly raises a validation error for unexpected arguments. This improves the robustness of the configuration parsing and provides clear feedback to users about invalid flags. The related test cases in tests/benchmarks/test_param_sweep.py that used a now-invalid argument have been correctly removed. The changes are well-targeted and effectively resolve the issue. The implementation is clean and follows best practices for Pydantic configuration. I have reviewed the changes and found no issues.
hmellor
left a comment
There was a problem hiding this comment.
Good catch, this should probably be implemented in all config classes. At some point I'll have another look into merging
@config
@dataclassinto just
@configwhere defaults like this can be set.
… args (vllm-project#30708) Signed-off-by: mgoin <mgoin64@gmail.com> Signed-off-by: Ubuntu <mjtaheri68@gmail.com>
… args (vllm-project#30708) Signed-off-by: mgoin <mgoin64@gmail.com> Signed-off-by: dsuhinin <suhinin.dmitriy@gmail.com>
… args (vllm-project#30708) Signed-off-by: mgoin <mgoin64@gmail.com>
Purpose
I noticed this when trying to run with an old flag of
-cc.use_inductor=Falsewhere I saw that my arg was completely ignored:Now when I run the same command, I see a usage failure as expected:
Test Plan
Test Result
Essential Elements of an Effective PR Description Checklist
supported_models.mdandexamplesfor a new model.