[CI ][Misc] Add timeout check for custom op CI and optimize test parameters - #8755
Conversation
Signed-off-by: ZT-AIA <1028681969@qq.com>
Summary of ChangesHello, I'm Gemini Code Assist1! I'm currently reviewing this pull request and will post my feedback shortly. In the meantime, here's a summary to help you and other reviewers quickly get up to speed! This pull request improves the stability of the CI pipeline by implementing a timeout check for custom operations. It adds logic to track slow test cases and skip them if they exceed defined limits, while also reducing the test matrix for specific fused operations to ensure faster and more reliable test runs. Highlights
🧠 New Feature in Public Preview: You can now enable Memory to help Gemini Code Assist learn from your team's feedback. This makes future code reviews more consistent and personalized to your project's style. Click here to enable Memory in your admin console. Using Gemini Code AssistThe full guide for Gemini Code Assist can be found on our documentation page, here are some quick tips. Invoking Gemini You can request assistance from Gemini at any point by creating a comment using either
Customization To customize Gemini Code Assist for GitHub experience, repository maintainers can create a configuration file and/or provide a custom code review style guide (such as PEP-8 for Python) by creating and adding files to a Limitations & Feedback Gemini Code Assist may make mistakes. Please leave feedback on any instances where its feedback is incorrect or counter productive. You can react with 👍 and 👎 on @gemini-code-assist comments. If you're interested in giving your feedback about your experience with Gemini Code Assist for GitHub and other Google products, sign up here. Footnotes
|
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request introduces a timeout mechanism in conftest.py to skip remaining tests in a file once a specific number of tests exceed a duration threshold, and it optimizes CI runtime by reducing the parameter space for test_fused_qkvzba_split_reshape_cat.py. Feedback indicates that the PR title and summary must be updated to comply with the repository's style guide. Additionally, the logic for skipping tests should be improved by checking the per-file slow case count directly instead of relying on a single global variable, which could be overwritten when multiple files are processed.
| from datetime import datetime | ||
| import pytest | ||
|
|
||
| DURATION_THRESHOLD = 120 |
There was a problem hiding this comment.
The Pull Request title and summary do not adhere to the repository's style guide regarding format and required sections.
Suggested PR Title:
[CI][Misc] Add timeout check for custom op CI and optimize test parametersSuggested PR Summary:
### What this PR does / why we need it?
This PR introduces a mechanism to track test duration in `conftest.py` and skip subsequent tests in a file if a certain number of tests exceed a timeout threshold. This is intended to prevent CI hangs or long-running nightly tests. Additionally, it reduces the parameter space for `test_fused_qkvzba_split_reshape_cat.py` to further optimize CI runtime.
### Does this PR introduce _any_ user-facing change?
no
### How was this patch tested?
nightlyReferences
- The PR title and summary must follow the specific format defined in the Repository Style Guide, including the [Branch][Module][Action] prefix for the title and specific headers for the summary. (link)
| _per_file_slow_cases = {} | ||
| _current_file = None | ||
|
|
||
|
|
||
| def pytest_runtest_setup(item): | ||
| item.start_time = time.time() | ||
|
|
||
|
|
||
| def pytest_runtest_teardown(item, nextitem): | ||
| global _current_file | ||
|
|
||
| file_path = item.fspath | ||
| duration = time.time() - item.start_time | ||
|
|
||
|
|
||
| if file_path not in _per_file_slow_cases: | ||
| _per_file_slow_cases[file_path] = 0 | ||
|
|
||
| if duration > DURATION_THRESHOLD: | ||
| _per_file_slow_cases[file_path] += 1 | ||
| cnt = _per_file_slow_cases[file_path] | ||
| print(f" Detected that the test case took too long, ({cnt}/{SLOW_COUNT_LIMIT}):{duration:.2f}s") | ||
|
|
||
| if cnt >= SLOW_COUNT_LIMIT: | ||
| print(f"\n The number of timeout test cases {file_path} ≥{SLOW_COUNT_LIMIT}\n") | ||
| _current_file = file_path | ||
|
|
||
|
|
||
| def pytest_runtest_call(item): | ||
| if _current_file == item.fspath: | ||
| print(f"CASE SKIP:{item.nodeid}") | ||
| pytest.skip(f"The use case takes too long.") |
There was a problem hiding this comment.
The current implementation uses a single global variable _current_file to track which file should be skipped. This logic is flawed if multiple test files are executed: once a new file exceeds the SLOW_COUNT_LIMIT, _current_file is overwritten, and any remaining tests in previously "timed out" files will no longer be skipped.
I suggest using the _per_file_slow_cases dictionary directly to check the skip condition for each file, which is more robust and removes the need for the _current_file global variable.
_per_file_slow_cases = {}
def pytest_runtest_setup(item):
item.start_time = time.time()
def pytest_runtest_teardown(item, nextitem):
file_path = item.fspath
duration = time.time() - item.start_time
if duration > DURATION_THRESHOLD:
_per_file_slow_cases[file_path] = _per_file_slow_cases.get(file_path, 0) + 1
cnt = _per_file_slow_cases[file_path]
print(f" Detected that the test case took too long, ({cnt}/{SLOW_COUNT_LIMIT}):{duration:.2f}s")
if cnt >= SLOW_COUNT_LIMIT:
print(f"\n The number of timeout test cases {file_path} ≥{SLOW_COUNT_LIMIT}\n")
def pytest_runtest_call(item):
if _per_file_slow_cases.get(item.fspath, 0) >= SLOW_COUNT_LIMIT:
print(f"CASE SKIP:{item.nodeid}")
pytest.skip("The use case takes too long.")
ZT-AIA
changed the title
0cc7686
into
vllm-project:releases/v0.18.0
…meters (vllm-project#8755) ### What this PR does / why we need it? This PR introduces a mechanism to track test duration in `conftest.py` and skip subsequent tests in a file if a certain number of tests exceed a timeout threshold. This is intended to prevent CI hangs or long-running nightly tests. Additionally, it reduces the parameter space for `test_fused_qkvzba_split_reshape_cat.py` to further optimize CI runtime. ### Does this PR introduce _any_ user-facing change? no ### How was this patch tested? nightly Signed-off-by: ZT-AIA <1028681969@qq.com>
2 participants
What this PR does / why we need it?
This PR introduces a mechanism to track test duration in
conftest.pyand skip subsequent tests in a file if a certain number of tests exceed a timeout threshold. This is intended to prevent CI hangs or long-running nightly tests. Additionally, it reduces the parameter space fortest_fused_qkvzba_split_reshape_cat.pyto further optimize CI runtime.Does this PR introduce any user-facing change?
no
How was this patch tested?
nightly