Skip to content

Conversation

@wangxiyuan
Copy link
Contributor

@wangxiyuan wangxiyuan commented Oct 16, 2025

Purpose

get_cu_count is only used by rocm which is controlled by current_platform.is_rocm already. Remove this redundant platform interface and move it to utils instead.

Test Plan

Test Result


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.

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 removes the get_cu_count method from the platform interface, replacing its usages with direct calls to torch.cuda.get_device_properties(0).multi_processor_count. While this simplifies the platform interface, it introduces code duplication and a potential correctness issue in multi-GPU environments due to the hardcoded device ID 0. My review includes critical feedback on this hardcoded device ID and suggests introducing a cached helper function to improve correctness, performance, and maintainability. I've also provided a suggestion to refactor the test code to improve its performance and robustness.

@wangxiyuan wangxiyuan changed the title [3/N][platform] remove get_cu_count [3/N][platform] Cleanup useless function Oct 16, 2025
@wangxiyuan wangxiyuan force-pushed the cleanup_platform_interface-3 branch from 5a08372 to 3f03c40 Compare October 23, 2025 02:22
@wangxiyuan wangxiyuan changed the title [3/N][platform] Cleanup useless function [2/N][platform] Cleanup useless function Oct 23, 2025
@wangxiyuan wangxiyuan force-pushed the cleanup_platform_interface-3 branch from 3f03c40 to 1c7e2e4 Compare October 29, 2025 03:45
@wangxiyuan wangxiyuan changed the title [2/N][platform] Cleanup useless function [2/N][platform] Move get_cu_count to utils Oct 29, 2025
@wangxiyuan
Copy link
Contributor Author

@DarkLight1337 @youkaichao platform interface cleanup. Need your review. Thanks.

@DarkLight1337 DarkLight1337 enabled auto-merge (squash) October 29, 2025 04:05
@github-actions github-actions bot added the ready ONLY add when PR is ready to merge/full CI is needed label Oct 29, 2025
auto-merge was automatically disabled November 12, 2025 08:53

Head branch was pushed to by a user without write access

@wangxiyuan wangxiyuan force-pushed the cleanup_platform_interface-3 branch from 1c7e2e4 to 9cc9a26 Compare November 12, 2025 08:53
@wangxiyuan wangxiyuan requested a review from tjtanaa as a code owner November 12, 2025 08:53
@wangxiyuan wangxiyuan changed the title [2/N][platform] Move get_cu_count to utils [platform] Move get_cu_count to utils Nov 12, 2025
Copy link
Collaborator

@Yikun Yikun left a comment

Choose a reason for hiding this comment

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

LGTM, if no more objection I will merge tmr.

Copy link
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 if this is a platform agnostic function.

@Yikun Yikun merged commit 2dacd57 into vllm-project:main Nov 13, 2025
53 checks passed
geodavic pushed a commit to geodavic/vllm that referenced this pull request Nov 16, 2025
Signed-off-by: wangxiyuan <[email protected]>
Signed-off-by: George D. Torres <[email protected]>
bwasti pushed a commit to bwasti/vllm that referenced this pull request Nov 17, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

ready ONLY add when PR is ready to merge/full CI is needed rocm Related to AMD ROCm

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants