Skip to content

[CI] Add timeouts to tests #24260

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.

Sign up for GitHub

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

Jump to bottom
Merged
merged 11 commits into from
Sep 6, 2025
Merged

[CI] Add timeouts to tests #24260

merged 11 commits into from
Sep 6, 2025
+102 −44

Conversation

rafvasq
Copy link
Contributor

@rafvasq rafvasq commented Sep 4, 2025 •
edited by github-actions bot
Loading

Purpose

cc: @njhill

@rafvasq rafvasq changed the title First pass adding timeouts based on previous times [CI] Add timeouts to tests Sep 4, 2025
@mergify mergify bot added the ci/build label Sep 4, 2025
gemini-code-assist[bot]
gemini-code-assist bot reviewed Sep 4, 2025
View reviewed changes
Copy link
Contributor

@gemini-code-assist gemini-code-assist bot left a 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 introduces timeouts to test jobs to prevent them from hanging, which is a valuable improvement for CI stability. My review focuses on ensuring these timeouts are correctly implemented. I've identified several instances where PYTEST_ADDOPTS is used for test steps that don't run pytest, rendering the timeout ineffective. I have provided suggestions to correct this by using the timeout command for the respective shell commands.

@njhill
Copy link
Member

njhill commented Sep 5, 2025

Thanks @rafvasq this looks great. Just a couple of comments:

  • Some of the buffers compared to observed times look a bit small, e.g. 10min -> 12min, could we add a bit more on... say at least 30% and a minimum of 10mins above the observed time?
  • There are some PRs to reorganize the tests that we'll hopefully merge imminently (such as [CI] Optimize entrypoints API server tests #23896), we may need to do another pass of this after that.

njhill
njhill reviewed Sep 5, 2025
View reviewed changes
@rafvasq
Apply 30p formula for timeouts
a104d3b 
Signed-off-by: Rafael Vasquez <[email protected]>
@rafvasq rafvasq requested a review from njhill September 5, 2025 18:55
@njhill
change to round numbers
9a5418f 
Signed-off-by: Nick Hill <[email protected]>
njhill
njhill approved these changes Sep 5, 2025
View reviewed changes
Copy link
Member

@njhill njhill left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks @rafvasq !

@njhill njhill added the ready ONLY add when PR is ready to merge/full CI is needed label Sep 5, 2025
@njhill
Copy link
Member

njhill commented Sep 5, 2025

Failures are broken on main: https://buildkite.com/vllm/ci/builds/29600#01991bbb-f4af-402b-9e94-85a8d905836a, https://buildkite.com/vllm/ci/builds/29529#01991808-f958-4ed1-a38a-f3e83990c4fd

@njhill njhill added ready-for-merge Indicate this PR is ready to be merged by the maintainers, used by reviewers without merge access. force-merge and removed ready-for-merge Indicate this PR is ready to be merged by the maintainers, used by reviewers without merge access. labels Sep 5, 2025
@simon-mo simon-mo merged commit c954c66 into vllm-project:main Sep 6, 2025
4 of 14 checks passed
@rafvasq rafvasq deleted the add-timeouts branch September 8, 2025 13:59
eicherseiji pushed a commit to eicherseiji/vllm that referenced this pull request Sep 9, 2025
@rafvasq @njhill @eicherseiji
[CI] Add timeouts to tests (vllm-project#24260)
394d607 
Signed-off-by: Rafael Vasquez <[email protected]>
Signed-off-by: Nick Hill <[email protected]>
Co-authored-by: Nick Hill <[email protected]>
skyloevil pushed a commit to skyloevil/vllm that referenced this pull request Sep 13, 2025
@rafvasq @njhill @skyloevil
[CI] Add timeouts to tests (vllm-project#24260)
290d183 
Signed-off-by: Rafael Vasquez <[email protected]>
Signed-off-by: Nick Hill <[email protected]>
Co-authored-by: Nick Hill <[email protected]>
FeiDaLI pushed a commit to FeiDaLI/vllm that referenced this pull request Sep 25, 2025
@rafvasq @njhill @FeiDaLI
[CI] Add timeouts to tests (vllm-project#24260)
2d15b36 
Signed-off-by: Rafael Vasquez <[email protected]>
Signed-off-by: Nick Hill <[email protected]>
Co-authored-by: Nick Hill <[email protected]>
xuebwang-amd pushed a commit to xuebwang-amd/vllm that referenced this pull request Oct 10, 2025
@rafvasq @njhill @xuebwang-amd
[CI] Add timeouts to tests (vllm-project#24260)
e9bdfb2 
Signed-off-by: Rafael Vasquez <[email protected]>
Signed-off-by: Nick Hill <[email protected]>
Co-authored-by: Nick Hill <[email protected]>
Signed-off-by: xuebwang-amd <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@njhill njhill njhill approved these changes

+1 more reviewer

@gemini-code-assist gemini-code-assist[bot] gemini-code-assist[bot] left review comments

Reviewers whose approvals may not affect merge requirements

Assignees

No one assigned

Labels

ci/build force-merge ready ONLY add when PR is ready to merge/full CI is needed

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

[CI]: Add timeouts to all tests

3 participants

@rafvasq @njhill @simon-mo