-
-
Notifications
You must be signed in to change notification settings - Fork 11.6k
[Frontend] Remap -O to -cc commandline flag #29557
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
Conversation
|
Documentation preview: https://vllm--29557.org.readthedocs.build/en/29557/ |
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 remaps the -O command-line flag to -cc for compilation configuration, deprecating the former. The changes span documentation, tests, and argument parsing logic. While the updates to documentation and tests are correct, I've found a critical issue with the implementation of the deprecation warning for the -O flag. The current approach will not trigger the warning due to how arguments are pre-processed. I have provided detailed comments and suggestions to fix this issue by relocating the warning logic. Once addressed, the PR will correctly implement the intended functionality.
vllm/engine/arg_utils.py
Outdated
| "--compilation-config", | ||
| "-cc", | ||
| "-O", | ||
| action=CompilationConfigAction, |
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.
ProExpertProg
left a comment
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.
Thanks for implementing this! We should wait for #26847 to merge first. Can you make the deprecation comments and warnings mention the functionality will be removed in v0.13.0?
tests/utils_/test_argparse_utils.py
Outdated
| args = parser.parse_args(["-O.mode", "0"]) | ||
| assert args.compilation_config == {"mode": 0} | ||
|
|
||
| args = parser.parse_args(["-O.mode=NONE"]) | ||
| assert args.compilation_config == {"mode": "NONE"} |
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.
Add a comment that we're planning to deprecate these in v0.13.0?
vllm/config/vllm.py
Outdated
| As a shorthand, one can append compilation arguments via | ||
| -cc.parameter=argument such as `-cc.mode=3` (same as `-cc='{"mode":3}'`). | ||
| Optimization level shortcuts like `-O3` are also supported (equivalent to | ||
| -cc.mode=3). |
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.
This will no longer be true after #26247; -O<n> will map to optimization level.
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.
OK, I will wait for #26847 to land and fix up the logic.
vllm/engine/arg_utils.py
Outdated
| "--compilation-config", "-O", **vllm_kwargs["compilation_config"] | ||
| "--compilation-config", | ||
| "-cc", | ||
| "-O", |
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.
Yeah let's remove the -O here completely and just remap any use of -O.<...>/-O {...} to -cc in argparse_utils
|
This pull request has merge conflicts that must be resolved before it can be |
3149903 to
c556b06
Compare
Migrate compilation configuration from -O to -cc flags while preserving the new optimization level functionality that uses -O flags. Changes: - Update argparse_utils.py to handle -O flag disambiguation - Add deprecation warnings for -O.* dotted syntax for compilation config - Convert -O.backend, -O.mode, etc. to -cc.backend, -cc.mode with warnings - Update CLI argument parsing to use -cc instead of -O for compilation config - Update tests to use new -cc syntax for compilation config - Maintain backward compatibility with proper deprecation messages 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude <noreply@anthropic.com> Signed-off-by: Yanan Cao <gmagogsfm@gmail.com>
Replace all remaining references to deprecated -O.* compilation config syntax with the new -cc.* syntax throughout the codebase: - Documentation: Update debug guide and torch_compile.md examples - Tests: Update compilation tests to use -cc.* syntax - CI scripts: Update buildkite XPU test script - Config docs: Update VllmConfig docstring examples This ensures consistency across the codebase and removes deprecated usage examples that might confuse users. 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude <noreply@anthropic.com> Signed-off-by: Yanan Cao <gmagogsfm@gmail.com>
|
Documentation preview: https://vllm--29557.org.readthedocs.build/en/29557/ |
vllm/utils/argparse_utils.py
Outdated
| ): | ||
| # Convert -O <n> to --optimization-level <n> | ||
| processed_args.append("--optimization-level") | ||
| elif arg.startswith("-O") and arg[2] == ".": |
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.
| elif arg.startswith("-O") and arg[2] == ".": | |
| elif arg.startswith("-O."): |
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.
Done
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.
Done
vllm/utils/argparse_utils.py
Outdated
| if any( | ||
| cc_option in arg | ||
| for cc_option in [ | ||
| ".mode", | ||
| ".backend", | ||
| ".custom_option", | ||
| ".enable_fusion", | ||
| ".disable_custom_fusion", | ||
| ] |
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.
This list is not exhaustive and it shouldn't need to exist. Any use of -O. is deprecated
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.
removed
- Handle all -O.* dotted syntax as deprecated, not just specific options - Emit warning and convert -O.* to -cc.* syntax automatically - Update codebase references from -O.* to -cc.* syntax - Add comprehensive test for deprecation warning - Simplify condition checking and remove unnecessary scope parameter The -O.* dotted syntax is deprecated and will be removed in v0.13.0 or v1.0.0, whichever is earlier. Users should use -cc.* instead. Example: -cc.backend=eager instead of -O.backend=eager. Signed-off-by: Yanan Cao <gmagogsfm@gmail.com>
hmellor
left a comment
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.
LGTM, thanks!
Users can now:
-O{1,2,3}, which now means optimization level, not compilation mode any more.-O.xxx(deprecation warning until later time when we remove the support)-cc(no warning, preferred)--compilation-config(no change)Warning message: "The -O flag for --compilation-config is deprecated and will be removed in a later release. Please use -cc instead. Example: -cc.mode=0 instead of -O.mode=0"
Fixes #27832