-
Notifications
You must be signed in to change notification settings - Fork 414
[Test] L2 & L3 Test Case Stratification Design for Omni Model #1272
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: wangyu31577 <[email protected]>
Signed-off-by: wangyu31577 <[email protected]>
Signed-off-by: wangyu31577 <[email protected]>
Signed-off-by: wangyu31577 <[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.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 66a1f1ef67
ℹ️ 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".
| path: /mnt/hf-cache | ||
| type: DirectoryOrCreate | ||
|
|
||
| # - label: "Bagel Text2Img Model Test with H100" |
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.
will this be included in PR-merge?
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.
Yes, I will contact the use case author to see how this use case can be split.
| from vllm.envs import VLLM_USE_MODELSCOPE | ||
| from vllm.multimodal.image import convert_image_mode | ||
|
|
||
| from tests.conftest import OmniRunner |
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.
why move it here?
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.
Because the original conftest only had the vllmrunner class, it seemed unnecessary to keep it as a separate file. Moreover, after merging it into the unified conftest, the functions for validating online use cases have been reused.
| @@ -8,13 +8,13 @@ | |||
| from vllm import SamplingParams | |||
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.
change file name to test_async_omni_engine_abort
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.
already rename
Signed-off-by: yenuo26 <[email protected]>
.buildkite/pipeline.yml
Outdated
| - export VLLM_TEST_CLEAN_GPU_MEMORY="1" | ||
| - pytest -s -v tests/e2e/offline_inference/test_qwen3_omni.py | ||
| - pytest -s -v tests/e2e/online_serving/test_qwen3_omni.py -m "core_model" --run-level "core_model" | ||
| - pytest -s -v tests/engine/test_abort.py |
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 line needs to change as well?
test_abort.py to test_async_omni_engine_abort.py
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.
yes, you're right, It has been modified.
…ons to use the new async engine abort test. Signed-off-by: yenuo26 <[email protected]>
|
@hsliuustc0106 @david6666666 please help add ready label |
Signed-off-by: yenuo26 <[email protected]>
…ci-qwen3 Signed-off-by: yenuo26 <[email protected]>
|
fix precommit & resolve conflicts |
Signed-off-by: yenuo26 <[email protected]>
Signed-off-by: yenuo26 <[email protected]>
Signed-off-by: yenuo26 <[email protected]>
fixed |
…fline inference tests. Updated pytest command to focus on specific tests and removed unnecessary import statements. Signed-off-by: yenuo26 <[email protected]>
…orker_type', and reduce max model length and batched tokens to 25000 for improved performance. Signed-off-by: yenuo26 <[email protected]>
…rt test, adjust synthetic video/image dimensions, and reduce max model length and batched tokens for Qwen2_5 Omni CI settings. Signed-off-by: yenuo26 <[email protected]>
Signed-off-by: wangyu <[email protected]>
Signed-off-by: yenuo26 <[email protected]>
This commit introduces a `kill_process_tree` function in `tests/conftest.py` to handle the termination of a process and its children, ensuring all processes are properly killed and verified. The existing `_kill_process_tree` method in the `OmniServer` class has been replaced with this new function for improved clarity and reusability. Additionally, the function is now utilized in the context manager exit methods of both `OmniServer` and `OmniRunner` classes to ensure proper cleanup of resources. Signed-off-by: yenuo26 <[email protected]>
This commit moves the `kill_process_tree` function into the `OmniServer` class as a private method `_kill_process_tree`, enhancing encapsulation. The method is now used in the context manager exit to ensure proper cleanup of resources. The previous standalone function has been removed to streamline the code. Signed-off-by: yenuo26 <[email protected]>
Signed-off-by: yenuo26 <[email protected]>
Signed-off-by: yenuo26 <[email protected]>
Signed-off-by: yenuo26 <[email protected]>
This commit adds a new private method `_cleanup_process` to the `OmniRunner` class, which iterates through running processes to terminate any related to "vllm" or "core". This method is called during the context manager exit to ensure proper resource cleanup. Signed-off-by: yenuo26 <[email protected]>
This commit modifies the `_cleanup_process` method in the `OmniRunner` class to remove the "vllm" keyword from the process termination logic, focusing solely on processes related to "core". This change streamlines the cleanup process during context manager exit. Signed-off-by: yenuo26 <[email protected]>
This commit modifies the `_cleanup_process` method in the `OmniRunner` class to change the process keyword from "core" to "enginecore". This adjustment refines the process filtering during cleanup operations. Signed-off-by: yenuo26 <[email protected]>
Signed-off-by: yenuo26 <[email protected]>
Signed-off-by: wangyu <[email protected]>
Signed-off-by: Hongsheng Liu <[email protected]>
|
@gcanlin @zhenwei-intel PTAL |
| @@ -13,8 +13,8 @@ stage_args: | |||
| model_arch: Qwen2_5OmniForConditionalGeneration | |||
| worker_type: ar | |||
| scheduler_cls: vllm_omni.core.sched.omni_ar_scheduler.OmniARScheduler | |||
| max_model_len: 896 | |||
| max_num_batched_tokens: 896 | |||
| max_model_len: 2400 | |||
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.
Why not also move rocm directory into tests/e2e/stage_configs/? we'd like to create the npu directory here as well.
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.
i will move it
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.
Copilot encountered an error and was unable to review this pull request. You can try again by re-requesting a review.
Signed-off-by: wangyu31577 <[email protected]>
Signed-off-by: wangyu31577 <[email protected]>
.buildkite/test-merge.yml
Outdated
| agents: | ||
| queue: "cpu_queue_premerge" | ||
|
|
||
| # - label: "Test on NPU" |
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.
rm these comments please
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.
Deleted
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.
Copilot encountered an error and was unable to review this pull request. You can try again by re-requesting a review.
Signed-off-by: wangyu31577 <[email protected]>
PLEASE FILL IN THE PR DESCRIPTION HERE ENSURING ALL CHECKLIST ITEMS (AT THE BOTTOM) HAVE BEEN CONSIDERED.
Purpose
L2 & L3 Test Case Stratification Design for Omni Model: refer to #1218
Related documentation can be found:#1167
The main changes are as follows:
1.Added test-merge.yaml to manage merge-level test suites in the future.
2.Standardized the existing Omni3 online use cases and integrated both L2 and L3 level execution logic into a single script, differentiated during execution via the --run-level parameter.
3.Removed the default configuration test scenarios from the original online use cases, retaining only the async_chunk scenario. Default configurations will be covered by offline use cases.
4.Removed the test_build_and_log_summary test case (a new UT case covering this logic will be submitted in #891) and migrated test_async_omni.py to the tests/engine directory.
Test Plan
1.run offline case
/workspace/.venv/bin/python -m pytest -sv tests/e2e/offline_inference/test_qwen3_omni.py --html=report.html --self-contained-html2.run online case
L2
/workspace/.venv/bin/python -m pytest -sv tests/e2e/online_serving/test_qwen3_omni.py -m core_model --run-level="core_model" --html=report.html --self-contained-html/workspace/.venv/bin/python -m pytest -sv tests/e2e/online_serving/test_qwen3_omni.py --html=report.html --self-contained-htmlL3
/workspace/.venv/bin/python -m pytest -sv tests/e2e/online_serving/test_qwen3_omni.py -m advanced_model --run-level="advanced_model" --html=report.html --self-contained-html3.run abort case
/workspace/vllm-omni# /workspace/.venv/bin/python -m pytest -sv tests/engine/test_abort.py --html=report.html --self-contained-htmlTest Result
1.offline case



2.online case
L2
L3

3.abort case

CI Result
Essential Elements of an Effective PR Description Checklist
supported_models.mdandexamplesfor a new model.BEFORE SUBMITTING, PLEASE READ https://github.com/vllm-project/vllm-omni/blob/main/CONTRIBUTING.md (anything written below this line will be removed by GitHub Actions)