Skip to content

refactor: remove models-store in favor of default ~/.docker/models #730

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.

Sign up for GitHub

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

Open
doringeman wants to merge 1 commit into
base: main
Choose a base branch
from
+8 −18
Open

refactor: remove models-store in favor of default ~/.docker/models #730

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 0 additions & 2 deletions .gitignore
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2,8 +2,6 @@
model-runner
model-runner.sock
docker-model
# Default MODELS_PATH in Makefile
models-store/
# Directory where we store the updated llama.cpp
updated-inference/
vendor/
Expand Down
5 changes: 1 addition & 4 deletions Makefile
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -11,7 +11,6 @@ DOCKER_IMAGE_SGLANG := docker/model-runner:latest-sglang
DOCKER_IMAGE_DIFFUSERS := docker/model-runner:latest-diffusers
DOCKER_TARGET ?= final-llamacpp
PORT := 8080
MODELS_PATH := $(shell pwd)/models-store
LLAMA_ARGS ?=
DOCKER_BUILD_ARGS := \
--load \
Expand Down Expand Up @@ -63,7 +62,6 @@ run: build
clean:
rm -f $(APP_NAME)
rm -f model-runner.sock
rm -rf $(MODELS_PATH)

# Run tests
test:
Expand Down Expand Up @@ -164,12 +162,11 @@ docker-run-diffusers: docker-build-diffusers
# Common implementation for running Docker container
docker-run-impl:
@echo ""
@echo "Starting service on port $(PORT) with model storage at $(MODELS_PATH)..."
@echo "Starting service on port $(PORT)..."
@echo "Service will be available at: http://localhost:$(PORT)"
@echo "Example usage: curl http://localhost:$(PORT)/models"
@echo ""
PORT="$(PORT)" \
MODELS_PATH="$(MODELS_PATH)" \
DOCKER_IMAGE="$(DOCKER_IMAGE)" \
LLAMA_ARGS="$(LLAMA_ARGS)" \
DMR_ORIGINS="$(DMR_ORIGINS)" \
Comment on lines +165 to 172
Copy link
Contributor

@gemini-code-assist gemini-code-assist bot Mar 3, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

high

These changes break the docker-run family of targets. While the Go application may have a default model path for local runs, when running in Docker, a volume must be mounted from the host to the container to share the model store. By removing MODELS_PATH from this rule, docker-run.sh will no longer mount any volume for models, and the container will be isolated from the host's ~/.docker/models directory.

To fix this while still defaulting to ~/.docker/models, you can use shell parameter expansion to provide a default value. This also keeps the ability for users to override the path via the command line (e.g., make docker-run MODELS_PATH=/custom/path).

	@echo "Starting service on port $(PORT) with model storage at $${MODELS_PATH:-$${HOME}/.docker/models}..."
	@echo "Service will be available at: http://localhost:$(PORT)"
	@echo "Example usage: curl http://localhost:$(PORT)/models"
	PORT="$(PORT)" \
	MODELS_PATH="$${MODELS_PATH:-$${HOME}/.docker/models}" \
	DOCKER_IMAGE="$(DOCKER_IMAGE)" \
	LLAMA_ARGS="$(LLAMA_ARGS)" \
	DMR_ORIGINS="$(DMR_ORIGINS)" \

Expand Down
4 changes: 2 additions & 2 deletions cmd/cli/Dockerfile
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -22,7 +22,7 @@ ARG DOCS_FORMATS
RUN --mount=target=/context \
--mount=target=.,type=tmpfs <<EOT
set -e
rsync -a --exclude='models-store' /context/. .
rsync -a /context/. .
docsgen --formats "$DOCS_FORMATS" --source "cmd/cli/docs/reference"
mkdir /out
cp -r cmd/cli/docs/reference/* /out/
Expand All @@ -35,7 +35,7 @@ FROM docs-build AS docs-validate
RUN --mount=target=/context \
--mount=target=.,type=tmpfs <<EOT
set -e
rsync -a --exclude='models-store' /context/. .
rsync -a /context/. .
git add -A
rm -rf cmd/cli/docs/reference/*
cp -rf /out/* ./cmd/cli/docs/reference/
Expand Down
2 changes: 1 addition & 1 deletion demos/embeddings/indexer.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -29,7 +29,7 @@ class CodebaseIndexer {
const ig = ignore().add(content);

// Always ignore these
ig.add(['node_modules', '.git', 'embeddings-index.json', 'models-store', 'model-store']);
ig.add(['node_modules', '.git', 'embeddings-index.json']);

return ig;
} catch (error) {
Expand Down
13 changes: 4 additions & 9 deletions scripts/docker-run.sh
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -33,9 +33,7 @@ add_optional_args() {
args+=(-p "$PORT:$PORT" -e "MODEL_RUNNER_PORT=$PORT")
fi

if [ -n "${MODELS_PATH-}" ]; then
args+=(-v "$MODELS_PATH:/models" -e MODELS_PATH=/models)
fi
args+=(-v "$models_path:/models" -e MODELS_PATH=/models)

for i in /usr/local/dcmi /usr/local/bin/npu-smi /usr/local/Ascend/driver/lib64/ /usr/local/Ascend/driver/version.info /etc/ascend_install.info; do
if [ -e "$i" ]; then
Expand Down Expand Up @@ -65,14 +63,11 @@ add_optional_args() {
main() {
set -eux -o pipefail

local models_path="${MODELS_PATH:-$HOME/.docker/models}"
Copy link
Contributor

@sourcery-ai sourcery-ai bot Mar 4, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

question (bug_risk): The new defaulting behavior removes the ability to opt out of a models volume by setting an empty MODELS_PATH.

${MODELS_PATH:-$HOME/.docker/models} now mounts the volume even when MODELS_PATH is empty, falling back to $HOME/.docker/models. If you want to preserve the ability to disable the mount via an empty MODELS_PATH, you could use ${MODELS_PATH:-} and only mount when it’s non-empty, or add an explicit flag to disable the models volume. If this change is intentional, it’s worth calling out as a behavioral change.

local args=(docker run --rm -e LLAMA_SERVER_PATH=/app/bin)
add_optional_args

# Ensure model path exists only if provided
if [ -n "${MODELS_PATH-}" ]; then
mkdir -p "$MODELS_PATH"
chmod a+rwx "$MODELS_PATH"
fi
mkdir -p "$models_path"
chmod a+rwx "$models_path"

if [ -z "${DOCKER_IMAGE-}" ]; then
echo "DOCKER_IMAGE is required" >&2
Expand Down
Loading