Repository navigation
[CI/Build] Add per-model reliability cases - #9109
Conversation
- Renamed reliability test files to remove the "test_reliability_" prefix, simplifying their names to "test_<model>.py". - Updated source file dependencies in CI configurations to reflect the new file names. - Added new reliability test for MiniCPM-o 4.5 and HunyuanImage, including various fault scenarios. - Enhanced documentation to clarify the structure and usage of reliability tests. This change improves consistency and clarity in the test suite structure. Signed-off-by: wangyu <410167048@qq.com>
- Modified reliability test file paths in the AMD pipeline configuration to point to the new locations of the test scripts. - Enhanced type checking by importing `OmniServerParams` conditionally based on `TYPE_CHECKING`. - Updated the source path in the helper test for `qwen3_omni` to reflect the new file structure. These changes enhance the organization of the test suite and improve code clarity. Signed-off-by: [Your Name] <your.email@example.com> Signed-off-by: wangyu <410167048@qq.com>
- Replaced the regex-based device parsing in `parse_stage_devices` with a new implementation using `stage_config_device_spec` for improved clarity and maintainability. - Introduced helper functions in `helpers.py` to streamline device extraction from stage configurations. - Simplified assertions in `_assert_text_200` methods across multiple test files to focus on response handling without explicit status checks. - Added a new utility function to handle default HTTP 200 assertions in error scenarios. These changes enhance the robustness and readability of the test suite while ensuring accurate device configuration handling. Signed-off-by: [Your Name] <your.email@example.com> Signed-off-by: wangyu <410167048@qq.com>
Codex Review SummaryThis comment shows the latest Codex review activity on this pull request.
ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
Codex reacts with 👀 while any review is running, comments if it has suggestions, and reacts with 👍 once all reviews finish with no findings. |
|
Codex usage limits have been reached for code reviews. Please check with the admins of this repo to increase the limits by adding credits. |
|
This PR appears to be related to model: minicpm. Model owners: @y-null @hsliuustc0106 Routing: @y-null via model owner; @hsliuustc0106 via CODEOWNERS @yenuo26, please review your own changes and leave a short self-review comment describing what you checked. PRs without author self-review may not be assigned a reviewer. Please take a look when you have a chance. If you would like an automated review, mention @vllm-omni-review-bot in a comment. |
hsliuustc0106
left a comment
There was a problem hiding this comment.
TODO: preserve worker-kill fast-fail coverage
-
P2 — restore the mandatory HTTP 500 probe after a no-load worker kill. @yenuo26, at head
1b1a61abd35a8870568bd05675f0e5e3b1321f89, the new scenario goes directly from killing the worker to the concurrent checker. This drops the old Qwen probe requiring HTTP 500. The new checker accepts matching exceptions; Qwen accepts timeout errors, with a 20-second HTTP timeout inside a 30-second convergence budget. Thus requests hanging until timeout, followed by/healthreturning 503, can now pass instead of exposing a fast-fail regression.Suggested fix: call the existing
model.assert_fast_fail(client, server)after_kill_workerand before the concurrent check. Acceptance: a focused regression rejects timeout-only chat failures with health 503, while bounded HTTP 500 responses pass. Please include the fix SHA and test command/environment/result.
Coverage: source-only review of all 31 changed files and related model/scenario/helper paths. No PR code or hardware tests executed by this review. Current GitHub checks pass, but no current-head CUDA L2 result was visible; this is not an approval.
|
@amy-why-3459 PTAL |
…nario - Revised docstrings in multiple test files to clarify the expected behavior after a quiet worker kill, specifying that the next request returns HTTP 500 followed by concurrent request failures. - Enhanced the docstring in the `ProcessKillWorkerNoLoad` class to provide detailed information about the test flow and assertions. These changes improve the clarity and accuracy of the reliability tests related to worker process failures. Signed-off-by: wangyu <410167048@qq.com>
amy-why-3459
left a comment
There was a problem hiding this comment.
Reviewed the FaultModel / scenario split (helpers.py, scenarios/), the four model files against the removed per-model suites, the tests/helpers client/runtime changes, and the CI dependency wiring. The Qwen signal/skip matrix is preserved (serve-root SIGKILL still skipped for vllm#43060, tree-with-load SIGTERM for #8912).
+1 on @hsliuustc0106's note about the dropped HTTP 500 probe after the no-load worker kill. The same probe was also dropped for HunyuanImage: the old suite called _assert_post_fault_image_fast_fail before the concurrent check, and the new HunyuanImage.assert_fast_fail is never reached from ProcessKillWorkerNoLoad. Calling model.assert_fast_fail in the scenario would cover both models.
A few more points inline.
| Other platform blocks stay out of the result. A CUDA run of a file that | ||
| also lists an XPU 8-GPU override keeps the base stage devices. | ||
| """ | ||
| raw = yaml.safe_load(Path(stage_config_path).read_text(encoding="utf-8")) |
There was a problem hiding this comment.
This re-implements deploy resolution with a raw yaml.safe_load, so base_config inheritance is skipped. 35 files under vllm_omni/deploy/ use base_config (e.g. minicpmo_4_5_turn.yaml, qwen3_omni_duplex.yaml); if a reliability suite points at one of those, the OOM hog either raises No runtime.devices found or only covers the overlay's stages. The server itself resolves base_config via resolve_deploy_yaml() and then applies _apply_platform_overrides() in vllm_omni/config/stage_config.py (same device_name.lower() key). Could this reuse that path so the hog targets exactly the devices the server uses?
Also, I don't see a CPU test for this function in test_helpers.py. A small one (base only, platforms.npu override, platforms.xpu ignored on CUDA, platform=None) would lock in the Hunyuan 0,1,2,3 vs 0..7 fix described in the PR.
There was a problem hiding this comment.
stage_config_device_spec now uses the server loader. load_deploy_config goes through resolve_deploy_yaml, so base_config is inherited, and a named platform is applied with _apply_platform_overrides using that same device_name key. platform=None keeps the inherited base stages and does not merge other platforms.* blocks.
test_stage_config_device_spec_uses_inherited_platform_devices covers a base-only file, an NPU override, CUDA ignoring an XPU 8-GPU list, an overlay that only sets base_config, and Hunyuan (0,1,2,3 vs 0..7). The test imports that loader, so it runs where vLLM is installed.
On 08fbdc959.
| return status if isinstance(status, int) else None | ||
|
|
||
|
|
||
| def _status_from_default_http_200_assertion(exc: BaseException) -> int | None: |
There was a problem hiding this comment.
Recovering the status by parsing the assertion text ties this scenario to the exact wording in send_chat_completions_http_request / send_images_generations_http_request. If either message changes, a plain 500 falls through to assert_fault_exception(exc, process_kill_error_keywords) and passes or fails based on keywords rather than the status. HunyuanImage relies on this path entirely, since it inherits the default submit_concurrent -> warmup, which asserts 200.
Since the concurrent probe expects failure anyway, could submit_concurrent pass the accepted failure codes explicitly (e.g. err_code=(500, 503)) and return the response, so the status comes from status_code?
There was a problem hiding this comment.
Removed the assertion-text parser. Qwen and Hunyuan submit_concurrent now pass err_code=(500, 503) and return the response, and the checker reads status_code. Hunyuan no longer inherits submit_concurrent -> warmup.
An AssertionError from a code mismatch is re-raised. A 200 is not treated as a failure just because the assertion text contains 500. Connection and timeout errors still go through assert_fault_exception.
On 08fbdc959.
|
|
||
| def _assert_concurrent_all_fail(model: FaultModel, client: Any, server: Any) -> None: | ||
| start = time.monotonic() | ||
| with concurrent.futures.ThreadPoolExecutor(max_workers=model.concurrent_workers) as executor: |
There was a problem hiding this comment.
concurrent.futures.wait(..., timeout=...) returns on time, but leaving the with block calls shutdown(wait=True), which waits for the hung requests to finish. So elapsed (line 77) and the pending assertion only run after the per-request HTTP timeout. For HunyuanImage the default submit_concurrent -> warmup uses timeout: 600, so a hang surfaces after ~10 min instead of at concurrent_timeout_sec=40. Evaluating pending/elapsed inside the block and then calling executor.shutdown(wait=False, cancel_futures=True) would fail fast. The old per-model code had the same shape, but it's now shared by every model.
There was a problem hiding this comment.
pending and elapsed are checked inside the executor block, then the pool is closed with shutdown(wait=False, cancel_futures=True). The with form was calling shutdown(wait=True) and waiting out each request's HTTP timeout before those asserts ran.
Hunyuan's concurrent probe also no longer uses warmup's 600s timeout. It posts /v1/images/generations with a 30s timeout, inside the 40s convergence budget.
On 08fbdc959.
| return _RELIABILITY_SERVER_LOG_DIR | ||
|
|
||
|
|
||
| def reliability_params_with_server_log(params: Sequence[Any], stem: str) -> list[Any]: |
There was a problem hiding this comment.
For the lifecycle server set, serve stdout/stderr now goes to a temp file and only the last 120 lines are echoed in __exit__. Nothing in this PR reads the file (the docstring mentions a normal-exit test asserting on it, but there's no such test here). For PartialStageIsolation, the stage-1 SIGKILL traceback and orchestrator errors happen well before the SIGTERM shutdown logs, so they can fall outside the tail and disappear from the Buildkite output. Maybe keep stdout inherited until a test consumes the log, or dump the full file when the test fails.
There was a problem hiding this comment.
The lifecycle server set no longer sets log_path. Serve stdout/stderr stay inherited, so the stage-1 SIGKILL traceback is in the Buildkite log as it happens instead of being dropped from a 120-line tail. Nothing in this PR reads that file, and there is no normal-exit test here.
If a later test does pass log_path, __exit__ prints the whole file rather than the last 120 lines.
On 08fbdc959.
| - tests/dfx/reliability/scenarios/ | ||
| - tests/dfx/reliability/helpers.py | ||
| - tests/dfx/reliability/conftest.py | ||
| - tests/helpers/runtime.py |
There was a problem hiding this comment.
The scenarios now depend on tests/dfx/conftest.py (assert_fault_exception, resolve_oom_device_spec, parse_stage_devices, create_reliability_omni_server_params), and this PR changes parse_stage_devices there. That file isn't in any of the four *_reliability keys or in source_filter_fallback (it's only under omni_qwen3_omni_duplex_perf), so a change limited to it won't select these jobs. Suggest adding tests/dfx/conftest.py next to tests/dfx/reliability/helpers.py in all four keys, including the new omni_minicpmo_4_5_reliability.
There was a problem hiding this comment.
Added tests/dfx/conftest.py next to tests/dfx/reliability/helpers.py on all four keys: omni_qwen3_omni_reliability, omni_minicpmo_4_5_reliability, diffusion_wan22_reliability, and diffusion_hunyuan_image3_reliability.
On 08fbdc959.
- Added `tests/dfx/conftest.py` to source file dependencies in CI configurations for improved test organization. - Refactored device specification handling in `helpers.py` to streamline device extraction from stage configurations. - Updated assertions in reliability tests to ensure accurate handling of concurrent request failures and improved clarity in test scenarios. These changes enhance the maintainability and robustness of the reliability test suite. Signed-off-by: [Your Name] <your.email@example.com> Signed-off-by: wangyu <410167048@qq.com>
amy-why-3459
left a comment
There was a problem hiding this comment.
Re-checked 1b1a61ab..08fbdc95. The earlier points look addressed: stage_config_device_spec now goes through load_deploy_config + _apply_platform_overrides with the same device_name.lower() key the server uses (and platform=None correctly skips the overlay instead of falling back to current_omni_platform), the assertion-text parsing is gone in favor of err_code=(500, 503), the pool now shuts down with wait=False so pending/elapsed fail within concurrent_timeout_sec, ProcessKillWorkerNoLoad runs assert_fast_fail before the concurrent check, and tests/dfx/conftest.py is in all four reliability keys.
One small leftover inline.
| init_timeout: int | None = None | ||
| stage_init_timeout: int | None = None # None: fixture supplies default (600 s) | ||
| startup_timeout: int = SERVER_STARTUP_TIMEOUT_S | ||
| log_path: str | None = None |
There was a problem hiding this comment.
With reliability_params_with_server_log removed, nothing in this PR sets log_path anymore, so this field, the OmniServer(log_path=...) kwarg, the stdout/stderr redirect in _start_server, and _close_server_log are now unused additions to the shared test harness. Could they be dropped here and come back together with the test that actually reads the log? If they stay, note that _close_server_log now prints the entire file on every __exit__ (not only on failure) and never removes it, which can get large for multi-stage serve logs.
There was a problem hiding this comment.
Agreed. After reliability_params_with_server_log was removed, nothing in this PR sets log_path, so OmniServerParams.log_path, the OmniServer(log_path=...) argument, the stdout/stderr redirect in _start_server, and _close_server_log are unused additions to the shared harness. Serve stdout/stderr for the lifecycle set stay inherited.
I'll drop those pieces on this branch and bring them back together with the test that reads the log. Leaving them also means __exit__ prints the whole file on every exit and never deletes it, which is the wrong default for a multi-stage serve log.
There was a problem hiding this comment.
Dropped on f646cf83d. OmniServerParams.log_path, the OmniServer(log_path=...) argument, the stdout/stderr redirect in _start_server, and _close_server_log are gone. Serve stdout/stderr stay inherited. The stage CLI's own per-stage logs are unchanged. This can come back together with the test that reads the file.
- Removed the `log_path` parameter from `OmniServerParams` and `OmniServer` classes to streamline server initialization. - Eliminated server log handling code, including the `_close_server_log` method, to simplify the server's exit process. - Updated related code sections to reflect the removal of logging functionality, enhancing code clarity and maintainability. These changes contribute to a cleaner implementation of the OmniServer class by focusing on essential parameters and reducing complexity. Signed-off-by: [Your Name] <your.email@example.com> Signed-off-by: wangyu <410167048@qq.com>
|
@amy-why-3459 PTAL |
amy-why-3459
left a comment
There was a problem hiding this comment.
Re-checked 08fbdc95..95b9fc26, leaving out the merge from main. f646cf83 removes the now-unused log_path plumbing in tests/helpers/runtime.py that I mentioned last round. Nothing further on this delta.
Signed-off-by: wangyu <410167048@qq.com> Signed-off-by: [Your Name] <your.email@example.com>
The test-mark pre-commit hook (vllm-project#8380, vllm-project#9109) rejects combining the module-level pytest.mark.cpu with hardware_test()'s accelerator marks on the CUDA cases; keep the module mark at core_model and attach pytest.mark.cpu per CPU case instead.
The test-mark pre-commit hook (vllm-project#8380, vllm-project#9109) rejects combining the module-level pytest.mark.cpu with hardware_test()'s accelerator marks on the CUDA cases; keep the module mark at core_model and attach pytest.mark.cpu per CPU case instead. Signed-off-by: yanbao1217 <yanliuwebsite493@gmail.com>

PLEASE FILL IN THE PR DESCRIPTION HERE.
Contributor intent (required human answers)
All four questions must be answered by the human contributor before submission.
An agent may reuse explicit human answers with a public-safe source reference;
it must not invent, infer, or prefill missing answers. If an answer is missing
or materially ambiguous, ask the human and wait. Do not publish private
conversation content without authorization.
1. What outcome do you want to achieve?
Add reliability test cases.
2. What is the highest-leverage variable for that outcome?
The reliability test cases.
3. How will this work move that variable?
Add fault scenarios and models to the reliability cases.
4. Whose customer or user needs does this address, and what are those needs?
Model quality. Reliability cases need to cover the models and fault scenarios that protect it.
Purpose
Weekly reliability tests were split across
test_reliability_<model>.pyfiles and shared fault scripts. They are now one test file per model, with the fault steps intests/dfx/reliability/scenarios/and the shared contract inFaultModel/FaultScenario.slow and cards_N, matching each file's hardware marks. AMD weekly stays on ROCm MI325 with the same card counts. MiniCPM is CUDA-only.send_chat_completions_http_requestnow requires HTTP 200 when neithererr_codenorerr_messageis set. Process-kill still treats a returned or asserted status of 400 or higher as a failed request.0,1,2,3fromhunyuan_image3_dit.yaml. The XPU list0,1,2,3,4,5,6,7is no longer merged in. If a later sidecar fails to start, sidecars that already started are stopped.docs/contributing/ci/anddocs/user_guide/fault_injection_reliability_matrix.mdpoint attests/dfx/reliability/test_<model>.py. No user-facing inference API or deploy default changed.Test Plan
vLLM Version: not installed in the local environment used for these checks
vLLM-Omni Commit:
1b1a61abd35a8870568bd05675f0e5e3b1321f89Test Result
BEFORE SUBMITTING: read CONTRIBUTING.md and run the precheck-pr skill with the code agent for a self-check against project conventions.
Note
To avoid agent spam, vLLM-Omni limits contributors without write access to 5 open PRs. If this blocks well-intentioned critical work, please contact a committer to request bypass list access.
(anything written below this line will be removed by GitHub Actions)