refactor(dockerfile): reorganize Dockerfile management#1200
Conversation
Summary of ChangesHello @shihaobai, 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 simplifies the Docker build infrastructure by consolidating numerous specialized Dockerfiles into a single, highly configurable Dockerfile. This change improves maintainability and reduces redundancy by leveraging build arguments to enable or disable features like DeepEP, NIXL, and caching, while also updating the base CUDA version. The introduction of dedicated build scripts further streamlines the process of generating various image configurations. Highlights
🧠 New Feature in Public Preview: You can now enable Memory to help Gemini Code Assist learn from your team's feedback. This makes future code reviews more consistent and personalized to your project's style. Click here to enable Memory in your admin console. Ignored Files
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 is a great step towards simplifying the build process by consolidating numerous Dockerfiles into a single, parameterized one. This significantly improves maintainability. My review includes a few suggestions to further enhance the new Dockerfile. I've pointed out a security concern with directory permissions, recommended refactoring a very large and complex RUN command to improve readability and build efficiency, and identified an inconsistent CUDA version in one of the new build scripts. Addressing these points will make the new build process more robust and maintainable.
docker/Dockerfile
Outdated
| RUN set -e; \ | ||
| if [ "${ENABLE_CACHE}" = "1" ]; then \ | ||
| /opt/conda/bin/conda install -y boost && /opt/conda/bin/conda clean -ya; \ | ||
| LIGHTMEM_REF=5900baf92d85ef4dbda6124093506b0af906011a; \ | ||
| pip install --no-deps -v "git+https://github.com/ModelTC/LightMem.git@${LIGHTMEM_REF}#egg=light_mem"; \ | ||
| fi; \ | ||
| if [ "${ENABLE_NIXL}" = "1" ] || [ "${ENABLE_DEEPEP}" = "1" ]; then \ | ||
| apt-get update && apt-get install -y wget devscripts debhelper dh-make build-essential dkms && \ | ||
| apt-get install -y ibverbs-providers infiniband-diags perftest rdma-core libibverbs-dev librdmacm-dev && \ | ||
| rm -rf /var/lib/apt/lists/*; \ | ||
| mkdir -p /tmp/gdrcopy && cd /tmp \ | ||
| && git clone https://github.com/NVIDIA/gdrcopy.git -b v2.4.4 \ | ||
| && cd gdrcopy/packages \ | ||
| && CUDA=/usr/local/cuda ./build-deb-packages.sh \ | ||
| && dpkg -i gdrdrv-dkms_*.deb libgdrapi_*.deb gdrcopy-tests_*.deb gdrcopy_*.deb \ | ||
| && cd / && rm -rf /tmp/gdrcopy; \ | ||
| fi; \ | ||
| if [ "${ENABLE_DEEPEP}" = "1" ]; then \ | ||
| ln -sf /usr/lib/x86_64-linux-gnu/libmlx5.so.1 /usr/lib/x86_64-linux-gnu/libmlx5.so; \ | ||
| NVSHMEM_VERSION=3.3.9; \ | ||
| CUDA_ARCHS=90; \ | ||
| wget https://developer.download.nvidia.com/compute/redist/nvshmem/${NVSHMEM_VERSION}/source/nvshmem_src_cuda12-all-all-${NVSHMEM_VERSION}.tar.gz \ | ||
| && tar -xf nvshmem_src_cuda12-all-all-${NVSHMEM_VERSION}.tar.gz && mv nvshmem_src nvshmem \ | ||
| && cd nvshmem \ | ||
| && rm -f /root/nvshmem_src_cuda12-all-all-${NVSHMEM_VERSION}.tar.gz \ | ||
| && NVSHMEM_SHMEM_SUPPORT=0 \ | ||
| NVSHMEM_UCX_SUPPORT=0 \ | ||
| NVSHMEM_USE_NCCL=0 \ | ||
| NVSHMEM_MPI_SUPPORT=0 \ | ||
| NVSHMEM_IBGDA_SUPPORT=1 \ | ||
| NVSHMEM_PMIX_SUPPORT=0 \ | ||
| NVSHMEM_TIMEOUT_DEVICE_POLLING=0 \ | ||
| NVSHMEM_USE_GDRCOPY=1 \ | ||
| cmake -S . -B build/ -DCMAKE_INSTALL_PREFIX=/root/nvshmem/install -DCMAKE_CUDA_ARCHITECTURES=${CUDA_ARCHS} \ | ||
| && cmake --build build --target install -j64; \ | ||
| DEEPEP_COMMIT=b6ce310bb0b75079682d09bc2ebc063a074fbd58; \ | ||
| git clone https://github.com/deepseek-ai/DeepEP.git && cd DeepEP && git checkout ${DEEPEP_COMMIT} && cd ..; \ | ||
| cd /root/DeepEP && NVSHMEM_DIR=/root/nvshmem/install python setup.py install; \ | ||
| fi; \ | ||
| if [ "${ENABLE_NIXL}" = "1" ]; then \ | ||
| apt-get update && apt-get install -y cmake automake autotools-dev libtool libz-dev && \ | ||
| DEBIAN_FRONTEND=noninteractive apt-get -y install --reinstall libibverbs-dev rdma-core ibverbs-utils libibumad-dev; \ | ||
| rm -rf /usr/lib/ucx && rm -rf /opt/hpcx/ucx && \ | ||
| cd /usr/local/src && \ | ||
| git clone https://github.com/openucx/ucx.git && \ | ||
| cd ucx && \ | ||
| git checkout v1.19.x && \ | ||
| ./autogen.sh && ./configure \ | ||
| --enable-shared \ | ||
| --disable-static \ | ||
| --disable-doxygen-doc \ | ||
| --enable-optimizations \ | ||
| --enable-cma \ | ||
| --enable-devel-headers \ | ||
| --with-cuda=/usr/local/cuda \ | ||
| --with-verbs=yes \ | ||
| --with-dm \ | ||
| --with-gdrcopy=/usr/local \ | ||
| --with-efa \ | ||
| --enable-mt && \ | ||
| make -j && \ | ||
| make -j install-strip && \ | ||
| ldconfig; \ | ||
| apt-get update && apt-get install -y pkg-config tmux net-tools && \ | ||
| cd /usr/local/src; \ | ||
| pip install --upgrade meson pybind11 patchelf; \ | ||
| git clone https://github.com/ai-dynamo/nixl.git -b main && \ | ||
| cd nixl && \ | ||
| rm -rf build && \ | ||
| mkdir build && \ | ||
| meson setup build/ --prefix=/usr/local/nixl --buildtype=release && \ | ||
| cd build && \ | ||
| ninja && \ | ||
| ninja install && \ | ||
| cd .. && pip install . --no-deps; \ | ||
| fi; |
There was a problem hiding this comment.
This large RUN block has become very complex, which impacts maintainability, readability, and build efficiency. I recommend refactoring it with the following points in mind:
- Refactor to a script: The logic is complex enough to be moved to a separate shell script (e.g.,
install_dependencies.sh). You couldCOPYandRUNthis script, which would make the Dockerfile much cleaner and the logic easier to debug. - Consolidate
apt-getcommands: There are multipleapt-get updatecalls across the Dockerfile (e.g., lines 16, 50, 62, 95). All package installations should be done in a single layer after oneapt-get updateto improve build speed and reduce image size. - Use
ARGfor versions: Several commit hashes and versions are hardcoded (e.g.,LIGHTMEM_REF,NVSHMEM_VERSION,DEEPEP_COMMIT). Moving these toARGs at the top of the file makes them easier to manage.
docker/scripts/build_nixl_deepep.sh
Outdated
| cd "${ROOT_DIR}" | ||
|
|
||
| IMAGE_PREFIX="${IMAGE_PREFIX:-lightllm}" | ||
| CUDA_VERSION="${CUDA_VERSION:-12.6.1}" |
There was a problem hiding this comment.
The CUDA_VERSION is hardcoded to 12.6.1 here, while the other new build scripts default to 12.8.0. This inconsistency can lead to confusion and unexpected build behavior. If this specific version is required for the nixl.deepep build, please add a comment explaining why. Otherwise, it should be aligned with the other scripts for consistency.
| CUDA_VERSION="${CUDA_VERSION:-12.6.1}" | |
| CUDA_VERSION="${CUDA_VERSION:-12.8.0}" |
| g++ \ | ||
| make \ | ||
| git && \ | ||
| RUN chmod 777 -R /tmp && \ |
There was a problem hiding this comment.
chmod 777 -R /tmp grants universal read, write, and execute permissions, which is a security risk. A more secure practice is to use chmod 1777 /tmp, which sets the sticky bit. This allows any user to create files in /tmp, but only the file's owner can delete or rename them.
RUN chmod 1777 -R /tmp && \
shihaobai
changed the title
1 participant
No description provided.