feat: record probed dependency versions in runner.py.json - #274
Conversation
yxf0314
commented
Sep 16, 2026
- 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
There was a problem hiding this comment.
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.
|
|
||
| OUTPUT_PATH="${1:-/etc/gpustack-runner/dependencies.json}" | ||
|
|
||
| if [[ -z "${DEPENDENCY_PACKAGES// /}" ]]; then |
There was a problem hiding this comment.
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.
| if [[ -z "${DEPENDENCY_PACKAGES// /}" ]]; then | |
| if [[ -z "${DEPENDENCY_PACKAGES//[[:space:]]/}" ]]; then |
There was a problem hiding this comment.
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.
84c1cce to
3e54113
Compare
- 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>
3e54113 to
a9e5932
Compare