-
-
Notifications
You must be signed in to change notification settings - Fork 17.7k
Fix #26037: Skip CUDA platform detection when displaying help #33550
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?
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,66 @@ | ||
| # SPDX-License-Identifier: Apache-2.0 | ||
| # SPDX-FileCopyrightText: Copyright contributors to the vLLM project | ||
| """Tests for the CLI main entrypoint to ensure help doesn't trigger | ||
| platform detection. | ||
| """ | ||
|
|
||
| import sys | ||
| from unittest.mock import patch | ||
|
|
||
| import pytest | ||
|
|
||
|
|
||
| @pytest.mark.parametrize( | ||
| "argv", | ||
| [ | ||
| ["vllm", "--help"], | ||
| ["vllm", "serve", "--help"], | ||
| ["vllm", "-h"], | ||
| ["vllm", "bench", "--help"], | ||
| ["vllm", "serve", "--help=ModelConfig"], | ||
| ], | ||
| ) | ||
| def test_needs_help_detects_help_flags(argv): | ||
| """Test that needs_help() correctly detects help flags in sys.argv.""" | ||
| from vllm.engine.arg_utils import needs_help | ||
|
|
||
| # patch.object on sys.argv is safe — it's a simple list attribute | ||
| # with no lazy-init or side-effect machinery. | ||
| with patch.object(sys, "argv", argv): | ||
| assert needs_help(), f"needs_help() should return True for {argv}" | ||
|
|
||
|
|
||
| @pytest.mark.parametrize( | ||
| "argv", | ||
| [ | ||
| ["vllm", "serve", "--model", "test"], | ||
| ["vllm", "bench", "latency", "--model", "test"], | ||
| ["vllm", "collect-env"], | ||
| ], | ||
| ) | ||
| def test_needs_help_returns_false_without_help_flags(argv): | ||
| """Test that needs_help() returns False when no help flag is present.""" | ||
| from vllm.engine.arg_utils import needs_help | ||
|
|
||
| with patch.object(sys, "argv", argv): | ||
| assert not needs_help(), f"needs_help() should return False for {argv}" | ||
|
Comment on lines
+13
to
+46
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. These can be merged into def test_needs_help(argv, expected):
... |
||
|
|
||
|
|
||
| def test_bench_help_skips_platform_detection(): | ||
| """Test that the bench guard in main() is skipped when --help is present. | ||
|
|
||
| The guard in main.py is: | ||
| if sys.argv[1] == "bench" and not showing_help | ||
| When showing_help is True, current_platform is never accessed for | ||
| the bench CPU-override, avoiding unnecessary platform detection. | ||
| """ | ||
| from vllm.engine.arg_utils import needs_help | ||
|
|
||
| # Verify the guard: needs_help() == True means "not showing_help" is False, | ||
| # so the bench platform-override block is skipped. | ||
| with patch.object(sys, "argv", ["vllm", "bench", "--help"]): | ||
| assert needs_help(), "needs_help() should be True for bench --help" | ||
|
|
||
| # Without --help the guard would be entered | ||
| with patch.object(sys, "argv", ["vllm", "bench", "latency"]): | ||
| assert not needs_help(), "needs_help() should be False without --help" | ||
|
Comment on lines
+49
to
+66
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. This duplicates what is already done in the previous 2 tests? |
||
|
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. This test is strangely specific. Surely we just want to check that there's no |
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,33 @@ | ||
| # SPDX-License-Identifier: Apache-2.0 | ||
| # SPDX-FileCopyrightText: Copyright contributors to the vLLM project | ||
| """Test that get_max_tokens lazily imports current_platform | ||
| rather than relying on a module-level import.""" | ||
|
|
||
| import subprocess | ||
| import sys | ||
|
|
||
|
|
||
| def test_get_max_tokens_lazy_platform_import(): | ||
| """current_platform should not be imported at module level in utils.py.""" | ||
| result = subprocess.run( | ||
| [ | ||
| sys.executable, | ||
| "-c", | ||
| ( | ||
| "import ast, inspect, textwrap; " | ||
| "from vllm.entrypoints import utils; " | ||
| "src = inspect.getsource(utils.get_max_tokens); " | ||
| "tree = ast.parse(textwrap.dedent(src)); " | ||
| "imports = [n for n in ast.walk(tree) " | ||
| " if isinstance(n, ast.ImportFrom) " | ||
| " and n.module == 'vllm.platforms']; " | ||
| "assert imports, 'get_max_tokens should have a local platform import'" | ||
| ), | ||
| ], | ||
| capture_output=True, | ||
| text=True, | ||
| ) | ||
| assert result.returncode == 0, ( | ||
| f"Subprocess failed (rc={result.returncode}):\n" | ||
| f"stdout: {result.stdout}\nstderr: {result.stderr}" | ||
| ) |
Uh oh!
There was an error while loading. Please reload this page.