-
-
Couldn't load subscription status.
- Fork 10.9k
[Misc] Clean up utils #27552
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
[Misc] Clean up utils #27552
Conversation
Signed-off-by: DarkLight1337 <[email protected]>
Signed-off-by: DarkLight1337 <[email protected]>
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 cleans up the utility modules by refactoring code into more specific files, removing unused functions, and eliminating backward compatibility shims. The changes are well-structured and improve the organization of the codebase. I've found one critical issue where argument pre-processing can lead to an unhandled exception, and I've provided a recommendation to add a test case to expose this bug and guide its resolution.
| def test_model_specification( | ||
| parser_with_config, cli_config_file, cli_config_file_with_model | ||
| ): |
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 FlexibleArgumentParser.parse_args method has a bug that causes an IndexError if vllm serve is called with --model as the last argument without a value. This should be handled gracefully by argparse (which would raise a SystemExit), but the current pre-processing logic crashes instead.
The crash occurs because the code at vllm/utils/argparse_utils.py:227 accesses args[model_idx + 1] without checking if the index is valid.
To ensure robust error handling and prevent this crash, I recommend adding a test case to expose this bug. This will help in fixing the underlying issue in FlexibleArgumentParser.parse_args and prevent future regressions.
Here is a suggested test case to add to this file:
def test_model_option_no_value(parser_with_config):
with pytest.raises(SystemExit):
parser_with_config.parse_args(["serve", "--model"])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.
💡 Codex Review
vllm/vllm/entrypoints/cli/serve.py
Lines 18 to 23 in 536298f
| from vllm.entrypoints.utils import VLLM_SUBCMD_PARSER_EPILOG | |
| from vllm.logger import init_logger | |
| from vllm.usage.usage_lib import UsageContext | |
| from vllm.utils import FlexibleArgumentParser | |
| from vllm.utils.network_utils import get_tcp_uri | |
| from vllm.utils.system_utils import decorate_logs, set_process_title |
The utils refactor removed the re‑export of argparse helpers from vllm.utils, but the CLI entrypoint still imports FlexibleArgumentParser from the old module. Running vllm serve will now raise ImportError: cannot import name 'FlexibleArgumentParser' from 'vllm.utils' before any parsing happens. This needs to import from vllm.utils.argparse_utils like the other entrypoints.
vllm/vllm/distributed/kv_transfer/kv_connector/v1/decode_bench_connector.py
Lines 39 to 46 in 536298f
| from vllm.distributed.kv_transfer.kv_connector.v1 import ( | |
| KVConnectorBase_V1, | |
| KVConnectorRole, | |
| ) | |
| from vllm.distributed.kv_transfer.kv_connector.v1.base import KVConnectorMetadata | |
| from vllm.logger import init_logger | |
| from vllm.utils import cdiv | |
The commit removes the math helper aliases from vllm.utils, yet this file still imports cdiv from that module. Importing the connector will now fail with ImportError when running decode benchmarking or any KV‑transfer workflow that loads the module. The import should point to vllm.utils.math_utils.cdiv (and similar changes may be needed in lmcache_integration/vllm_v1_adapter.py).
vllm/vllm/lora/punica_wrapper/punica_gpu.py
Lines 14 to 17 in 536298f
| from vllm.lora.layers import LoRAMapping | |
| from vllm.triton_utils import HAS_TRITON, triton | |
| from vllm.utils import round_up | |
After the refactor, round_up is no longer re‑exported from vllm.utils, so this module (and other users such as layers/quantization/mxfp4.py) will fail to import with ImportError. Replace the import with from vllm.utils.math_utils import round_up or similar to match the new organization.
ℹ️ About Codex in GitHub
Codex has been enabled to automatically review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".
Signed-off-by: DarkLight1337 <[email protected]>
Signed-off-by: DarkLight1337 <[email protected]>
|
/gemini review |
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 provides a good cleanup of the vllm.utils module. The changes primarily involve refactoring by moving utilities into more specific submodules like argparse_utils and math_utils, and removing several unused or legacy (V0) functions and backward compatibility shims. The import paths across the codebase have been updated consistently to reflect these changes. The refactoring improves code organization and maintainability. I have reviewed the changes and found no issues.
Signed-off-by: DarkLight1337 <[email protected]>
|
Documentation preview: https://vllm--27552.org.readthedocs.build/en/27552/ |
Signed-off-by: DarkLight1337 <[email protected]>
Purpose
Part of #26900
Test Plan
Test Result
Essential Elements of an Effective PR Description Checklist
supported_models.mdandexamplesfor a new model.