centralize version variables into .versions by alimx07 · Pull Request #821 · docker/model-runner

alimx07 · 2026-04-01T19:31:44Z

Closes #742

Add a .versions file as the single source of truth for version variables (Go, vLLM, vLLM upstream, SGLang, llama-server, vllm-metal, diffusers, base image), replacing values scattered across the Makefile, Dockerfile and CI workflows.
Add make validate-versions target to catch drift between .versions and Dockerfile ARG defaults, also runs as part of make validate-all

sourcery-ai

Hey - I've left some high level feedback:

The repeated Load GO version steps across multiple workflows (and even twice in ci.yml) could be deduplicated by using a small reusable script or composite action, which would reduce copy‑paste and keep the GO version loading logic in one place.
The validate-versions target assumes .versions lines map directly to ARG <KEY>=<value> with no quotes or inline comments; consider either documenting/enforcing this format or normalizing values (e.g., stripping quotes/comments) so the comparison is robust to small formatting differences.

Prompt for AI Agents

Please address the comments from this code review:

## Overall Comments
- The repeated `Load GO version` steps across multiple workflows (and even twice in `ci.yml`) could be deduplicated by using a small reusable script or composite action, which would reduce copy‑paste and keep the GO version loading logic in one place.
- The `validate-versions` target assumes `.versions` lines map directly to `ARG <KEY>=<value>` with no quotes or inline comments; consider either documenting/enforcing this format or normalizing values (e.g., stripping quotes/comments) so the comparison is robust to small formatting differences.

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.}

gemini-code-assist

Code Review

Review: Centralized Version Management ### Summary This PR centralizes versioning by introducing a .versions source of truth, integrated via the Makefile and validated against the Dockerfile. ### Critical - Makefile:112: The read loop in validate-versions will skip the last line of .versions because it lacks a trailing newline; add || [ -n "$$key" ] to the loop condition. - scripts/build-vllm-metal-tarball.sh:23: Fetching VLLM_VERSION instead of VLLM_UPSTREAM_VERSION causes an unintended downgrade from 0.17.1 to 0.17.0. ### What looks good - Centralizing version variables into a single .versions file improves maintainability and reduces the risk of version drift across components.

Makefile

gemini-code-assist · 2026-04-01T19:33:32Z

scripts/build-vllm-metal-tarball.sh

 TARBALL="$(cd "$(dirname "$TARBALL_ARG")" && pwd)/$(basename "$TARBALL_ARG")"

-VLLM_VERSION="0.17.1"
+VLLM_VERSION=$(grep '^VLLM_VERSION=' "$(cd "$(dirname "$0")/.." && pwd)/.versions" | cut -d= -f2)


This script previously used version 0.17.1. By switching to the VLLM_VERSION key from .versions, it has been downgraded to 0.17.0. Based on the .versions file content and the vllm-metal-dev target in the Makefile, this should likely use VLLM_UPSTREAM_VERSION (which is 0.17.1) to maintain consistency and avoid an unintended version downgrade.

Suggested change

VLLM_VERSION=$(grep '^VLLM_VERSION=' "$(cd "$(dirname "$0")/.." && pwd)/.versions" | cut -d= -f2)

VLLM_VERSION=$(grep '^VLLM_UPSTREAM_VERSION=' "$(cd "$(dirname "$0")/.." && pwd)/.versions" | cut -d= -f2)

ilopezluna

Thank you very much for your contribution 🙏 could you please address gemini's comment? Seems to be right about the accidental downgrade

alimx07 · 2026-04-02T10:42:46Z

Thank you very much for your contribution 🙏 could you please address gemini's comment? Seems to be right about the accidental downgrade
Just to be sure:

Just to be sure, VLLM_VERSION is 0.17.0 and VLLM_UPSTREAM_VERSION is 0.17.1. Previously, the script hardcoded VLLM_VERSION= 0.17.1. so it is actually VLLM_UPSTREAM_VERSION and not VLLM_VERSION, right?

ilopezluna · 2026-04-02T11:00:52Z

Thank you very much for your contribution 🙏 could you please address gemini's comment? Seems to be right about the accidental downgrade
Just to be sure:

Just to be sure, VLLM_VERSION is 0.17.0 and VLLM_UPSTREAM_VERSION is 0.17.1. Previously, the script hardcoded VLLM_VERSION= 0.17.1. so it is actually VLLM_UPSTREAM_VERSION and not VLLM_VERSION, right?

Yeah, it's confusing.

so it is actually VLLM_UPSTREAM_VERSION and not VLLM_VERSION, right?

Exactly. Maybe we should unify both and just use only one env var VLLM_VERSION, I think we keep both because we get vllm for cuda from wheels and we don't use wheels for vllm_metal, but not sure if that's required.

Add a .versions file as the single source of truth for version variables (Go, vLLM, vLLM upstream, SGLang, llama-server, vllm-metal, diffusers, base image), replacing values scattered across the Makefile, Dockerfile, CI workflows, and scripts.

.versions

doringeman · 2026-04-02T11:52:53Z

Makefile

+	while IFS='=' read -r key value || [ -n "$$key" ]; do \
+		case "$$key" in ''|\#*) continue ;; esac; \
+		value=$$(echo "$$value" | sed 's/[[:space:]]*#.*//;s/[[:space:]]*$$//'); \
+		dockerfile_val=$$(grep -m1 "^ARG $${key}=" Dockerfile | cut -d= -f2- | sed 's/[[:space:]]*#.*//;s/[[:space:]]*$$//'); \


Is it expected for the script to skip keys where no ARG X= exists in Dockerfile?

Yes, the current behavior check that every key in .versions and also appears as an ARG in the Dockerfile has the same value, while keys that only live in .versions (e.g. VLLM_METAL_RELEASE) are silently skipped.

Is it worth having the values provided in the Dockerfile as well and not rely on the Makefile to always supply it?

ARG BASE_IMAGE

yes , I think it is worth and you already mentioned that here in this comment :

One concern: Dockerfile ARG defaults can't be removed entirely. The Dockerfile needs to work standalone (docker build . without Make), for example in the inference-engine repo's CI which builds the image directly. Removing ARG defaults would break that. Instead, keep the ARG defaults in the Dockerfile but always pass --build-arg from Make/CI so the .versions values take precedence. The Dockerfile defaults become the fallback, not the source of truth.

doringeman · 2026-04-02T14:00:22Z

/gemini review

gemini-code-assist

Code Review

This pull request centralizes version management by introducing a .versions file as the single source of truth for project dependencies, including Go, vLLM, and SGLang. The changes refactor the Makefile, Dockerfile, and supporting scripts to dynamically load these versions and include a new validate-versions target to ensure consistency across the codebase. I have no feedback to provide.

doringeman · 2026-04-03T09:17:24Z

Dockerfile

 # syntax=docker/dockerfile:1

-ARG GO_VERSION=1.25
+ARG GO_VERSION=1.25.8


Suggested change

ARG GO_VERSION=1.25.8

ARG GO_VERSION=1.25

Let's not use patch versions, see docker/compose#13417 (comment).

sourcery-ai bot reviewed Apr 1, 2026

View reviewed changes

gemini-code-assist bot reviewed Apr 1, 2026

View reviewed changes

alimx07 force-pushed the add-versions-file branch from 559ac0a to 8f3b3fa Compare April 1, 2026 20:04

ilopezluna reviewed Apr 2, 2026

View reviewed changes

alimx07 force-pushed the add-versions-file branch from 8f3b3fa to 8743df1 Compare April 2, 2026 11:46

doringeman reviewed Apr 2, 2026

View reviewed changes

gemini-code-assist bot reviewed Apr 2, 2026

View reviewed changes

fix: add trailing newline to .versions

32ea7c6

doringeman reviewed Apr 3, 2026

View reviewed changes

Drop go patch version in Dockerfile

12b677b

	VLLM_VERSION=$(grep '^VLLM_VERSION=' "$(cd "$(dirname "$0")/.." && pwd)/.versions" \| cut -d= -f2)
	VLLM_VERSION=$(grep '^VLLM_UPSTREAM_VERSION=' "$(cd "$(dirname "$0")/.." && pwd)/.versions" \| cut -d= -f2)

Conversation

alimx07 commented Apr 1, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

sourcery-ai bot left a comment

Choose a reason for hiding this comment

Uh oh!

gemini-code-assist bot left a comment

Choose a reason for hiding this comment

Code Review

Uh oh!

Uh oh!

gemini-code-assist bot Apr 1, 2026

Choose a reason for hiding this comment

Uh oh!

ilopezluna left a comment

Choose a reason for hiding this comment

Uh oh!

alimx07 commented Apr 2, 2026

Uh oh!

ilopezluna commented Apr 2, 2026

Uh oh!

Uh oh!

doringeman Apr 2, 2026

Choose a reason for hiding this comment

Uh oh!

alimx07 Apr 2, 2026

Choose a reason for hiding this comment

Uh oh!

doringeman Apr 3, 2026

Choose a reason for hiding this comment

Uh oh!

alimx07 Apr 3, 2026

Choose a reason for hiding this comment

Uh oh!

doringeman commented Apr 2, 2026

Uh oh!

gemini-code-assist bot left a comment

Choose a reason for hiding this comment

Code Review

Uh oh!

doringeman Apr 3, 2026

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

alimx07 commented Apr 1, 2026 •

edited

Loading