-
-
Notifications
You must be signed in to change notification settings - Fork 11.5k
[CI/Build] Allow hermetic builds #18064
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] Allow hermetic builds #18064
Conversation
|
👋 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 🚀 |
b001fdd to
8767f63
Compare
tlrmchlsmth
left a comment
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.
Is there any way to unit test this?
tlrmchlsmth
left a comment
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 overall and makes sense. I am a little concerned about fragility -- it isn't clear from looking at the Dockerfile why these variables are factored out into arguments, so I could easily see someone breaking the hermetic builds. I suggest adding some inline comments.
@khluu could you please take a look as well?
8767f63 to
6fae6df
Compare
|
@tlrmchlsmth, that's a fair point. I have added comments for the build arguments. Regarding unit testing, I'm not sure, unless we want to setup mirrors for the various artifacts. I have mostly focused on non-regression for the existing build system. |
6fae6df to
26ddb80
Compare
|
This pull request has merge conflicts that must be resolved before it can be |
e29a974 to
6ef1977
Compare
|
|
|
This pull request has merge conflicts that must be resolved before it can be |
c1b62cd to
a5444ad
Compare
8e0ce96 to
68a4420
Compare
|
You can merge from main after #18543 is merged to fix the CI |
68a4420 to
9110843
Compare
|
This pull request has merge conflicts that must be resolved before it can be |
364e751 to
78c3923
Compare
|
https://buildkite.com/vllm/ci/builds/22272/steps/table?sid=01978274-0ef8-4e19-81ea-2521eaaf40af 11.8 build failed, either due to rebase or parametrization. |
4ab5f84 to
927c04d
Compare
|
@simon-mo, I am not sure it's because of the parameterization. The CUDA 12.1 and 11.8 versions don't support the 10.0 capabilities, which were added in 12.4. So, maybe we should drop CUDA <= 12.4 on the main branch. |
927c04d to
5bbafb3
Compare
docker/Dockerfile
Outdated
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.
Can we make the base image for vllm-openai to be just the runtime cuda image, whereas we can maybe keep the devel image for test and CI.
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 PR is not meant to change the behaviour of the current build, only allow using internal URL for hermetic builds. Not saying it's a bad idea, only not the purpose, so it could be a follow-up PR.
|
This pull request has merge conflicts that must be resolved before it can be |
Signed-off-by: Fabien Dupont <[email protected]>
Signed-off-by: Fabien Dupont <[email protected]>
Signed-off-by: Fabien Dupont <[email protected]>
Signed-off-by: Fabien Dupont <[email protected]>
Signed-off-by: Fabien Dupont <[email protected]>
Signed-off-by: Fabien Dupont <[email protected]>
Signed-off-by: Fabien Dupont <[email protected]>
Signed-off-by: Tyler Michael Smith <[email protected]>
The default values for the build args should be specified only once to reduce risk of desynchronization. This commit also moves the comments to the top of the Dockerfile to avoid text duplication. Co-authored-by: Elias Levy <[email protected]> Signed-off-by: Fabien Dupont <[email protected]>
UV is using its own environment variables to set the package indexes. This commit adds the UV_INDEX_URL, UV_EXTRA_INDEX_URL and UV_KEYRING_PROVIDER. The defaults are copied from the PIP build args for convenience. Co-authored-by: Elias Levy <[email protected]> Signed-off-by: Fabien Dupont <[email protected]>
Signed-off-by: Harry Mellor <[email protected]>
- Remove extra white spaces after '\' on end of line. This shouldn't be an issue with the current command, but could make troubleshooting more difficult. - Use PYTHON_VERSION variable when copying to Python paths. Signed-off-by: Fabien Dupont <[email protected]>
5bbafb3 to
d53f83b
Compare
|
This merge request is a great move into the right direction. There are additional repository definitions directly in the requirement files like |
The current Dockerfile assumes that many build artifacts are available from public repositories and downloads them from these repositories, making it more difficult for downstream distributions to perform hermetic builds of vLLM container images.
This pull request introduces changes that should be backward compatible, while improving the situation for hermetic builds. Below is the list of changes:
Co-authored-by: Elias Levy [email protected]