-
Notifications
You must be signed in to change notification settings - Fork 2.5k
[docker] feat: update Dockerfile.rocm7 #3781
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
[docker] feat: update Dockerfile.rocm7 #3781
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 successfully parameterizes the Dockerfile.rocm7 for the base image and the verl dependency, which is a good improvement for maintainability. The update of the Ray environment variable for ROCm is also a correct and necessary change. I have one suggestion to improve the efficiency of the git clone command to optimize the Docker build process.
| RUN git clone ${VERL_REPO} && \ | ||
| cd verl && \ | ||
| git checkout ${VERL_BRANCH} && \ | ||
| pip install -e . |
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.
The current git clone command downloads the entire history of the repository before checking out the specified branch. This is inefficient for Docker builds as it increases build time and layer size. A shallow clone of just the required branch would be more efficient.
RUN git clone --depth 1 --branch ${VERL_BRANCH} ${VERL_REPO} verl && \
cd verl && \
pip install -e .
yushengsu-thu
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.
@vickytsang In this version vllm, it uses GLoo backend, right?
### What does this PR do? Parameterize Dockerfile.rocm ### Test DOCKER_BUILDKIT=1 docker build --no-cache -f docker/Dockerfile.rocm7 --build-arg VERL_BRANCH=v0.6.x -t verl-0.6.x_rocm7.0 .
### What does this PR do? Parameterize Dockerfile.rocm ### Test DOCKER_BUILDKIT=1 docker build --no-cache -f docker/Dockerfile.rocm7 --build-arg VERL_BRANCH=v0.6.x -t verl-0.6.x_rocm7.0 .
### What does this PR do? Parameterize Dockerfile.rocm ### Test DOCKER_BUILDKIT=1 docker build --no-cache -f docker/Dockerfile.rocm7 --build-arg VERL_BRANCH=v0.6.x -t verl-0.6.x_rocm7.0 .
### What does this PR do? Parameterize Dockerfile.rocm ### Test DOCKER_BUILDKIT=1 docker build --no-cache -f docker/Dockerfile.rocm7 --build-arg VERL_BRANCH=v0.6.x -t verl-0.6.x_rocm7.0 .
### What does this PR do? Parameterize Dockerfile.rocm ### Test DOCKER_BUILDKIT=1 docker build --no-cache -f docker/Dockerfile.rocm7 --build-arg VERL_BRANCH=v0.6.x -t verl-0.6.x_rocm7.0 .
What does this PR do?
Parameterize Dockerfile.rocm
Test
DOCKER_BUILDKIT=1 docker build --no-cache -f docker/Dockerfile.rocm7 --build-arg VERL_BRANCH=v0.6.x -t verl-0.6.x_rocm7.0 .