Skip to content

Conversation

@angelayi
Copy link
Contributor

Attempting to fix test failure

@ProExpertProg ProExpertProg added the ready ONLY add when PR is ready to merge/full CI is needed label Nov 18, 2025
@ProExpertProg ProExpertProg marked this pull request as ready for review November 18, 2025 04:35
Copy link

@chatgpt-codex-connector chatgpt-codex-connector bot left a comment

Choose a reason for hiding this comment

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

💡 Codex Review

Here are some automated review suggestions for this pull request.

ℹ️ About Codex in GitHub

Codex has been enabled to automatically review pull requests in this repo. Reviews are triggered when you

  • Open a pull request for review
  • Mark a draft as ready
  • Comment "@codex review".

If Codex has suggestions, it will comment; otherwise it will react with 👍.

When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".

@ProExpertProg
Copy link
Collaborator

Seems like this works, can you add it to E2E tests as well? Or maybe better add a test util file that just calls this function, so that we can insert it into the test pipeline and not pollute the tests?

@angelayi angelayi force-pushed the angelayi/cleanup branch 2 times, most recently from 9770b6d to f7e0570 Compare November 19, 2025 00:14
Copy link
Collaborator

@ProExpertProg ProExpertProg left a comment

Choose a reason for hiding this comment

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

I like the envvar approach! I think you should take into account the CUDA_VISIBLE_DEVICES variable, as I think that these tests should only be running on 2xH200 at a time, and the CI failure implies that the fixture is waiting on all 8 GPUs on the node.

@mergify
Copy link

mergify bot commented Nov 19, 2025

This pull request has merge conflicts that must be resolved before it can be
merged. Please rebase the PR, @angelayi.

https://docs.github.com/en/pull-requests/collaborating-with-pull-requests/working-with-forks/syncing-a-fork

@mergify mergify bot added the needs-rebase label Nov 19, 2025
Signed-off-by: angelayi <yiangela7@gmail.com>
@ProExpertProg
Copy link
Collaborator

Okay this looks good now! We finally fixed the failure with e2e model tests. There's now a DBO failure which I assume snuck in from Lucas's refactor PR because this test failed before the DBO tests could run

@vllm-bot vllm-bot merged commit 4b17ce6 into vllm-project:main Nov 29, 2025
15 of 17 checks passed
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.

4 participants