Fix vllm ci#265
Conversation
There was a problem hiding this comment.
Pull request overview
This PR fixes issues with the vLLM continuous integration (CI) pipeline by reorganizing the build process, improving error handling in workflows, and correcting the binary naming.
Changes:
- Modified Makefile to allow TOKENIZER_VERSION override and simplified venv checks
- Reorganized Dockerfile build stages to properly sequence venv setup and dependency installation
- Updated CI workflows to run setup-venv before caching and use sudo only where necessary
Reviewed changes
Copilot reviewed 4 out of 4 changed files in this pull request and generated 4 comments.
| File | Description |
|---|---|
| Makefile | Changed TOKENIZER_VERSION to conditional assignment, updated venv existence checks from pip to python binary, and simplified install-python-deps error handling |
| Dockerfile | Reordered build steps to run venv setup before copying dependencies, moved tokenizer download to make build phase, corrected binary name from llm-d-kv-cache-manager to llm-d-kv-cache, and added import verification tests |
| .github/workflows/ci-pr-checks.yaml | Reordered setup-venv to run before caching, moved go mod download into Install dependencies step, removed unnecessary sudo from build and test commands, and added error suppression for empty apt-archives |
| .github/workflows/ci-examples.yaml | Added error suppression for empty apt-archives and used sudo with environment preservation for install-python-deps |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| COPY --from=python-builder /workspace/pkg/preprocessing/chat_completions /app/pkg/preprocessing/chat_completions | ||
| COPY --from=python-builder /usr/local/lib/python3.12/site-packages /usr/local/lib/python3.12/site-packages | ||
| COPY --from=python-builder /workspace/build/venv/lib/python3.12/site-packages /workspace/build/venv/lib/python3.12/site-packages | ||
|
|
||
| # Set the PYTHONPATH. This mirrors the Makefile's export, ensuring both this project's | ||
| # Python code and the installed libraries (site-packages) are found at runtime. | ||
| ENV PYTHONPATH=/app/pkg/preprocessing/chat_completions:/usr/lib64/python3.12/site-packages | ||
| ENV PYTHONPATH=/app/pkg/preprocessing/chat_completions:/workspace/build/venv/lib/python3.12/site-packages |
There was a problem hiding this comment.
The file paths are inconsistent in the final image stage. Line 76 copies Python source code to '/app/pkg/preprocessing/chat_completions', while line 77 copies site-packages to '/workspace/build/venv/lib/python3.12/site-packages'. The PYTHONPATH on line 81 reflects this mixed structure with both '/app/' and '/workspace/' prefixes. While this will likely function correctly, it's inconsistent and harder to maintain. Consider organizing all Python-related files under a single directory prefix (either '/app/' or '/workspace/') for better clarity and maintainability.
| # Extract RELEASE_VERSION from Dockerfile | ||
| TOKENIZER_VERSION := $(shell grep '^ARG RELEASE_VERSION=' Dockerfile | cut -d'=' -f2) | ||
| TOKENIZER_VERSION ?= $(shell grep '^ARG RELEASE_VERSION=' Dockerfile | cut -d'=' -f2) | ||
|
|
||
| .PHONY: download-tokenizer | ||
| download-tokenizer: $(TOKENIZER_LIB) |
There was a problem hiding this comment.
The TOKENIZER_VERSION defined here is used by the download-tokenizer target below to fetch a precompiled native libtokenizers archive from https://github.com/daulet/tokenizers/releases/download/$(TOKENIZER_VERSION)/... via curl | tar without any checksum, signature, or content-hash verification, and the resulting library is linked into your binary. If the upstream GitHub release, the tag $(TOKENIZER_VERSION), or the network path is compromised, an attacker can silently inject arbitrary native code into the built kv-cache binary, leading to full compromise of any environment running this image. To mitigate, pin the dependency to an immutable identifier (e.g., a specific release asset hash or commit), verify the downloaded archive against a known checksum or signature before extraction, or vendor the library into the repository or a trusted internal artifact store.
There was a problem hiding this comment.
This dependency will be removed in the upcoming refactor. Given the short lifespan, I'd prefer to keep it simple.
|
/lgtm |
|
/approve |
Signed-off-by: HyunKyun Moon <mhg5303@gmail.com>
f41f23a to
0494dbd
Compare
Signed-off-by: HyunKyun Moon <mhg5303@gmail.com>
Initially, this PR was aimed at fixing CI failures. However, since those failures were resolved in another PR, I have pivoted this PR to focus on integrating Makefile commands into the Dockerfile for the build process. This change improves code reusability across the environment.