-
-
Notifications
You must be signed in to change notification settings - Fork 9.9k
[CI] Unifying Dockerfiles for ARM and X86 Builds #21343
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
Conversation
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 unifies the Dockerfiles for ARM and x86 CPU builds. However, the current method of setting the LD_PRELOAD
environment variable dynamically based on the architecture is unreliable. The review suggests using an entrypoint script to ensure the variable is correctly set at runtime.
👋 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 🚀 |
f685564
to
73dc19b
Compare
runs-on: ${{ matrix.os }} | ||
strategy: | ||
matrix: | ||
os: [ubuntu-latest, ubuntu-24.04-arm] |
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.
This workflow is just for checking the Helm charts, is it necessary to run it on multiple hardwares?
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.
This test is not just a deployment test of helm, but actually tests the most basic reasoning ability. Or do you mean that we should build some real arm tests? Or do you mean that there is no need for arm tests?
We currently lack arm cpu tests, and it is a good idea to use Github's actions arm runner to do basic smoke testing.
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.
@hmellor Any feedback? Should I remove it?
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.
Perhaps you can run this only on Arm platform as a basic smoke testing.
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.
Sorry, I don't quite understand. Do you mean to keep only ubuntu-24.04-arm
?
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.
Right, to save the time :)
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.
Thanks, updated. BTW, these two are actually executed concurrently.
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, just some nits. Please take a look.
Please also update the note on top of the Dockerfile.cpu
and add note for build argument --platform
.
runs-on: ${{ matrix.os }} | ||
strategy: | ||
matrix: | ||
os: [ubuntu-latest, ubuntu-24.04-arm] |
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.
Perhaps you can run this only on Arm platform as a basic smoke testing.
c27f79b
to
51bf7c4
Compare
Signed-off-by: Kebe <[email protected]>
51bf7c4
to
8728c90
Compare
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.
Thanks for the refactor :)
Hope @hmellor 's concern also is resolved. Let's see whether related tests can pass.
Yeah my concern is resolved. I was just pointing out that that GitHub actions workflow is just for testing Helm charts work properly and that the images built in the workflow are not kept/uploaded anywhere, so it wasn't worth doing both. |
Since switching to the arm GitHub actions runners, the |
Then I'll submit a PR to switch back. I didn't notice the time cost of this Pipeline before. |
No problem, we weren't to know it'd be that much slower. Please cc me in your PR :) |
Signed-off-by: Kebe <[email protected]>
Signed-off-by: Kebe <[email protected]> Signed-off-by: shuw <[email protected]>
Signed-off-by: Kebe <[email protected]> Signed-off-by: x22x22 <[email protected]>
Signed-off-by: Kebe <[email protected]>
Signed-off-by: Kebe <[email protected]>
Signed-off-by: Kebe <[email protected]> Signed-off-by: Jinzhen Lin <[email protected]>
Signed-off-by: Kebe <[email protected]> Signed-off-by: Paul Pak <[email protected]>
Signed-off-by: Kebe <[email protected]>
Signed-off-by: Kebe <[email protected]> Signed-off-by: Boyuan Feng <[email protected]>
Signed-off-by: Kebe <[email protected]> Signed-off-by: Diego-Castan <[email protected]>
Signed-off-by: Kebe <[email protected]>
Signed-off-by: Kebe <[email protected]>
Essential Elements of an Effective PR Description Checklist
supported_models.md
andexamples
for a new model.Purpose
Unify the way CPUs are built to reduce unnecessary maintenance costs.
Test Plan
See CI.
Test Result
https://github.com/vllm-project/vllm/actions/runs/16434542371/job/46442029158?pr=21343
(Optional) Documentation Update