Skip to content

[CI] Add timeouts to tests#24260

Merged
simon-mo merged 11 commits intovllm-project:mainfrom
rafvasq:add-timeouts
Sep 6, 2025
Merged

[CI] Add timeouts to tests#24260
simon-mo merged 11 commits intovllm-project:mainfrom
rafvasq:add-timeouts

Conversation

@rafvasq
Copy link
Contributor

@rafvasq rafvasq commented Sep 4, 2025

Purpose

cc: @njhill

Signed-off-by: Rafael Vasquez <rafvasq21@gmail.com>
@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
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.

Signed-off-by: Rafael Vasquez <rafvasq21@gmail.com>
@rafvasq rafvasq requested a review from njhill September 5, 2025 18:55
Signed-off-by: Nick Hill <nhill@redhat.com>
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

Signed-off-by: Nick Hill <nhill@redhat.com>
@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
Signed-off-by: Rafael Vasquez <rafvasq21@gmail.com>
Signed-off-by: Nick Hill <nhill@redhat.com>
Co-authored-by: Nick Hill <nhill@redhat.com>
skyloevil pushed a commit to skyloevil/vllm that referenced this pull request Sep 13, 2025
Signed-off-by: Rafael Vasquez <rafvasq21@gmail.com>
Signed-off-by: Nick Hill <nhill@redhat.com>
Co-authored-by: Nick Hill <nhill@redhat.com>
FeiDaLI pushed a commit to FeiDaLI/vllm that referenced this pull request Sep 25, 2025
Signed-off-by: Rafael Vasquez <rafvasq21@gmail.com>
Signed-off-by: Nick Hill <nhill@redhat.com>
Co-authored-by: Nick Hill <nhill@redhat.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

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

Projects

None yet

Development

Successfully merging this pull request may close these issues.

[CI]: Add timeouts to all tests

3 participants