Skip to content

[ROCm][CI/Build] Fix the pytest hook to properly print out the summary#38585

Merged
tjtanaa merged 2 commits intovllm-project:mainfrom
ROCm:fix_pytest_summary
Apr 3, 2026
Merged

[ROCm][CI/Build] Fix the pytest hook to properly print out the summary#38585
tjtanaa merged 2 commits intovllm-project:mainfrom
ROCm:fix_pytest_summary

Conversation

@gshtras
Copy link
Copy Markdown
Collaborator

@gshtras gshtras commented Mar 30, 2026

A follow up for #38252

Modifying the ROCm7.2.1 pytest workaround to properly finish including printing the summary, in addition to preserving the exit code

@gshtras gshtras requested a review from tjtanaa as a code owner March 30, 2026 20:57
Copy link
Copy Markdown

@claude claude Bot left a comment

Choose a reason for hiding this comment

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

Claude Code Review

This pull request is from a fork — automated review is disabled. A repository maintainer can comment @claude review to run a one-time review.

@mergify mergify Bot added ci/build rocm Related to AMD ROCm labels Mar 30, 2026
@github-project-automation github-project-automation Bot moved this to Todo in AMD Mar 30, 2026
Copy link
Copy Markdown
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 updates the Dockerfile.rocm to improve how pytest handles exits in CI by replacing a one-line conftest.py generation with a more detailed heredoc implementation. Feedback suggests ensuring I/O buffers are flushed before the immediate process exit, initializing the exit code to a non-zero value to avoid masking early failures, and correcting the imports.

Comment thread docker/Dockerfile.rocm
@AndreasKaratzas AndreasKaratzas added the ready ONLY add when PR is ready to merge/full CI is needed label Mar 30, 2026
Signed-off-by: Gregory Shtrasberg <Gregory.Shtrasberg@amd.com>
@gshtras gshtras force-pushed the fix_pytest_summary branch from d83d1f9 to 2ef61aa Compare March 31, 2026 15:54
Signed-off-by: Gregory Shtrasberg <Gregory.Shtrasberg@amd.com>
Copy link
Copy Markdown
Collaborator

@tjtanaa tjtanaa left a comment

Choose a reason for hiding this comment

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

LGTM

@tjtanaa tjtanaa merged commit a7d79fa into vllm-project:main Apr 3, 2026
13 checks passed
@github-project-automation github-project-automation Bot moved this from Todo to Done in AMD Apr 3, 2026
Comment thread docker/Dockerfile.rocm
# This is a workaround to ensure pytest exits with the correct status code in CI tests.
RUN echo "import os\n\ndef pytest_sessionfinish(session, exitstatus):\n os._exit(int(exitstatus))" > /vllm-workspace/conftest.py
RUN cat << 'EOF' > /vllm-workspace/conftest.py
import os
Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

it's missing the sys import too.

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

UPDATE: #38951

HenryTangDev pushed a commit to HenryTangMain/vllm that referenced this pull request Apr 6, 2026
vllm-project#38585)

Signed-off-by: Gregory Shtrasberg <Gregory.Shtrasberg@amd.com>
puririshi98 pushed a commit to puririshi98/vllm that referenced this pull request Apr 7, 2026
vllm-project#38585)

Signed-off-by: Gregory Shtrasberg <Gregory.Shtrasberg@amd.com>
Signed-off-by: Rishi Puri <riship@nvidia.com>
mtparet pushed a commit to blackfuel-ai/vllm that referenced this pull request Apr 9, 2026
vllm-project#38585)

Signed-off-by: Gregory Shtrasberg <Gregory.Shtrasberg@amd.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 rocm Related to AMD ROCm

Projects

Status: Done

Development

Successfully merging this pull request may close these issues.

3 participants