Skip to content

feat: record probed dependency versions in runner.py.json - #274

Merged
thxCode merged 1 commit into
gpustack:mainfrom
yxf0314:spec/image-dependency-versions
Sep 16, 2026
Merged

thxCode merged 1 commit into
gpustack:mainfrom
yxf0314:spec/image-dependency-versions

Conversation

@yxf0314

@yxf0314 yxf0314 commented Sep 16, 2026

Copy link
Copy Markdown
Collaborator
  • probe the installed versions of a whitelisted set of Python packages inside every built image, and export them from the same buildx build
  • merge them into runner.py.json, refreshing them after a post operation
  • filter runners by PEP 440 version range on all three query entrypoints
  • guard the Dockerfile wiring with a test, since no pull request builds an image

@yxf0314
yxf0314 requested a review from thxCode September 16, 2026 07:13

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

Copy link
Copy Markdown

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 introduces a dependency probing mechanism for Docker images, allowing the system to record and query the versions of installed Python packages. It adds a build-time probe script, updates Dockerfiles to include this probe, and modifies the runner query logic to support dependency-based filtering. The review comments provide actionable improvements: adding robust error handling for non-compliant version strings, improving whitespace handling in bash scripts, and ensuring consistent UTF-8 encoding when reading or writing files.

Comment thread gpustack_runner/runner.py Outdated

OUTPUT_PATH="${1:-/etc/gpustack-runner/dependencies.json}"

if [[ -z "${DEPENDENCY_PACKAGES// /}" ]]; then

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

medium

The current bash check only strips literal space characters. If DEPENDENCY_PACKAGES contains other whitespace characters like tabs or newlines, the check will fail to identify it as empty, leading to a potential error in the Python script. Using [[:space:]] is more robust.

Suggested change
if [[ -z "${DEPENDENCY_PACKAGES// /}" ]]; then
if [[ -z "${DEPENDENCY_PACKAGES//[[:space:]]/}" ]]; then

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

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

Not taking this one — the mechanism is right but the remedy inverts the intent.

${VAR// /} does only strip literal spaces, correct. But a whitespace-only value is deliberately not treated as "empty": it falls through to the Python block, which fails hard with a precise message:

# A whitespace-only value (e.g. a stray tab) slips past the bash-side empty
# check but yields zero names here, and would be written out as a plausible
# `{}`. Hard failure instead -- see the three outcomes in the header.

.github/workflows/pack.yml rejects it one layer earlier for the same reason, with a comment naming this exact bash behavior:

# Match on "has a non-whitespace character" rather than stripping spaces:
# `${VAR// /}` strips spaces only, so a tab would pass as non-empty.

The empty case is the documented escape hatch for manual local builds ("skip probing, write no file"). Folding whitespace-only into it would turn an immediate, explanatory failure into a silent skip that only surfaces later when the export stage cannot find the file — which is the outcome the script header exists to prevent ("an empty map and a failed probe must never look alike").

Leaving the thread open for a human reviewer to weigh in.

Comment thread pack/shared/probe_dependencies.sh Outdated
Comment thread tests/pack/test_probe_wiring.py Outdated
Comment thread tests/pack/test_probe_wiring.py Outdated
Comment thread tests/pack/test_probe_wiring.py Outdated
@yxf0314
yxf0314 force-pushed the spec/image-dependency-versions branch 2 times, most recently from 84c1cce to 3e54113 Compare September 16, 2026 10:37
- probe the installed versions of a whitelisted set of Python packages
  inside every built image, and export them from the same buildx build
- merge them into runner.py.json, refreshing them after a post operation
- filter runners by PEP 440 version range on all three query entrypoints
- guard the Dockerfile wiring with a test, since no pull request builds
  an image

Signed-off-by: yxf <xf.ye@gpustack.ai>
@yxf0314
yxf0314 force-pushed the spec/image-dependency-versions branch from 3e54113 to a9e5932 Compare September 16, 2026 10:58

@thxCode thxCode left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

LGTM

@thxCode
thxCode merged commit 3c66b7c into gpustack:main Sep 16, 2026
6 checks passed
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.

2 participants