feat: add vLLM Dynamo image to Earthly #700
Conversation
…/dynamo into hannahz/earthfile-improvements
…arthly image [WIP]
hhzhang16
requested review from
ishandhanani,
ptarasiewiczNV,
rmccorm4 and
tanmayv25
as code owners
hhzhang16
requested review from
a team,
alec-flowers and
nnshah1
as code owners
hhzhang16
requested review from
biswapanda,
hutm,
julienmancuso and
mohammedabdulwahhab
|
This PR is stale because it has been open 30 days with no activity. Remove stale label or comment or this will be closed in 5 days. |
WalkthroughA new multi-stage Earthfile was added for building vLLM, uv, and NIXL components in a CUDA-enabled environment. The main Earthfile was refactored to introduce reusable CUDA setup, update Docker image argument conventions, add a dedicated LLM base image build, and reorganize build orchestration targets. The README was updated to reflect the new Earthly-based image build and push workflow. Changes
Sequence Diagram(s)sequenceDiagram
participant User
participant Earthfile
participant Docker
participant ArtifactStore
User->>Earthfile: Run +dynamo-base-docker-llm (with DOCKER_SERVER, IMAGE_TAG)
Earthfile->>Docker: Build image with CUDA, NIXL, vLLM, UCX
Earthfile->>ArtifactStore: Copy NIXL/vLLM/uv artifacts from container/Earthfile
Earthfile->>Docker: Install dependencies, set env vars, verify imports
Earthfile->>Docker: Push built image to DOCKER_SERVER with IMAGE_TAG
Possibly related PRs
Suggested labels
Suggested reviewers
Poem
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
SupportNeed help? Create a ticket on our support page for assistance with any issues or questions. Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (2)
container/Earthfile (2)
60-61: Consider using a more modern manylinux tag.The
manylinux1tag is deprecated. Consider usingmanylinux2014ormanylinux_2_17for better library support while maintaining compatibility.- sed -i 's/Tag: cp38-abi3-linux_x86_64/Tag: cp38-abi3-manylinux1_x86_64/g' ${VLLM_PATCHED_PACKAGE_NAME}-${VLLM_PATCHED_PACKAGE_VERSION}.dist-info/WHEEL && \ - sed -i "s/-cp38-abi3-linux_x86_64.whl/-cp38-abi3-manylinux1_x86_64.whl/g" ${VLLM_PATCHED_PACKAGE_NAME}-${VLLM_PATCHED_PACKAGE_VERSION}.dist-info/RECORD && \ + sed -i 's/Tag: cp38-abi3-linux_x86_64/Tag: cp38-abi3-manylinux2014_x86_64/g' ${VLLM_PATCHED_PACKAGE_NAME}-${VLLM_PATCHED_PACKAGE_VERSION}.dist-info/WHEEL && \ + sed -i "s/-cp38-abi3-linux_x86_64.whl/-cp38-abi3-manylinux2014_x86_64.whl/g" ${VLLM_PATCHED_PACKAGE_NAME}-${VLLM_PATCHED_PACKAGE_VERSION}.dist-info/RECORD && \
83-83: Consider providing a default value for NIXL_COMMIT.The
NIXL_COMMITargument is required but has no default value, which could cause build failures if not provided.- ARG NIXL_COMMIT + ARG NIXL_COMMIT=main # or a specific commit hashAlternatively, add a build-time check to ensure it's provided:
ARG NIXL_COMMIT + RUN test -n "${NIXL_COMMIT}" || (echo "Error: NIXL_COMMIT must be provided" && exit 1)
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
Earthfile(4 hunks)README.md(1 hunks)container/Earthfile(1 hunks)
🔇 Additional comments (8)
container/Earthfile (2)
67-70: LGTM!Clean and efficient way to extract the uv binary from the official image.
144-144: The previous ripgrep run didn’t search any files—let’s locate the Earthfile and inspect both the base image and the CUDA flag:#!/bin/bash # Locate the Earthfile earthfile=$(find . -type f -name Earthfile | head -n1) echo "Using Earthfile: $earthfile" # Show the Docker base image line grep -n "FROM nvcr.io/nvidia/cuda-dl-base" "$earthfile" || echo "Base image line not found" # Show UCX build flags for CUDA grep -n "with-cuda" "$earthfile" || echo "No 'with-cuda' flags found" # Show any references to /usr/local/cuda grep -n "/usr/local/cuda" "$earthfile" || echo "No '/usr/local/cuda' references found"README.md (2)
60-64: LGTM!The updated build instructions using Earthly are clear and align with the new build process.
68-68: LGTM!Correctly references the new LLM-optimized image with the
-llmsuffix.Earthfile (4)
61-85: LGTM!Excellent implementation of a reusable CUDA setup function with comprehensive environment variables and proper error handling.
100-100: LGTM!Good refactoring to use the reusable SETUP_CUDA function.
150-151: LGTM!The new argument names are more descriptive and flexible than the CI-specific ones.
224-241: LGTM!Excellent reorganization of build targets into logical groups. The separation between cloud components and base images improves maintainability and allows for more granular builds.
|
|
||
| RUN mkdir -p /workspace/dist | ||
| RUN mkdir /tmp/vllm && \ | ||
| pip download --only-binary=:all: --no-deps --dest /tmp/vllm vllm==v${VLLM_REF} && \ |
There was a problem hiding this comment.
Fix the version specifier in pip download command.
The PyPI version format doesn't include the 'v' prefix. This will cause the download to fail.
Apply this diff to fix the version specifier:
- pip download --only-binary=:all: --no-deps --dest /tmp/vllm vllm==v${VLLM_REF} && \
+ pip download --only-binary=:all: --no-deps --dest /tmp/vllm vllm==${VLLM_REF} && \📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| pip download --only-binary=:all: --no-deps --dest /tmp/vllm vllm==v${VLLM_REF} && \ | |
| pip download --only-binary=:all: --no-deps --dest /tmp/vllm vllm==${VLLM_REF} && \ |
🤖 Prompt for AI Agents
In container/Earthfile at line 50, the pip download command incorrectly includes
a 'v' prefix in the version specifier for vllm, which is not valid for PyPI
versions. Remove the 'v' prefix from the version specifier so that it reads
vllm==${VLLM_REF} instead of vllm==v${VLLM_REF} to ensure the download succeeds.
| rm -rf /workspace/ai_dynamo_vllm-*.whl | ||
|
|
||
| # Verify both Dynamo and vllm are properly installed | ||
| RUN python3 -c "import dynamo; import vllm" || (echo "Failed to import Dynamo or vllm" && exit 1) |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Use the virtual environment Python for import verification.
The verification command uses python3 which might not be the virtual environment Python. Use the activated environment's Python instead.
- RUN python3 -c "import dynamo; import vllm" || (echo "Failed to import Dynamo or vllm" && exit 1)
+ RUN . /opt/dynamo/venv/bin/activate && python -c "import dynamo; import vllm" || (echo "Failed to import Dynamo or vllm" && exit 1)📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| RUN python3 -c "import dynamo; import vllm" || (echo "Failed to import Dynamo or vllm" && exit 1) | |
| RUN . /opt/dynamo/venv/bin/activate && python -c "import dynamo; import vllm" || (echo "Failed to import Dynamo or vllm" && exit 1) |
🤖 Prompt for AI Agents
In Earthfile at line 216, the import verification uses `python3` which may not
point to the virtual environment's Python interpreter. Change the command to use
the Python executable from the activated virtual environment, typically
referenced by `$(which python)` or `$VIRTUAL_ENV/bin/python`, to ensure the
imports are checked within the correct environment.
|
Earthly is going away: #2154 |
Overview:
This MR updates
Earthfileand createscontainer/Earthfilefor Dynamo vLLM Docker image support for easy building of a succinct image and for the CI.Details:
dynamo-base-docker-llmto build and verify the installation of both Dynamo and vllmcontainer/Earthfilecontaining avllm-buildtarget that builds a patched version of the vllm package. It will only need to rebuild when changes are made to thecontainers/deps/vllmsubdirectorySimple docker run result to show NIXL and GPU support:
Where should the reviewer start?
Run
earthly +dynamo-base-docker-llmto test locallyRelated Issues: (use one of the action keywords Closes / Fixes / Resolves / Relates to)
Summary by CodeRabbit
New Features
Improvements
Documentation