Skip to content

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

Draft
doringeman wants to merge 1 commit intodocker:mainfrom
doringeman:rm-models-path-makefile
Draft

refactor: remove models-store in favor of default ~/.docker/models#730
doringeman wants to merge 1 commit intodocker:mainfrom
doringeman:rm-models-path-makefile

Conversation

@doringeman
Copy link
Contributor

The Go code already defaults MODELS_PATH to ~/.docker/models, so the Makefile override to a local models-store/ directory was unnecessary. This shares the model store with Docker Desktop's Model Runner.

Copy link
Contributor

@sourcery-ai sourcery-ai bot left a comment

Choose a reason for hiding this comment

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

Hey - I've reviewed your changes and they look great!


Sourcery is free for open source - if you like our reviews please consider sharing them ✨
Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.

Copy link
Contributor

@gemini-code-assist gemini-code-assist bot left a comment

Choose a reason for hiding this comment

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

Code Review

This pull request aims to remove the local models-store in favor of the default ~/.docker/models path used by Docker Desktop. The changes in .gitignore, cmd/cli/Dockerfile, and demos/embeddings/indexer.js are correct and align with this goal. However, the modification to the Makefile breaks the docker-run functionality by removing the volume mount for the model store. I've provided a suggestion to fix this by using a default path for MODELS_PATH in the docker-run-impl target, ensuring that Docker-based runs continue to work as expected while adopting the new default model path.

Comment on lines +165 to 172
@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)" \
Copy link
Contributor

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)" \

@doringeman doringeman force-pushed the rm-models-path-makefile branch from 762dd9c to 4087d46 Compare March 4, 2026 08:02
The Go code already defaults MODELS_PATH to ~/.docker/models, so the Makefile override to a local models-store/ directory was unnecessary. This shares the model store with Docker Desktop's Model Runner.

Signed-off-by: Dorin Geman <dorin.geman@docker.com>
@doringeman doringeman force-pushed the rm-models-path-makefile branch from 4087d46 to 3731f27 Compare March 4, 2026 08:02
@doringeman doringeman marked this pull request as draft March 4, 2026 08:11
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant