dockerfile for cuda12.8.0 #1106
Conversation
Summary of ChangesHello @hiworldwzj, I'm Gemini Code Assist1! I'm currently reviewing this pull request and will post my feedback shortly. In the meantime, here's a summary to help you and other reviewers quickly get up to speed! This pull request significantly expands the project's Docker support by introducing dedicated Dockerfiles for both CUDA 12.6.1 and 12.8.0. These new Dockerfiles cater to various specialized environments, including configurations for DeepEP and NixL, offering greater flexibility for users. The accompanying documentation updates provide clear guidance on how to build these images, streamlining the setup process for diverse use cases. Highlights
Using Gemini Code AssistThe full guide for Gemini Code Assist can be found on our documentation page, here are some quick tips. Invoking Gemini You can request assistance from Gemini at any point by creating a comment using either
Customization To customize Gemini Code Assist for GitHub experience, repository maintainers can create a configuration file and/or provide a custom code review style guide (such as PEP-8 for Python) by creating and adding files to a Limitations & Feedback Gemini Code Assist may make mistakes. Please leave feedback on any instances where its feedback is incorrect or counter productive. You can react with 👍 and 👎 on @gemini-code-assist comments. If you're interested in giving your feedback about your experience with Gemini Code Assist for Github and other Google products, sign up here. You can also get AI-powered code generation, chat, as well as code reviews directly in the IDE at no cost with the Gemini Code Assist IDE Extension. Footnotes
|
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request introduces new Dockerfiles for CUDA 12.6.1 and 12.8.0, organizing them into versioned directories, and updates the documentation accordingly. My review focuses on improving the security and maintainability of these Dockerfiles. I have identified several critical and high-severity security vulnerabilities, such as the use of --allow-insecure-repositories and world-writable permissions on /tmp. Additionally, I've pointed out opportunities to improve image size and build efficiency by consolidating RUN commands and removing redundant operations. There is significant code duplication across the Dockerfiles, which could be addressed in the future by using a common base image. I've also suggested a minor improvement to the English documentation for clarity.
| RUN chmod 777 -R /tmp && apt-get update --allow-insecure-repositories && DEBIAN_FRONTEND=noninteractive apt-get install -y --no-install-recommends \ | ||
| ca-certificates \ | ||
| libssl-dev \ | ||
| curl \ | ||
| g++ \ | ||
| make \ | ||
| git && \ | ||
| rm -rf /var/lib/apt/lists/* |
There was a problem hiding this comment.
This RUN command has two significant security issues:
--allow-insecure-repositories: This flag disables signature verification for APT repositories, making the build vulnerable to man-in-the-middle attacks. This should be removed.chmod 777 -R /tmp: This makes the/tmpdirectory world-writable, which is a security risk.
I've removed both from the suggestion.
RUN apt-get update && DEBIAN_FRONTEND=noninteractive apt-get install -y --no-install-recommends \
ca-certificates \
libssl-dev \
curl \
g++ \
make \
git && \
rm -rf /var/lib/apt/lists/*
| RUN chmod 777 -R /tmp && apt-get update --allow-insecure-repositories && DEBIAN_FRONTEND=noninteractive apt-get install -y --no-install-recommends \ | ||
| ca-certificates \ | ||
| libssl-dev \ | ||
| curl \ | ||
| g++ \ | ||
| make \ | ||
| git && \ | ||
| rm -rf /var/lib/apt/lists/* |
There was a problem hiding this comment.
This RUN command has two significant security issues:
--allow-insecure-repositories: This flag disables signature verification for APT repositories, making the build vulnerable to man-in-the-middle attacks. This should be removed.chmod 777 -R /tmp: This makes the/tmpdirectory world-writable, which is a security risk.
The suggestion below removes these.
RUN apt-get update && DEBIAN_FRONTEND=noninteractive apt-get install -y --no-install-recommends \
ca-certificates \
libssl-dev \
curl \
g++ \
make \
git && \
rm -rf /var/lib/apt/lists/*
| RUN chmod 777 -R /tmp && apt-get update --allow-insecure-repositories && DEBIAN_FRONTEND=noninteractive apt-get install -y --no-install-recommends \ | ||
| ca-certificates \ | ||
| libssl-dev \ | ||
| curl \ | ||
| g++ \ | ||
| make \ | ||
| git && \ | ||
| rm -rf /var/lib/apt/lists/* |
There was a problem hiding this comment.
This RUN command has two significant security issues:
--allow-insecure-repositories: This flag disables signature verification for APT repositories, making the build vulnerable to man-in-the-middle attacks. This should be removed.chmod 777 -R /tmp: This makes the/tmpdirectory world-writable, which is a security risk.
The suggestion below removes these.
RUN apt-get update && DEBIAN_FRONTEND=noninteractive apt-get install -y --no-install-recommends \
ca-certificates \
libssl-dev \
curl \
g++ \
make \
git && \
rm -rf /var/lib/apt/lists/*
| # TODO: offline compile | ||
| # RUN git clone https://github.com/ModelTC/LightKernel.git && cd LightKernel && pip install --no-deps -v . | ||
|
|
||
| RUN apt-get update --allow-insecure-repositories && apt-get install -y libnuma-dev wget devscripts debhelper dh-make build-essential dkms |
There was a problem hiding this comment.
Using --allow-insecure-repositories is a significant security risk as it disables repository signature verification, making the build vulnerable to man-in-the-middle attacks. Please remove this flag. If a specific repository is causing issues, its key should be imported securely.
RUN apt-get update && apt-get install -y libnuma-dev wget devscripts debhelper dh-make build-essential dkms
| RUN chmod 777 -R /tmp && apt-get update && DEBIAN_FRONTEND=noninteractive apt-get install -y --no-install-recommends \ | ||
| ca-certificates \ | ||
| libssl-dev \ | ||
| curl \ | ||
| g++ \ | ||
| make \ | ||
| git && \ | ||
| rm -rf /var/lib/apt/lists/* |
There was a problem hiding this comment.
Using chmod 777 -R /tmp is a security risk as it makes the temporary directory world-writable. This could allow any process or user within the container to tamper with files created by other processes. It's better to use more restrictive permissions or avoid changing them if not strictly necessary.
RUN apt-get update && DEBIAN_FRONTEND=noninteractive apt-get install -y --no-install-recommends \
ca-certificates \
libssl-dev \
curl \
g++ \
make \
git && \
rm -rf /var/lib/apt/lists/*
| curl -fsSL -o ~/mambaforge.sh -v "https://github.com/conda-forge/miniforge/releases/download/${MAMBA_VERSION}/Mambaforge-${MAMBA_VERSION}-Linux-${MAMBA_ARCH}.sh" && \ | ||
| bash ~/mambaforge.sh -b -p /opt/conda && \ | ||
| rm ~/mambaforge.sh |
There was a problem hiding this comment.
The -v (verbose) flag for curl is present here, which will add a lot of output to the build logs. This is usually only needed for debugging. It's recommended to remove it for cleaner build logs.
curl -fsSL -o ~/mambaforge.sh "https://github.com/conda-forge/miniforge/releases/download/${MAMBA_VERSION}/Mambaforge-${MAMBA_VERSION}-Linux-${MAMBA_ARCH}.sh" && \
bash ~/mambaforge.sh -b -p /opt/conda && \
rm ~/mambaforge.sh
| RUN --mount=type=cache,target=/root/.cache/pip pip install vllm --pre --extra-index-url https://wheels.vllm.ai/nightly | ||
| RUN --mount=type=cache,target=/root/.cache/pip git clone https://github.com/ModelTC/LightKernel.git && cd LightKernel && pip install --no-deps -v . | ||
|
|
||
| RUN apt-get update && apt-get install -y libnuma-dev wget devscripts debhelper dh-make build-essential dkms |
There was a problem hiding this comment.
This Dockerfile contains multiple RUN apt-get update commands (here, and on lines 55 and 80). To optimize image size and build time, it's best to consolidate all apt-get install commands into a single RUN layer. This layer should start with apt-get update, install all necessary packages, and end with rm -rf /var/lib/apt/lists/* to clean up.
| RUN pip install -U pip | ||
| RUN pip install -r /lightllm/requirements.txt --no-cache-dir |
There was a problem hiding this comment.
To reduce the number of Docker image layers and improve build efficiency, you can combine these pip commands into a single RUN instruction.
RUN pip install -U pip && \
pip install -r /lightllm/requirements.txt --no-cache-dir
| curl -fsSL -o ~/mambaforge.sh -v "https://github.com/conda-forge/miniforge/releases/download/${MAMBA_VERSION}/Mambaforge-${MAMBA_VERSION}-Linux-${MAMBA_ARCH}.sh" && \ | ||
| bash ~/mambaforge.sh -b -p /opt/conda && \ | ||
| rm ~/mambaforge.sh |
There was a problem hiding this comment.
The -v (verbose) flag for curl is present here, which will add a lot of output to the build logs. This is usually only needed for debugging. It's recommended to remove it for cleaner build logs.
curl -fsSL -o ~/mambaforge.sh "https://github.com/conda-forge/miniforge/releases/download/${MAMBA_VERSION}/Mambaforge-${MAMBA_VERSION}-Linux-${MAMBA_ARCH}.sh" && \
bash ~/mambaforge.sh -b -p /opt/conda && \
rm ~/mambaforge.sh
| $ # Manually build the image | ||
| $ docker build -t <image_name> . | ||
| $ docker build -t <image_name> -f ./docker/Dockerfile . |
There was a problem hiding this comment.
This is a good update to point to the specific Dockerfile. To make it even more helpful for users, you could add a note similar to the one in the Chinese documentation, explaining that there are multiple Dockerfiles for different scenarios in the docker/ directory.
| $ # Manually build the image | |
| $ docker build -t <image_name> . | |
| $ docker build -t <image_name> -f ./docker/Dockerfile . | |
| $ # Manually build the image. There are multiple Dockerfiles for different scenarios in the docker/ directory. | |
| $ docker build -t <image_name> -f ./docker/Dockerfile . |
No description provided.