- 
          
 - 
                Notifications
    
You must be signed in to change notification settings  - Fork 11k
 
Cleanup basic and entrypoint test organisation #27747
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
Signed-off-by: Harry Mellor <19981378+hmellor@users.noreply.github.com>
Signed-off-by: Harry Mellor <19981378+hmellor@users.noreply.github.com>
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 focuses on reorganizing the test structure for better maintainability and efficiency. It involves moving unit tests into a dedicated directory, splitting a large test group into shards, and updating test pipeline configurations. The review identifies a critical issue related to potential test failures due to missing multi-GPU configurations.
| 
               | 
          ||
| 
               | 
          ||
| @pytest.mark.parametrize("tp_size", [1, 2]) | ||
| @pytest.mark.parametrize("backend", ["mp", "ray"]) | ||
| @create_new_process_for_each_test() | ||
| def test_collective_rpc(tp_size, backend, monkeypatch): | ||
| if torch.cuda.device_count() < tp_size: | ||
| pytest.skip(f"Not enough GPUs for tensor parallelism {tp_size}") | 
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 test_collective_rpc test is marked as a multi-GPU test, but the pytestmark is defined after the imports. This could lead to tests being run without the multi-gpu setup if pytest runs before the mark is applied. It's better to define pytestmark at the top of the file to ensure it's always applied. This is a critical issue as it can lead to tests passing incorrectly when they should be failing due to insufficient GPU resources.
pytestmark = pytest.mark.multi_gpu_test(num_gpus=2)
from ...utils import create_new_process_for_each_test
@pytest.mark.parametrize("tp_size", [1, 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.
We don't do this anywhere else that we use pytestmark
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
Here are some automated review suggestions for this pull request.
ℹ️ 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".
| import pytest | ||
| import torch | ||
| 
               | 
          ||
| from vllm import LLM | ||
| 
               | 
          ||
| from ...utils import create_new_process_for_each_test | ||
| 
               | 
          ||
| pytestmark = pytest.mark.multi_gpu_test(num_gpus=2) | ||
| 
               | 
          ||
| 
               | 
          ||
| @pytest.mark.parametrize("tp_size", [1, 2]) | ||
| @pytest.mark.parametrize("backend", ["mp", "ray"]) | ||
| @create_new_process_for_each_test() | ||
| def test_collective_rpc(tp_size, backend, monkeypatch): | ||
| if torch.cuda.device_count() < tp_size: | ||
| pytest.skip(f"Not enough GPUs for tensor parallelism {tp_size}") | ||
| if tp_size == 1 and backend == "ray": | ||
| pytest.skip("Skip duplicate test case") | 
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.
  Removing device-count guard lets multi-GPU test run on single GPU
The test previously skipped when torch.cuda.device_count() < tp_size, but the new module-level pytestmark = pytest.mark.multi_gpu_test(num_gpus=2) does not perform that check—the multi_gpu_test skip logic lives in the decorator in tests/utils.py, not in a pytest marker. As a result, on environments with only one GPU this test will now run and attempt to create an LLM with tensor_parallel_size=2, causing a failure instead of a skip. To restore the behaviour, either keep the explicit device-count guard or apply the multi_gpu_test decorator (or its marks) so the skip happens before the LLM is constructed.
Useful? React with 👍 / 👎.
          
 This is mostly correct, except   | 
    
| 
           Thanks for the additional context. My reasoning for moving them there was that they were run in the  I'll move the ones that start the server into   | 
    
Signed-off-by: Harry Mellor <19981378+hmellor@users.noreply.github.com>
Signed-off-by: Harry Mellor <19981378+hmellor@users.noreply.github.com>
unitdirectory so we don't have to manually ignore everything elseopenai/tool_parsersinto theunittests because that's what they are