Skip to content

[dockerfile]: swich to vllm==0.21.0 - #9

Closed
princepride wants to merge 3 commits into
mainfrom
add-verl-vllm-docker-build
Closed

[dockerfile]: swich to vllm==0.21.0#9
princepride wants to merge 3 commits into
mainfrom
add-verl-vllm-docker-build

Conversation

@princepride

Copy link
Copy Markdown
Collaborator

Purpose

Removed all SGLang-related Docker dependencies and replaced with vllm==0.21.0.

@gcanlin @aoshen02 I don't know how to test it because right now vime is still heavily dependent on SGLang.

Signed-off-by: princepirde <wangzhipeng628@gmail.com>
Signed-off-by: princepride <wangzhipeng628@gmail.com>

@gemini-code-assist gemini-code-assist Bot left a comment

Copy link
Copy Markdown
Contributor

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 migrates the Docker build process from an SGLang-based image to a vLLM-based image. Key changes include updating the base image, implementing a script to filter out SGLang dependencies from requirements.txt, and adding validation checks to ensure a clean vLLM environment. Review feedback suggests streamlining the VLLM_VERSION argument to avoid redundancy and removing an unreachable check for git conflict markers, as the shell is already configured to exit on error.

Comment thread docker/Dockerfile Outdated
Comment on lines +1 to +6
ARG BASE_IMAGE=vllm/vllm-openai:v0.21.0
FROM ${BASE_IMAGE}

# ======================================== Arguments =============================================

ARG PATCH_VERSION=latest
ARG MEGATRON_COMMIT=1dcf0dafa884ad52ffb243625717a3471643e087
ARG VLLM_VERSION=0.21.0

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

medium

The VLLM_VERSION is defined twice: once in the default value of BASE_IMAGE and once as a standalone ARG. This redundancy can lead to version mismatches if the base image is overridden but the version argument is not. Consider using VLLM_VERSION to construct the default BASE_IMAGE to ensure consistency.

ARG VLLM_VERSION=0.21.0
ARG BASE_IMAGE=vllm/vllm-openai:v${VLLM_VERSION}
FROM ${BASE_IMAGE}

# ======================================== Arguments =============================================

ARG VLLM_VERSION

Comment thread docker/Dockerfile
Comment on lines +56 to +61
git apply megatron.patch --3way && \
if grep -R -n '^<<<<<<< ' .; then \
echo "Megatron patch failed to apply cleanly. Please resolve conflicts." && \
exit 1; \
fi && \
rm megatron.patch && \

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

medium

This check for conflict markers is redundant and unreachable. The SHELL instruction on line 12 enables pipefail and errexit (-e), so the build will fail immediately if git apply returns a non-zero exit code. Additionally, git apply --3way does not leave conflict markers in files upon failure; it simply fails the operation.

    git apply megatron.patch --3way && \
    rm megatron.patch && \

@gcanlin

gcanlin commented May 18, 2026

Copy link
Copy Markdown
Collaborator

Purpose

Removed all SGLang-related Docker dependencies and replaced with vllm==0.21.0.

@gcanlin @aoshen02 I don't know how to test it because right now vime is still heavily dependent on SGLang.

I guess we need to wait #7 if we want to remove the dependencies from SGLang. But we can add vllm installation first. And after #7 done, we can remove SGLang totally in Dockerfile.

@princepride princepride self-assigned this May 18, 2026
@aoshen02

Copy link
Copy Markdown
Collaborator

Is there any cuda/pytorch/transformers version conflict between original configuration and what vLLM 0.21.0 needs?

@aoshen02 aoshen02 mentioned this pull request May 18, 2026
14 tasks
@aoshen02

Copy link
Copy Markdown
Collaborator

To let training not break, I think should still install sglang with --no-deps for now.

Comment thread docker/Dockerfile
RUN pip install tilelang -f https://tile-ai.github.io/whl/nightly/cu128/

# TE does not have wheel on cuda 13 yet, thus need to install from source
RUN if [ "${ENABLE_CUDA_13}" = "1" ]; then \

@aoshen02 aoshen02 May 18, 2026

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

We need TE, it's a dependency of mcore

@princepride

Copy link
Copy Markdown
Collaborator Author

Is there any cuda/pytorch/transformers version conflict between original configuration and what vLLM 0.21.0 needs?

So we have better fallback to 0.20.2 ?

Signed-off-by: princepride <wangzhipeng628@gmail.com>
@aoshen02

Copy link
Copy Markdown
Collaborator

#12 I

Is there any cuda/pytorch/transformers version conflict between original configuration and what vLLM 0.21.0 needs?

So we have better fallback to 0.20.2 ?

Not sure.

@aoshen02

Copy link
Copy Markdown
Collaborator

#12 I have a follow up PR that fix some problems, please check.

@princepride
princepride deleted the add-verl-vllm-docker-build branch June 9, 2026 03:19
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants