-
-
Notifications
You must be signed in to change notification settings - Fork 13.4k
[Bugfix] FIx TorchAO config bugs #34430
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: main
Are you sure you want to change the base?
Conversation
|
Documentation preview: https://vllm--34430.org.readthedocs.build/en/34430/ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Code Review
This pull request introduces a useful enhancement by allowing --hf-overrides to be specified as either a JSON string or a file path. The implementation is clean, and the inclusion of tests and documentation is commendable. I've identified a couple of minor correctness issues where the loaded JSON is not validated to be a dictionary, which could lead to less clear error messages downstream. I've provided suggestions to address these. Overall, this is a solid improvement.
| raise ValueError("hf_overrides file path is empty.") | ||
| try: | ||
| with open(path, encoding="utf-8") as handle: | ||
| self.hf_overrides = json.load(handle) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The loaded JSON from the file should be validated to be a dictionary. The hf_overrides argument is expected to be a dictionary of overrides. Currently, if the file contains a JSON primitive (like a string or a number), it will be parsed successfully but will cause an AttributeError later when .items() is called on it. This can be confusing for the user. It's better to fail early with a clear error message.
loaded_json = json.load(handle)
if not isinstance(loaded_json, dict):
raise ValueError(
f"hf_overrides file must contain a JSON object: {path}"
)
self.hf_overrides = loaded_json|
|
||
| if re.match(r"(?s)^\s*{.*}\s*$", raw): | ||
| try: | ||
| self.hf_overrides = json.loads(raw) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Similar to the file loading case, the loaded JSON from the string should be validated to be a dictionary. If the string is a valid JSON primitive (e.g., \"a string\" or 123), it will be parsed successfully but will cause an AttributeError later. It's better to fail early with a clear error message ensuring the provided string is a JSON object.
loaded_json = json.loads(raw)
if not isinstance(loaded_json, dict):
raise ValueError("hf_overrides string must be a JSON object.")
self.hf_overrides = loaded_json|
This pull request has merge conflicts that must be resolved before it can be |
75a5ccb to
e12487e
Compare
Signed-off-by: jwpark33 <[email protected]>
e12487e to
90daf05
Compare
Purpose
Fix
--hf-overridesnormalization for TorchAO (and general HF overrides) so CLI inputs are handled consistently.This PR adds normalization logic in
EngineArgsto support:--hf-overrides '{"quantization_config_file": "..."}'@path, e.g.--hf-overrides '@/path/to/overrides.json'It also adds tests and docs updates:
vllm/engine/arg_utils.py: add_normalize_hf_overrides()tests/engine/test_arg_utils.py: add normalization tests for string/file/invalid casesvllm/config/model.py: clarify accepted string formats in docstringdocs/features/quantization/torchao.md: add online TorchAO usage examples with--hf-overridesTest Plan
Run targeted arg-utils tests for hf-overrides:
Test Result
All passed
Essential Elements of an Effective PR Description Checklist
supported_models.mdandexamplesfor a new model.