-
-
Notifications
You must be signed in to change notification settings - Fork 10.4k
[CI/Build] get rid of unused VLLM_FA_CMAKE_GPU_ARCHES #21599
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
[CI/Build] get rid of unused VLLM_FA_CMAKE_GPU_ARCHES #21599
Conversation
This variable is currently unused and is confusing Signed-off-by: Daniele Trifirò <[email protected]>
There was a problem hiding this 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 cleanly removes the unused build argument VLLM_FA_CMAKE_GPU_ARCHES
. The changes are applied consistently across the CI script, Dockerfiles, and documentation. The removal of this obsolete variable improves code clarity and maintainability. The changes are correct and I see no issues.
fb9de49
to
330ef6a
Compare
cc @LucasWilkinson as discussed a few weeks ago 😅 |
👋 Hi! Thank you for contributing to the vLLM project. 💬 Join our developer Slack at https://slack.vllm.ai to discuss your PR in #pr-reviews, coordinate on features in #feat- channels, or join special interest groups in #sig- channels. Just a reminder: PRs would not trigger full CI run by default. Instead, it would only run Once the PR is approved and ready to go, your PR reviewer(s) can run CI to test the changes comprehensively before merging. To run CI, PR reviewers can either: Add 🚀 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Does this not reduce the binary size like the comments suggest?
@hmellor No, it's unused everywhere. You can grep for it in this repo and in flash-attention and it's not being used anywhere. See related PR in flash-attention repo here vllm-project/flash-attention#74 for more details. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
In that case, LGTM as long as the tests pass.
It does look like the failing image build is likely related though |
@hmellor nope: |
job restarted |
Oops, I mistook the unexpected size to be coming from our image, not a download |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM, Thank you for doing this!
…1599) Signed-off-by: Daniele Trifirò <[email protected]>
…1599) Signed-off-by: Daniele Trifirò <[email protected]> Signed-off-by: x22x22 <[email protected]>
…1599) Signed-off-by: Daniele Trifirò <[email protected]>
…1599) Signed-off-by: Daniele Trifirò <[email protected]> Signed-off-by: Jinzhen Lin <[email protected]>
…1599) Signed-off-by: Daniele Trifirò <[email protected]> Signed-off-by: Noam Gat <[email protected]>
…1599) Signed-off-by: Daniele Trifirò <[email protected]> Signed-off-by: Paul Pak <[email protected]>
…1599) Signed-off-by: Daniele Trifirò <[email protected]> Signed-off-by: Diego-Castan <[email protected]>
…1599) Signed-off-by: Daniele Trifirò <[email protected]>
…1599) Signed-off-by: Daniele Trifirò <[email protected]>
This variable is currently unused and is confusing.
Related: vllm-project/flash-attention#74