Skip to content

Conversation

@jcyang43
Copy link
Contributor

@jcyang43 jcyang43 commented Sep 25, 2025

Purpose

Update TPU benchmark expected threshold for stable version of torch_xla

Test Result

CI


Essential Elements of an Effective PR Description Checklist
  • The purpose of the PR, such as "Fix some issue (link existing issues this PR will resolve)".
  • The test plan, such as providing test command.
  • The test results, such as pasting the results comparison before and after, or e2e results
  • (Optional) The necessary documentation update, such as updating supported_models.md and examples for a new model.
  • (Optional) Release notes update. If your change is user facing, please update the release notes draft in the Google Doc.

@mergify mergify bot added ci/build tpu Related to Google TPUs labels Sep 25, 2025
@jcyang43 jcyang43 force-pushed the update-tpu-bm-threshold branch from 9bec147 to b19d058 Compare September 25, 2025 21:55
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 updates the expected throughput threshold for a TPU benchmark. The change lowers the expected performance, which is a significant modification. My review focuses on improving the maintainability of this change by suggesting the addition of an inline comment to document the reason for this adjustment, preventing future confusion.

MAX_MODEL_LEN=2048
DOWNLOAD_DIR=/mnt/disks/persist
EXPECTED_THROUGHPUT=10.0
EXPECTED_THROUGHPUT=8.9
Copy link
Contributor

Choose a reason for hiding this comment

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

high

This change lowers the expected throughput by over 10%, which is a significant performance regression. To ensure code maintainability and provide context for future developers, it's crucial to document the reason for this change directly in the configuration file. Adding a comment will clarify why the threshold was adjusted and prevent confusion or unnecessary investigations into this performance change in the future.

# Lowered from 10.0 to align with performance on the stable torch_xla version.
EXPECTED_THROUGHPUT=8.9

Signed-off-by: Johnny Yang <johnnyyang@google.com>
@jcyang43 jcyang43 force-pushed the update-tpu-bm-threshold branch from b19d058 to bf98a7c Compare October 6, 2025 21:46
@yaochengji yaochengji added the ready ONLY add when PR is ready to merge/full CI is needed label Oct 6, 2025
Copy link
Collaborator

@yaochengji yaochengji left a comment

Choose a reason for hiding this comment

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

LGTM!

@yaochengji yaochengji merged commit 59012df into vllm-project:main Oct 7, 2025
18 checks passed
mrasquinha-g pushed a commit to mrasquinha-g/vllm that referenced this pull request Oct 9, 2025
Signed-off-by: Johnny Yang <johnnyyang@google.com>
xuebwang-amd pushed a commit to xuebwang-amd/vllm that referenced this pull request Oct 10, 2025
Signed-off-by: Johnny Yang <johnnyyang@google.com>
Signed-off-by: xuebwang-amd <xuebwang@amd.com>
Dhruvilbhatt pushed a commit to Dhruvilbhatt/vllm that referenced this pull request Oct 14, 2025
Signed-off-by: Johnny Yang <johnnyyang@google.com>
Signed-off-by: Dhruvil Bhatt <bhattdbh@amazon.com>
lywa1998 pushed a commit to lywa1998/vllm that referenced this pull request Oct 20, 2025
Signed-off-by: Johnny Yang <johnnyyang@google.com>
alhridoy pushed a commit to alhridoy/vllm that referenced this pull request Oct 24, 2025
Signed-off-by: Johnny Yang <johnnyyang@google.com>
xuebwang-amd pushed a commit to xuebwang-amd/vllm that referenced this pull request Oct 24, 2025
Signed-off-by: Johnny Yang <johnnyyang@google.com>
Signed-off-by: xuebwang-amd <xuebwang@amd.com>
rtourgeman pushed a commit to rtourgeman/vllm that referenced this pull request Nov 10, 2025
Signed-off-by: Johnny Yang <johnnyyang@google.com>
@jcyang43 jcyang43 deleted the update-tpu-bm-threshold branch November 14, 2025 20:23
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 tpu Related to Google TPUs

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants