-
Notifications
You must be signed in to change notification settings - Fork 94
feat: add vLLM ROCm variant for AMD GPUs #638
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
base: main
Are you sure you want to change the base?
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -5,8 +5,10 @@ LLAMA_SERVER_VERSION := latest | |
| LLAMA_SERVER_VARIANT := cpu | ||
| BASE_IMAGE := ubuntu:24.04 | ||
| VLLM_BASE_IMAGE := nvidia/cuda:13.0.2-runtime-ubuntu24.04 | ||
| VLLM_ROCM_BASE_IMAGE := rocm/pytorch:rocm6.3.4_ubuntu24.04_py3.12_pytorch_release_2.6.0 | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. vllm tends to pull it's own version of everything on install in the venv, etc. We might be able to use plain-old ubuntu for all vLLM images. Llama.cpp is a bit different, need to install the dependancies ourselves
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. llama.cpp and vLLM can be on different ROCm versions if needs be. Hard to avoid that, we are moving to the one container image per backend model anyway. |
||
| DOCKER_IMAGE := docker/model-runner:latest | ||
| DOCKER_IMAGE_VLLM := docker/model-runner:latest-vllm-cuda | ||
| DOCKER_IMAGE_VLLM_ROCM := docker/model-runner:latest-vllm-rocm | ||
| DOCKER_IMAGE_SGLANG := docker/model-runner:latest-sglang | ||
| DOCKER_IMAGE_DIFFUSERS := docker/model-runner:latest-diffusers | ||
| DOCKER_TARGET ?= final-llamacpp | ||
|
|
@@ -26,7 +28,7 @@ DOCKER_BUILD_ARGS := \ | |
| BUILD_DMR ?= 1 | ||
|
|
||
| # Main targets | ||
| .PHONY: build run clean test integration-tests test-docker-ce-installation docker-build docker-build-multiplatform docker-run docker-build-vllm docker-run-vllm docker-build-sglang docker-run-sglang docker-run-impl help validate lint docker-build-diffusers docker-run-diffusers | ||
| .PHONY: build run clean test integration-tests test-docker-ce-installation docker-build docker-build-multiplatform docker-run docker-build-vllm docker-run-vllm docker-build-vllm-rocm docker-run-vllm-rocm docker-build-sglang docker-run-sglang docker-run-impl help validate lint docker-build-diffusers docker-run-diffusers | ||
| # Default target | ||
| .DEFAULT_GOAL := build | ||
|
|
||
|
|
@@ -106,6 +108,18 @@ docker-build-vllm: | |
| docker-run-vllm: docker-build-vllm | ||
| @$(MAKE) -s docker-run-impl DOCKER_IMAGE=$(DOCKER_IMAGE_VLLM) | ||
|
|
||
| # Build vLLM ROCm Docker image | ||
| docker-build-vllm-rocm: | ||
| @$(MAKE) docker-build \ | ||
| DOCKER_TARGET=final-vllm-rocm \ | ||
| DOCKER_IMAGE=$(DOCKER_IMAGE_VLLM_ROCM) \ | ||
| LLAMA_SERVER_VARIANT=rocm \ | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. This This will result in a path mismatch and a To fix this, the |
||
| BASE_IMAGE=$(VLLM_ROCM_BASE_IMAGE) | ||
|
|
||
| # Run vLLM ROCm Docker container with TCP port access and mounted model storage | ||
| docker-run-vllm-rocm: docker-build-vllm-rocm | ||
| @$(MAKE) -s docker-run-impl DOCKER_IMAGE=$(DOCKER_IMAGE_VLLM_ROCM) | ||
|
|
||
| # Build SGLang Docker image | ||
| docker-build-sglang: | ||
| @$(MAKE) docker-build \ | ||
|
|
@@ -160,6 +174,8 @@ help: | |
| @echo " docker-run - Run in Docker container with TCP port access and mounted model storage" | ||
| @echo " docker-build-vllm - Build vLLM Docker image" | ||
| @echo " docker-run-vllm - Run vLLM Docker container" | ||
| @echo " docker-build-vllm-rocm - Build vLLM ROCm Docker image" | ||
| @echo " docker-run-vllm-rocm - Run vLLM ROCm Docker container" | ||
| @echo " docker-build-sglang - Build SGLang Docker image" | ||
| @echo " docker-run-sglang - Run SGLang Docker container" | ||
| @echo " docker-build-diffusers - Build Diffusers Docker image" | ||
|
|
||
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.
There's significant duplication between this new
vllm-rocmstage and the existingvllmstage (lines 86-112). Many of the setup steps, such as installing system dependencies, creating directories, and installinguv, are identical.To improve maintainability and reduce code duplication, consider creating a common base stage (e.g.,
vllm-base) from which bothvllmandvllm-rocmcan inherit. This base stage would handle the common setup, and the specificvllmandvllm-rocmstages would only contain the logic for installing their respective versions of thevllmpackage.For example:
This would make the Dockerfile cleaner and easier to manage as more variants are added.
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.
It's a fair point about the duplication here