[CI] Restructure vLLM-Omni Test Layout, Fixture Scope, and Support Modules#2620
[CI] Restructure vLLM-Omni Test Layout, Fixture Scope, and Support Modules#2620hsliuustc0106 merged 29 commits intovllm-project:mainfrom
Conversation
Signed-off-by: wangyu <410167048@qq.com>
…tions Signed-off-by: wangyu <410167048@qq.com>
|
Codex usage limits have been reached for code reviews. Please check with the admins of this repo to increase the limits by adding credits. |
Signed-off-by: wangyu <410167048@qq.com>
…, video, and image generation. Introduce parameters for cache directory and force regeneration, enhancing performance and usability. Remove deprecated save_to_file logic and improve error handling for media processing. Signed-off-by: wangyu <410167048@qq.com>
|
cc @Gaohan123 @tzhouam @princepride @lishunyang12 @linyueqian @ZeldaHuang @wtomin @SamitHuang PTAL for your tests ownership files |
| import pytest | ||
| import torch | ||
|
|
||
| from tests.helpers.env import _run_post_test_cleanup, _run_pre_test_cleanup |
There was a problem hiding this comment.
tests.helpers.env imports vllm.platforms and vllm_omni.platforms at module level, so loading this plugin at conftest time still pulls them in before default_env runs. Tbh this partially defeats the RFC #2299 goal — consider moving these imports inside clean_gpu_memory_between_tests so helpers.env only loads after session fixtures.
| print("=" * 80) | ||
|
|
||
|
|
||
| def _run_pre_test_cleanup(enable_force: bool = False) -> None: |
There was a problem hiding this comment.
These are in __all__ and imported from fixtures/env.py and helpers/runtime.py, so they're effectively public. Drop the leading underscore?
…orts of platform-specific modules until needed to ensure proper execution order of fixtures. Introduce a new function for forced GPU cleanup to streamline cleanup processes across different classes. Enhance memory monitoring logic for better clarity and performance during tests. Signed-off-by: wangyu <410167048@qq.com>
Signed-off-by: wangyu <410167048@qq.com>
…ate test markers for better categorization in zimage_parallelism tests. Enhance GPU memory cleanup messages for improved debugging during test execution. Signed-off-by: wangyu <410167048@qq.com>
…e count discrepancies in MP4 validation. Update docstring for clarity on expected behavior and adjust frame count assertion logic. Signed-off-by: wangyu <410167048@qq.com>
… helpers from the runtime module. This change improves code organization and maintainability by consolidating cleanup functions under a common namespace. Signed-off-by: wangyu <410167048@qq.com>
…hem to a new helpers module. This change improves code organization and reusability, while also adding functionality to compute and assert SSIM and PSNR metrics for model outputs. The previous utility functions have been removed from the utils module to streamline the codebase. Signed-off-by: wangyu <410167048@qq.com>
| if params.use_omni and params.stage_init_timeout is not None: | ||
| server_args = [*server_args, "--stage-init-timeout", str(params.stage_init_timeout)] | ||
| else: | ||
| server_args = [*server_args, "--stage-init-timeout", "600"] |
There was a problem hiding this comment.
This changes the non-omni path too: when use_omni=False, vllm_omni.entrypoints.cli.main forwards straight to upstream vllm_main() unless --omni is present, so --stage-init-timeout / --init-timeout are not recognized there. We still have use_omni=False coverage in tests/e2e/accuracy/conftest.py, so this will make those server launches fail. Can we keep these timeout args gated behind params.use_omni, like the old fixture did?
…ompatibility with non-omni paths. Timeout flags are now gated behind the use_omni parameter, aligning with legacy behavior and improving code clarity. Signed-off-by: wangyu <410167048@qq.com>
…ding Signed-off-by: wangyu <410167048@qq.com>
Signed-off-by: wangyu <410167048@qq.com>
…ameter mapping functions Signed-off-by: wangyu <410167048@qq.com>
hsliuustc0106
left a comment
There was a problem hiding this comment.
Solid refactor overall. The modular split and media caching are clear improvements. A few issues to address.
| # Marker for Buildkite log folding before pytest summary lines. | ||
| terminalreporter.write_sep("-", "Result Summary") | ||
|
|
||
|
|
There was a problem hiding this comment.
This eager import of tests.helpers.assertions pulls in transformers.pipeline at conftest load time (see assertions.py:12). The PR description says the root conftest should avoid "premature loading of heavy dependencies" — this defeats that goal. Either defer these re-exports via __getattr__ (like the runtime exports below), or move the transformers.pipeline import inside _load_gender_pipeline() in assertions.py.
|
|
||
| import numpy as np | ||
| import soundfile as sf | ||
| from PIL import Image |
There was a problem hiding this comment.
from transformers import pipeline at module level means any import of tests.helpers.assertions (including from tests.conftest) triggers a heavy transformers load. Move this into _load_gender_pipeline() — it's only used there and already guarded by a lazy singleton pattern.
| "H100: Tests that require H100 GPU", | ||
| "L4: Tests that require L4 GPU", | ||
| "B60: Tests that require B60", | ||
| "MI325: Tests that require MI325 GPU (AMD/ROCm)", |
There was a problem hiding this comment.
Duplicate B60 marker entry. Line 197 adds "B60: Tests that require B60" (no description) and line 199 already has "B60: Tests that require Intel Arc Pro B60 XPU". pytest may silently accept duplicates, but this is confusing. Remove the one added here.
| env_dict=params.env_dict, | ||
| use_omni=params.use_omni, | ||
| ) | ||
| if port |
There was a problem hiding this comment.
The with OmniServer(...) if port else OmniServer(...) pattern duplicates the entire constructor call just to optionally pass port. Simplify to:
kwargs = dict(model=model, server_args=server_args, env_dict=params.env_dict, use_omni=params.use_omni)
if port:
kwargs["port"] = port
with OmniServer(**kwargs) as server:…rmance Signed-off-by: wangyu <410167048@qq.com>
Signed-off-by: wangyu <410167048@qq.com>
Signed-off-by: wangyu <410167048@qq.com>
… advanced model Signed-off-by: wangyu <410167048@qq.com>
Signed-off-by: wangyu <410167048@qq.com>
- Removed unused export `dummy_messages_from_mix_data` from `_STAGE_CONFIG_EXPORT_NAMES`. - Added `dummy_messages_from_mix_data` to the imports in multiple test files for consistency. - Adjusted the `_REPO_ROOT` path comment in `stage_config.py` for clarity. Signed-off-by: wangyu <410167048@qq.com>
Signed-off-by: wangyu <410167048@qq.com>
…dules (vllm-project#2620) Signed-off-by: wangyu <410167048@qq.com>
PLEASE FILL IN THE PR DESCRIPTION HERE ENSURING ALL CHECKLIST ITEMS (AT THE BOTTOM) HAVE BEEN CONSIDERED.
Purpose
due to #2299
Background and Goals
The original root directory tests/conftest.py centralized a large number of fixtures, assertions, media, and runtime logic, which could easily lead to:
This PR aims to thin the entry point, move implementations outward, modularize fixtures as plugins, and clarify import path instructions in the documentation.
Overview of Main Changes
conftest.pypytest_pluginsregistration, backward-compatible re-exports fromtests.helpers.*, and lazy loading of runtime symbols via__getattr__to avoid immediately importingtests.helpers.runtimewhenconftestloads.2)Add '--- Running Summary' before the pytest summary to create a foldable group in Buildkite.
tests/helpers/__init__.pydeliberately avoids star-import aggregation to prevent altering import order.2)Refactor media helper functions to support caching of synthetic audio, video, and image generation.
tests/helpers/fixtures/pytest_plugins: env, log, run_args, runtime; runtime fixtures then importOmniServer/OmniRunnerinternally, delaying heavy dependency initialization.tests/utils.pyhardware_test/hardware_marksforwarded totests.helpers.markconftestconftestfiles retained as needed intests/e2e/accuracy,tests/examples, etc., separating responsibilities from the root directory.docs/contributing/ci/updated to referencetests.helpers.mark, etc., consistent with the implementation.assertions/mediatools/pre_commit/check_pickle_imports.pyand others aligned with the new module layout.Impact on Contributors (Migration Notes)
New code: Prefer from tests.helpers.mark import hardware_test, hardware_marks; use tests.helpers.env for GPU/environment-related utilities. The re-exports in tests.conftest are only for transitional compatibility.
OmniRunner / OmniServer, etc.: Can still be lazily loaded from tests.conftest, but long-term recommendation is to switch to from tests.helpers.runtime import ..., aligning with the direction noted in the PR.
Test Plan
Test Result
1.ready in local
2.merge in local
success in merge

summary log

Essential Elements of an Effective PR Description Checklist
supported_models.mdandexamplesfor a new model. Please runmkdocs serveto sync the documentation editions to./docs.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)