Add automated base image detection with hardware compatibility matrix - #2395
Closed
AadityaAnand wants to merge 1 commit into
Closed
Add automated base image detection with hardware compatibility matrix#2395AadityaAnand wants to merge 1 commit into
AadityaAnand wants to merge 1 commit into
Conversation
AadityaAnand
requested review from
bmahabirbu,
cgruver,
engelmi,
jhjaggars,
maxamillion,
rhatdan and
swarajpande5
as code owners
February 4, 2026 23:05
Contributor
Reviewer's GuideImplements an automated container image selection system driven by a hardware detection layer and an image compatibility matrix, along with defaults and tests, while preserving explicit user image overrides and adding config helpers to locate matrix files and schemas. File-Level Changes
Possibly linked issues
Tips and commandsInteracting with Sourcery
Customizing Your ExperienceAccess your dashboard to:
Getting Help
|
Contributor
Summary of ChangesHello @AadityaAnand, I'm Gemini Code Assist1! I'm currently reviewing this pull request and will post my feedback shortly. In the meantime, here's a summary to help you and other reviewers quickly get up to speed! This pull request introduces a significant enhancement to the system's ability to adapt to diverse hardware environments. By automating the selection of base container images, it streamlines deployment and ensures optimal performance across different configurations. The new framework intelligently identifies system capabilities and matches them against a defined compatibility matrix, making the process more robust and user-friendly, especially for non-standard setups. Highlights
🧠 New Feature in Public Preview: You can now enable Memory to help Gemini Code Assist learn from your team's feedback. This makes future code reviews more consistent and personalized to your project's style. Click here to enable Memory in your admin console. Changelog
Using Gemini Code AssistThe full guide for Gemini Code Assist can be found on our documentation page, here are some quick tips. Invoking Gemini You can request assistance from Gemini at any point by creating a comment using either
Customization To customize Gemini Code Assist for GitHub experience, repository maintainers can create a configuration file and/or provide a custom code review style guide (such as PEP-8 for Python) by creating and adding files to a Limitations & Feedback Gemini Code Assist may make mistakes. Please leave feedback on any instances where its feedback is incorrect or counter productive. You can react with 👍 and 👎 on @gemini-code-assist comments. If you're interested in giving your feedback about your experience with Gemini Code Assist for Github and other Google products, sign up here. You can also get AI-powered code generation, chat, as well as code reviews directly in the IDE at no cost with the Gemini Code Assist IDE Extension. Footnotes
|
![sourcery-ai[bot]](https://avatars.githubusercontent.com/in/48477?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Hey - I've found 1 security issue, 9 other issues, and left some high level feedback:
Security issues:
- Detected subprocess function 'run' without a static string. If this data can be controlled by a malicious actor, it may be an instance of command injection. Audit the use of this call to ensure it is not controllable by an external resource. You may consider using 'shlex.escape()'. (link)
General comments:
- In hardware.py, MIN_VRAM_BYTES is defined but never used; either wire this into the AMD detection logic (e.g., to skip GPUs below the threshold) or remove it to avoid confusion about intended behavior.
- The _tag_image helper treats any colon in the image string as indicating an existing tag, which will misbehave for registries with ports (e.g., 'registry:5000/image'); consider tightening the check to distinguish between port separators and tags (such as only treating a colon after the last '/' as a tag delimiter and ignoring '@' digests separately).
Prompt for AI Agents
Please address the comments from this code review:
## Overall Comments
- In hardware.py, MIN_VRAM_BYTES is defined but never used; either wire this into the AMD detection logic (e.g., to skip GPUs below the threshold) or remove it to avoid confusion about intended behavior.
- The _tag_image helper treats any colon in the image string as indicating an existing tag, which will misbehave for registries with ports (e.g., 'registry:5000/image'); consider tightening the check to distinguish between port separators and tags (such as only treating a colon after the last '/' as a tag delimiter and ignoring '@' digests separately).
## Individual Comments
### Comment 1
<location> `ramalama/hardware.py:77-79` </location>
<code_context>
+ return "linux"
+
+
+def is_arm() -> bool:
+ """Check if the system is ARM architecture."""
+ return platform.machine() in ('arm64', 'aarch64')
+
+
</code_context>
<issue_to_address>
**issue:** Normalize architecture in is_arm() to avoid missing ARM variants like 'ARM64' or 'AARCH64'.
Here you compare platform.machine() directly, but elsewhere (e.g., get_architecture) it’s normalized with .lower(). On some systems this can return 'ARM64' or 'AARCH64', causing ARM/ROCm detection to fail. Please normalize before comparison or reuse get_architecture for consistency.
</issue_to_address>
### Comment 2
<location> `ramalama/hardware.py:25-26` </location>
<code_context>
+GpuType: TypeAlias = Literal["cuda", "hip", "intel", "asahi", "cann", "musa", "vulkan", "metal", "none"]
+OsType: TypeAlias = Literal["linux", "darwin", "windows"]
+
+# Minimum VRAM threshold for AMD GPU detection (1 GiB)
+MIN_VRAM_BYTES = 1073741824
+
+
</code_context>
<issue_to_address>
**suggestion:** The defined AMD VRAM threshold is not actually applied in AMD GPU detection.
MIN_VRAM_BYTES is defined and documented as a 1 GiB threshold for AMD GPUs, but _detect_amd_gpus() never uses it, so very small or misconfigured GPUs may still be counted. Please either apply this threshold in the AMD VRAM detection logic (e.g., skip GPUs below it) or remove the constant/comment to match the actual behavior.
</issue_to_address>
### Comment 3
<location> `ramalama/image_compat.py:156-158` </location>
<code_context>
+ entries=[ImageEntry.from_dict(e) for e in data.get("images", [])],
+ )
+
+ @classmethod
+ def load(cls, path: Path) -> "ImageCompatibilityMatrix":
+ """
+ Load compatibility matrix from YAML file.
+
+ Args:
+ path: Path to YAML file
+
+ Returns:
+ ImageCompatibilityMatrix object
+
+ Raises:
+ FileNotFoundError: If file doesn't exist
+ yaml.YAMLError: If YAML is invalid
+ """
+ with open(path) as f:
+ data = yaml.safe_load(f)
+ return cls.from_dict(data)
+
</code_context>
<issue_to_address>
**suggestion (bug_risk):** ImageCompatibilityMatrix.load() assumes YAML always yields a dict; empty or malformed files will raise at runtime.
`yaml.safe_load()` can return `None` (e.g., empty file) or a non-dict, which will cause `cls.from_dict(data)` to fail with an unhelpful `AttributeError`. Consider checking that `data` is a dict and raising a clear configuration error when it isn’t, so issues in `images.yaml` are easier to diagnose.
```suggestion
with open(path) as f:
data = yaml.safe_load(f)
if not isinstance(data, dict):
raise ValueError(
f"Invalid image compatibility matrix YAML at {path}: "
f"expected a mapping at the top level but got {type(data).__name__!r}"
)
return cls.from_dict(data)
```
</issue_to_address>
### Comment 4
<location> `test/unit/test_image_selector.py:37-46` </location>
<code_context>
+ assert _tag_image("quay.io/ramalama/cuda", "0.17") == "quay.io/ramalama/cuda:0.17"
+
+
+class TestLoadImageMatrix:
+ """Tests for load_image_matrix function."""
+
+ def setup_method(self):
+ clear_image_cache()
+
+ @patch("ramalama.image_selector.get_image_matrix_files")
+ def test_no_files_returns_default(self, mock_get_files):
+ mock_get_files.return_value = []
+ matrix = load_image_matrix()
+ assert matrix.default_image == "quay.io/ramalama/ramalama"
+ assert len(matrix.entries) > 0 # Default matrix has entries
+
+
</code_context>
<issue_to_address>
**suggestion (testing):** Add tests for load_image_matrix precedence and caching behavior
Currently `TestLoadImageMatrix` only exercises the default path. Please also:
- Patch `get_image_matrix_files` to return multiple paths and assert `load_image_matrix()` picks the last one (e.g. `[Path("dev.yaml"), Path("system.yaml"), Path("user.yaml")]`).
- Add a test for caching: call `load_image_matrix()` twice and assert `ImageCompatibilityMatrix.load` is called once, then after `clear_image_cache()` assert it is called again.
This will validate both config precedence and the LRU cache behavior described in the docstring.
Suggested implementation:
```python
from unittest.mock import MagicMock, patch
from pathlib import Path
import pytest
from ramalama.hardware import GpuInfo, HardwareProfile
from ramalama.image_compat import (
ImageCompatibilityMatrix,
ImageConstraints,
ImageEntry,
)
```
```python
class TestLoadImageMatrix:
"""Tests for load_image_matrix function."""
def setup_method(self):
clear_image_cache()
@patch("ramalama.image_selector.get_image_matrix_files")
def test_no_files_returns_default(self, mock_get_files):
mock_get_files.return_value = []
matrix = load_image_matrix()
assert matrix.default_image == "quay.io/ramalama/ramalama"
assert len(matrix.entries) > 0 # Default matrix has entries
@patch("ramalama.image_selector.ImageCompatibilityMatrix.load")
@patch("ramalama.image_selector.get_image_matrix_files")
def test_last_file_has_precedence(self, mock_get_files, mock_load):
# Simulate multiple candidate config files; the last one should be chosen
paths = [Path("dev.yaml"), Path("system.yaml"), Path("user.yaml")]
mock_get_files.return_value = paths
mock_load.return_value = ImageCompatibilityMatrix(
default_image="quay.io/ramalama/custom",
entries=[],
)
matrix = load_image_matrix()
# Ensure the last path is used for loading the matrix
mock_load.assert_called_once_with(paths[-1])
assert matrix.default_image == "quay.io/ramalama/custom"
@patch("ramalama.image_selector.ImageCompatibilityMatrix.load")
def test_caching_and_clear_image_cache(self, mock_load):
# First load: ImageCompatibilityMatrix.load should be called
mock_load.return_value = ImageCompatibilityMatrix(
default_image="quay.io/ramalama/cached",
entries=[],
)
matrix1 = load_image_matrix()
matrix2 = load_image_matrix()
# LRU cache: load should only be called once for repeated calls
assert mock_load.call_count == 1
assert matrix1 is matrix2
# After clearing the cache, load should be called again
clear_image_cache()
matrix3 = load_image_matrix()
assert mock_load.call_count == 2
assert matrix3 is not None
```
</issue_to_address>
### Comment 5
<location> `test/unit/test_image_selector.py:51-60` </location>
<code_context>
+class TestSelectImage:
</code_context>
<issue_to_address>
**suggestion (testing):** Cover case where no matrix entries match and select_image falls back to default_image tagging
The current tests don’t cover the case where the matrix loads successfully but **no entries match** the hardware/runtime, so we never verify the fallback tagging behavior.
Please add a test where:
- `load_image_matrix` returns a matrix whose entries all fail the mocked `HardwareProfile` constraints.
- `config.default_image` is an untagged image (e.g. `"quay.io/ramalama/ramalama"`).
- `minor_release()` is patched to a known value.
Then assert `select_image` returns `"<default_image>:<minor_release>"`. This will exercise the default-image fallback path when the matrix is present but has no match.
Suggested implementation:
```python
class TestSelectImage:
"""Tests for select_image function."""
@patch("ramalama.image_selector.detect_hardware_profile")
@patch("ramalama.image_selector.load_image_matrix")
@patch("ramalama.common.minor_release")
def test_default_image_fallback_tags_untagged_default(
self, mock_release, mock_matrix, mock_profile
):
"""When no matrix entries match, default_image should be tagged with minor_release."""
from types import SimpleNamespace
from ramalama.image_selector import select_image
# Hardware profile is irrelevant; we just need "no matches"
mock_profile.return_value = SimpleNamespace()
# Minor release is fixed so we can assert the exact tag
mock_release.return_value = "1.2"
# Matrix loads successfully, but has no entries that can match
mock_matrix.return_value = SimpleNamespace(
default_image="quay.io/ramalama/ramalama",
entries=[], # no entries => nothing can match
)
# Config with an untagged default_image; structure kept minimal for this test
config = SimpleNamespace(default_image="quay.io/ramalama/ramalama")
image = select_image(config)
assert image == "quay.io/ramalama/ramalama:1.2"
```
Depending on the actual implementation, you may need to adjust this test:
1. **Config type/signature**
- If `select_image` expects a specific config type (e.g. `RamalamaConfig`) or additional attributes beyond `default_image`, replace the `SimpleNamespace` with an appropriate config fixture or constructor.
- If `select_image` has a different call signature (e.g. `select_image(config, runtime)`), update the call `select_image(config)` accordingly and, if needed, add a dummy `runtime` argument.
2. **Image matrix type**
- If `load_image_matrix()` is expected to return a specific `ImageMatrix` class rather than a generic object, construct an instance of that class instead of `SimpleNamespace`, ensuring `default_image` is untagged and `entries` is empty (or populated with entries that are guaranteed not to match the mocked `HardwareProfile`).
3. **Hardware profile matching**
- If the matching logic requires specific attributes on the hardware profile (e.g. `gpu_type`, `cpu_arch`), populate those attributes on the `SimpleNamespace` (or use the proper `HardwareProfile` type) so that all matrix entries are guaranteed not to match.
</issue_to_address>
### Comment 6
<location> `test/unit/test_image_compat.py:206-215` </location>
<code_context>
+ def test_select_image_with_tag_version_specific(self):
</code_context>
<issue_to_address>
**suggestion (testing):** Extend select_image_with_tag tests for major-only mappings and pre-tagged images
The existing tests only cover exact `major.minor` matches and the no-tag default. Please also add coverage for:
1. **Major-only tags** – e.g. `tags={"12": ...}` with driver `12.8.0`, confirming the major-only mapping is used.
2. **Pre-tagged/digest images** – when `ImageEntry.image` already has a tag or digest, ensure the function returns it unchanged instead of appending another tag.
These will better exercise the selection logic and mirror `_tag_image` behavior.
Suggested implementation:
```python
profile = HardwareProfile(
architecture="x86_64",
gpu=GpuInfo(gpu_type="cuda"),
os_type="linux",
)
# Higher priority should win
assert matrix.select_image(profile, "llama.cpp") == "quay.io/ramalama/cuda-high"
def test_select_image_with_tag_version_specific(self):
matrix = ImageCompatibilityMatrix(
schema_version="1.0.0",
default_image="quay.io/ramalama/ramalama",
entries=[
ImageEntry(
image="quay.io/ramalama/cuda",
constraints=ImageConstraints(gpu_types=["cuda"]),
priority=100,
tags={"12.4": "latest-12.4.1", "12.5": "latest-12.4.1"},
),
],
)
profile = HardwareProfile(
architecture="x86_64",
gpu=GpuInfo(gpu_type="cuda"),
os_type="linux",
)
# Exact major.minor match should use the corresponding tag
assert (
matrix.select_image(profile, "12.4.1")
== "quay.io/ramalama/cuda:latest-12.4.1"
)
# Different minor version with same mapping key should still use the configured tag
assert (
matrix.select_image(profile, "12.5.0")
== "quay.io/ramalama/cuda:latest-12.4.1"
)
def test_select_image_with_tag_major_only(self):
matrix = ImageCompatibilityMatrix(
schema_version="1.0.0",
default_image="quay.io/ramalama/ramalama",
entries=[
ImageEntry(
image="quay.io/ramalama/cuda",
constraints=ImageConstraints(gpu_types=["cuda"]),
priority=100,
# Only major version is specified in the mapping
tags={"12": "cuda-12-latest"},
),
],
)
profile = HardwareProfile(
architecture="x86_64",
gpu=GpuInfo(gpu_type="cuda"),
os_type="linux",
)
# When only a major mapping exists, a more specific driver version should
# still resolve via the major-only key.
assert (
matrix.select_image(profile, "12.8.0")
== "quay.io/ramalama/cuda:cuda-12-latest"
)
@pytest.mark.parametrize(
"pretagged_image",
[
"quay.io/ramalama/cuda:pretagged",
"quay.io/ramalama/cuda@sha256:deadbeef" # digest style
],
)
def test_select_image_with_pre_tagged_image_is_unchanged(self, pretagged_image):
matrix = ImageCompatibilityMatrix(
schema_version="1.0.0",
default_image="quay.io/ramalama/ramalama",
entries=[
ImageEntry(
image=pretagged_image,
constraints=ImageConstraints(gpu_types=["cuda"]),
priority=100,
# Even if tags are configured, pre-tagged/digested images
# should be returned as-is.
tags={"12.4": "ignored-tag"},
),
],
)
profile = HardwareProfile(
architecture="x86_64",
gpu=GpuInfo(gpu_type="cuda"),
os_type="linux",
)
assert matrix.select_image(profile, "12.4.1") == pretagged_image
```
If `pytest` is not already imported at the top of `test/unit/test_image_compat.py`, add `import pytest` alongside the other imports to support the `@pytest.mark.parametrize` decorator used in `test_select_image_with_pre_tagged_image_is_unchanged`.
</issue_to_address>
### Comment 7
<location> `test/unit/test_hardware.py:187-74` </location>
<code_context>
+class TestGpuDetection:
</code_context>
<issue_to_address>
**suggestion (testing):** Add targeted tests for individual GPU vendor detectors and cache clearing
The current tests cover only the top-level orchestration and not the vendor-specific helpers. Please add a few focused tests that:
- Patch the low-level command/file access used by each detector (e.g. `_run_cmd`, `open`, `glob.glob`) to simulate a positive detection and assert the returned `GpuInfo` (type, device_count, etc.) is correct.
- Verify `clear_detection_cache()` by calling `detect_cuda_version` / `detect_rocm_version` / `detect_gpu` twice with different mocked results and confirming that, after clearing the cache, the second result reflects the new mocks.
This will better protect the hardware detection layer as new backends are added.
Suggested implementation:
```python
from unittest.mock import patch, MagicMock
```
```python
version = detect_rocm_version()
assert version is None
class TestVendorGpuDetectors:
"""Targeted tests for individual GPU vendor detectors."""
@patch("ramalama.hardware._run_cmd")
def test_detect_nvidia_positive(self, mock_run_cmd):
# Simulate `nvidia-smi --query-gpu=name --format=csv,noheader`
mock_run_cmd.return_value = "NVIDIA A100-SXM4-40GB\nNVIDIA A100-SXM4-40GB\n"
from ramalama.hardware import _detect_nvidia, GpuInfo
info = _detect_nvidia()
assert isinstance(info, GpuInfo)
# These attribute names / semantics may need to be aligned with GpuInfo
assert getattr(info, "vendor", "").lower() == "nvidia"
assert getattr(info, "device_count", 2) == 2
@patch("ramalama.hardware.glob.glob")
def test_detect_asahi_positive(self, mock_glob):
# Simulate two Apple/Asahi DRM devices present
mock_glob.return_value = ["/dev/dri/card0", "/dev/dri/card1"]
from ramalama.hardware import _detect_asahi, GpuInfo
info = _detect_asahi()
assert isinstance(info, GpuInfo)
assert getattr(info, "device_count", 2) == 2
# Vendor may be "apple", "asahi", or a specific enum depending on implementation
assert getattr(info, "vendor", "").lower() in ("apple", "asahi")
@patch("ramalama.hardware.open", create=True)
@patch("ramalama.hardware.glob.glob")
def test_detect_amd_positive(self, mock_glob, mock_open):
# Simulate a single AMD GPU by exposing one DRM device and a name file
mock_glob.return_value = ["/sys/class/drm/card0/device"]
mock_file = MagicMock()
mock_file.__enter__.return_value.read.return_value = "AMD Radeon Pro"
mock_open.return_value = mock_file
from ramalama.hardware import _detect_amd, GpuInfo
info = _detect_amd()
assert isinstance(info, GpuInfo)
assert getattr(info, "device_count", 1) == 1
assert getattr(info, "vendor", "").lower() == "amd"
class TestGpuDetectionCache:
"""Tests for GPU detection cache behavior."""
def setup_method(self):
clear_detection_cache()
@patch("ramalama.hardware._run_cmd")
def test_detect_cuda_version_cache_and_clear(self, mock_run_cmd):
from ramalama.hardware import detect_cuda_version
mock_run_cmd.return_value = "12.1"
first = detect_cuda_version()
# Change mocked result, but cached value should be returned
mock_run_cmd.return_value = "13.0"
second = detect_cuda_version()
assert second == first
clear_detection_cache()
# After clearing cache, the new mocked value should be observed
third = detect_cuda_version()
assert third != first
@patch("ramalama.hardware._run_cmd")
def test_detect_rocm_version_cache_and_clear(self, mock_run_cmd):
from ramalama.hardware import detect_rocm_version
mock_run_cmd.return_value = "6.0.0"
first = detect_rocm_version()
mock_run_cmd.return_value = "6.1.0"
second = detect_rocm_version()
assert second == first
clear_detection_cache()
third = detect_rocm_version()
assert third != first
@patch("ramalama.hardware._detect_nvidia")
@patch("ramalama.hardware._detect_asahi")
def test_detect_gpu_cache_and_clear(self, mock_asahi, mock_nvidia):
from ramalama.hardware import detect_gpu, GpuInfo
# First detection result
first_gpu = GpuInfo(
vendor="nvidia",
name="First GPU",
device_count=1,
)
mock_asahi.return_value = None
mock_nvidia.return_value = first_gpu
first = detect_gpu()
# Change underlying detector return value
second_gpu = GpuInfo(
vendor="nvidia",
name="Second GPU",
device_count=2,
)
mock_nvidia.return_value = second_gpu
# Cached result should mask the change
second = detect_gpu()
assert second is first
clear_detection_cache()
# After clearing cache, the new value should be returned
third = detect_gpu()
assert third is second_gpu
class TestGpuDetection:
```
The above edits assume several details that may need to be aligned with your actual implementation:
1. `GpuInfo`:
- The tests currently access `vendor`, `name`, and `device_count` attributes and construct `GpuInfo(vendor=..., name=..., device_count=...)`.
- If `GpuInfo` has a different constructor signature or different attribute names (e.g. `type` instead of `vendor` or uses an enum), adjust the constructor calls and assertions accordingly.
2. Vendor-specific detector functions:
- The tests assume the existence of `_detect_nvidia`, `_detect_asahi`, and `_detect_amd` in `ramalama.hardware`.
- If the AMD detector has a different name (e.g. `_detect_rocm` or `_detect_amd_gpu`), update the imports and patches in `TestVendorGpuDetectors.test_detect_amd_positive`.
3. Low-level dependencies:
- The positive detector tests patch `ramalama.hardware._run_cmd`, `ramalama.hardware.glob.glob`, and `ramalama.hardware.open`.
- If the detector implementations invoke these helpers under different names or from different modules (e.g. `subprocess.run` directly or `pathlib.Path.glob`), update the patch targets to match the real call sites.
4. Cache semantics:
- The cache-behavior tests assume that `detect_cuda_version`, `detect_rocm_version`, and `detect_gpu` each cache their results and that `clear_detection_cache()` clears all of those caches.
- If the caching is implemented differently (e.g. per-backend clear functions or separate caches), adjust the tests so they exercise the actual cache invalidation behavior.
5. Imports:
- The file-level import edit assumes there is a line `from unittest.mock import patch`. If your file already imports `MagicMock` or has a differently formatted import line, adjust the search/replace so `MagicMock` is added without duplicating or breaking imports.
</issue_to_address>
### Comment 8
<location> `test/unit/test_image_selector.py:20` </location>
<code_context>
+from functools import lru_cache
+from typing import TYPE_CHECKING
+
+from ramalama.config import DEFAULT_IMAGE, get_image_matrix_files
+from ramalama.hardware import HardwareProfile, detect_hardware_profile
+from ramalama.image_compat import ImageCompatibilityMatrix, create_default_matrix
</code_context>
<issue_to_address>
**suggestion (testing):** Add tests that cover get_image_matrix_files / get_image_matrix_schema_files discovery behavior
These helpers are central to how `load_image_matrix` discovers config, but their directory search and precedence rules aren’t tested.
Please add focused tests (e.g. `test_config_image_matrix.py`) that:
- Use `tmp_path` and monkeypatched base paths to simulate `inference-spec/images/`, `/etc/...`, and `~/.config/...` with different `images.yaml`/`images.yml` and schema files.
- Assert `get_image_matrix_files()` returns files in the correct precedence order and only includes `images.(yml|yaml)`.
- Assert `get_image_matrix_schema_files()` maps version strings from filenames (e.g. `schema.1-0-0.json`) to the correct paths.
This will harden the config discovery behavior that the rest of the selection pipeline depends on.
</issue_to_address>
### Comment 9
<location> `ramalama/version_constraints.py:12` </location>
<code_context>
+VersionOperator: TypeAlias = Literal[">=", "<=", "==", ">", "<", "!="]
+
+
+@dataclass(frozen=True)
+class Version:
+ """Represents a semantic version (major.minor.patch)."""
</code_context>
<issue_to_address>
**issue (complexity):** Consider replacing the custom Version, VersionConstraint, and VersionRange classes with a single tuple-based helper function that parses versions and evaluates constraint strings directly.
The extra `Version`, `VersionConstraint`, and `VersionRange` types add a lot of surface area for what is effectively “given a driver version and a constraint string, is it compatible?”. You can keep all current behavior (operators, multiple comma-separated constraints, `"*"` / empty as “any”, `None` handling) with a single helper and a simple tuple/`packaging.version` representation.
Two concrete simplifications:
### 1. Drop `Version` in favor of tuples (or `packaging.version.Version`)
Instead of a custom class with `from_string`, `from_tuple`, `as_tuple`, and `__str__`, treat versions as normalized tuples:
```python
def _parse_version(version_str: str) -> tuple[int, int, int]:
parts = [int(p) for p in version_str.strip().split(".")]
# pad / trim to (major, minor, patch)
parts += [0] * (3 - len(parts))
return tuple(parts[:3])
```
Usage stays the same conceptually:
```python
v1 = _parse_version("12.4")
v2 = _parse_version("12.4.1")
if v1 <= v2:
...
```
That removes an entire custom type without losing any behavior.
### 2. Replace `VersionConstraint`/`VersionRange` with a single matcher
You can fold constraint parsing and matching into one internal function instead of two dataclasses and a mini-DSL:
```python
import re
from typing import Literal
VersionOperator = Literal[">=", "<=", "==", ">", "<", "!="]
_CONSTRAINT_RE = re.compile(r"^(>=|<=|==|>|<|!=)(.+)$")
def matches_version_range(
version: tuple[int, int, int] | None,
range_str: str | None,
) -> bool:
# "*" or empty -> accept any
if not range_str or range_str.strip() == "*":
return True
if version is None:
return False
v1 = version
for part in range_str.split(","):
part = part.strip()
if not part:
continue
m = _CONSTRAINT_RE.match(part)
if not m:
raise ValueError(f"Invalid version constraint: {part}")
op: VersionOperator = m.group(1) # type: ignore[assignment]
v2 = _parse_version(m.group(2))
if op == ">=" and not (v1 >= v2):
return False
if op == "<=" and not (v1 <= v2):
return False
if op == "==" and not (v1 == v2):
return False
if op == ">" and not (v1 > v2):
return False
if op == "<" and not (v1 < v2):
return False
if op == "!=" and not (v1 != v2):
return False
return True
```
Call sites that currently construct `VersionRange`/`Version` can be updated to:
```python
driver_version = _parse_version(driver_version_str) if driver_version_str else None
if matches_version_range(driver_version, ">=12.4,<13.0"):
...
```
This keeps:
- All current operators (`>=`, `<=`, `==`, `>`, `<`, `!=`)
- Comma-separated conjunction semantics
- `"*"` / empty as “any version”
- `None` handling identical to `VersionRange.matches`
while removing three public types and their associated mental overhead.
</issue_to_address>
### Comment 10
<location> `ramalama/hardware.py:84-90` </location>
<code_context>
return subprocess.run(
args,
capture_output=True,
text=True,
encoding=encoding,
check=True,
)
</code_context>
<issue_to_address>
**security (python.lang.security.audit.dangerous-subprocess-use-audit):** Detected subprocess function 'run' without a static string. If this data can be controlled by a malicious actor, it may be an instance of command injection. Audit the use of this call to ensure it is not controllable by an external resource. You may consider using 'shlex.escape()'.
*Source: opengrep*
</issue_to_address>Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
Comment on lines
+77
to
+79
| def is_arm() -> bool: | ||
| """Check if the system is ARM architecture.""" | ||
| return platform.machine() in ('arm64', 'aarch64') |
Contributor
There was a problem hiding this comment.
issue: Normalize architecture in is_arm() to avoid missing ARM variants like 'ARM64' or 'AARCH64'.
Here you compare platform.machine() directly, but elsewhere (e.g., get_architecture) it’s normalized with .lower(). On some systems this can return 'ARM64' or 'AARCH64', causing ARM/ROCm detection to fail. Please normalize before comparison or reuse get_architecture for consistency.
Comment on lines
+25
to
+26
| # Minimum VRAM threshold for AMD GPU detection (1 GiB) | ||
| MIN_VRAM_BYTES = 1073741824 |
Contributor
There was a problem hiding this comment.
suggestion: The defined AMD VRAM threshold is not actually applied in AMD GPU detection.
MIN_VRAM_BYTES is defined and documented as a 1 GiB threshold for AMD GPUs, but _detect_amd_gpus() never uses it, so very small or misconfigured GPUs may still be counted. Please either apply this threshold in the AMD VRAM detection logic (e.g., skip GPUs below it) or remove the constant/comment to match the actual behavior.
Comment on lines
+156
to
+158
| with open(path) as f: | ||
| data = yaml.safe_load(f) | ||
| return cls.from_dict(data) |
Contributor
There was a problem hiding this comment.
suggestion (bug_risk): ImageCompatibilityMatrix.load() assumes YAML always yields a dict; empty or malformed files will raise at runtime.
yaml.safe_load() can return None (e.g., empty file) or a non-dict, which will cause cls.from_dict(data) to fail with an unhelpful AttributeError. Consider checking that data is a dict and raising a clear configuration error when it isn’t, so issues in images.yaml are easier to diagnose.
Suggested change
| with open(path) as f: | |
| data = yaml.safe_load(f) | |
| return cls.from_dict(data) | |
| with open(path) as f: | |
| data = yaml.safe_load(f) | |
| if not isinstance(data, dict): | |
| raise ValueError( | |
| f"Invalid image compatibility matrix YAML at {path}: " | |
| f"expected a mapping at the top level but got {type(data).__name__!r}" | |
| ) | |
| return cls.from_dict(data) |
Comment on lines
+37
to
+46
| class TestLoadImageMatrix: | ||
| """Tests for load_image_matrix function.""" | ||
|
|
||
| def setup_method(self): | ||
| clear_image_cache() | ||
|
|
||
| @patch("ramalama.image_selector.get_image_matrix_files") | ||
| def test_no_files_returns_default(self, mock_get_files): | ||
| mock_get_files.return_value = [] | ||
| matrix = load_image_matrix() |
Contributor
There was a problem hiding this comment.
suggestion (testing): Add tests for load_image_matrix precedence and caching behavior
Currently TestLoadImageMatrix only exercises the default path. Please also:
- Patch
get_image_matrix_filesto return multiple paths and assertload_image_matrix()picks the last one (e.g.[Path("dev.yaml"), Path("system.yaml"), Path("user.yaml")]). - Add a test for caching: call
load_image_matrix()twice and assertImageCompatibilityMatrix.loadis called once, then afterclear_image_cache()assert it is called again.
This will validate both config precedence and the LRU cache behavior described in the docstring.
Suggested implementation:
from unittest.mock import MagicMock, patch
from pathlib import Path
import pytest
from ramalama.hardware import GpuInfo, HardwareProfile
from ramalama.image_compat import (
ImageCompatibilityMatrix,
ImageConstraints,
ImageEntry,
)class TestLoadImageMatrix:
"""Tests for load_image_matrix function."""
def setup_method(self):
clear_image_cache()
@patch("ramalama.image_selector.get_image_matrix_files")
def test_no_files_returns_default(self, mock_get_files):
mock_get_files.return_value = []
matrix = load_image_matrix()
assert matrix.default_image == "quay.io/ramalama/ramalama"
assert len(matrix.entries) > 0 # Default matrix has entries
@patch("ramalama.image_selector.ImageCompatibilityMatrix.load")
@patch("ramalama.image_selector.get_image_matrix_files")
def test_last_file_has_precedence(self, mock_get_files, mock_load):
# Simulate multiple candidate config files; the last one should be chosen
paths = [Path("dev.yaml"), Path("system.yaml"), Path("user.yaml")]
mock_get_files.return_value = paths
mock_load.return_value = ImageCompatibilityMatrix(
default_image="quay.io/ramalama/custom",
entries=[],
)
matrix = load_image_matrix()
# Ensure the last path is used for loading the matrix
mock_load.assert_called_once_with(paths[-1])
assert matrix.default_image == "quay.io/ramalama/custom"
@patch("ramalama.image_selector.ImageCompatibilityMatrix.load")
def test_caching_and_clear_image_cache(self, mock_load):
# First load: ImageCompatibilityMatrix.load should be called
mock_load.return_value = ImageCompatibilityMatrix(
default_image="quay.io/ramalama/cached",
entries=[],
)
matrix1 = load_image_matrix()
matrix2 = load_image_matrix()
# LRU cache: load should only be called once for repeated calls
assert mock_load.call_count == 1
assert matrix1 is matrix2
# After clearing the cache, load should be called again
clear_image_cache()
matrix3 = load_image_matrix()
assert mock_load.call_count == 2
assert matrix3 is not None
Comment on lines
+51
to
+60
| class TestSelectImage: | ||
| """Tests for select_image function.""" | ||
|
|
||
| def setup_method(self): | ||
| clear_image_cache() | ||
|
|
||
| @patch("ramalama.image_selector.detect_hardware_profile") | ||
| @patch("ramalama.image_selector.load_image_matrix") | ||
| @patch("ramalama.common.minor_release") | ||
| def test_user_config_override(self, mock_release, mock_matrix, mock_profile): |
Contributor
There was a problem hiding this comment.
suggestion (testing): Cover case where no matrix entries match and select_image falls back to default_image tagging
The current tests don’t cover the case where the matrix loads successfully but no entries match the hardware/runtime, so we never verify the fallback tagging behavior.
Please add a test where:
load_image_matrixreturns a matrix whose entries all fail the mockedHardwareProfileconstraints.config.default_imageis an untagged image (e.g."quay.io/ramalama/ramalama").minor_release()is patched to a known value.
Then assert select_image returns "<default_image>:<minor_release>". This will exercise the default-image fallback path when the matrix is present but has no match.
Suggested implementation:
class TestSelectImage:
"""Tests for select_image function."""
@patch("ramalama.image_selector.detect_hardware_profile")
@patch("ramalama.image_selector.load_image_matrix")
@patch("ramalama.common.minor_release")
def test_default_image_fallback_tags_untagged_default(
self, mock_release, mock_matrix, mock_profile
):
"""When no matrix entries match, default_image should be tagged with minor_release."""
from types import SimpleNamespace
from ramalama.image_selector import select_image
# Hardware profile is irrelevant; we just need "no matches"
mock_profile.return_value = SimpleNamespace()
# Minor release is fixed so we can assert the exact tag
mock_release.return_value = "1.2"
# Matrix loads successfully, but has no entries that can match
mock_matrix.return_value = SimpleNamespace(
default_image="quay.io/ramalama/ramalama",
entries=[], # no entries => nothing can match
)
# Config with an untagged default_image; structure kept minimal for this test
config = SimpleNamespace(default_image="quay.io/ramalama/ramalama")
image = select_image(config)
assert image == "quay.io/ramalama/ramalama:1.2"Depending on the actual implementation, you may need to adjust this test:
-
Config type/signature
- If
select_imageexpects a specific config type (e.g.RamalamaConfig) or additional attributes beyonddefault_image, replace theSimpleNamespacewith an appropriate config fixture or constructor. - If
select_imagehas a different call signature (e.g.select_image(config, runtime)), update the callselect_image(config)accordingly and, if needed, add a dummyruntimeargument.
- If
-
Image matrix type
- If
load_image_matrix()is expected to return a specificImageMatrixclass rather than a generic object, construct an instance of that class instead ofSimpleNamespace, ensuringdefault_imageis untagged andentriesis empty (or populated with entries that are guaranteed not to match the mockedHardwareProfile).
- If
-
Hardware profile matching
- If the matching logic requires specific attributes on the hardware profile (e.g.
gpu_type,cpu_arch), populate those attributes on theSimpleNamespace(or use the properHardwareProfiletype) so that all matrix entries are guaranteed not to match.
- If the matching logic requires specific attributes on the hardware profile (e.g.
Comment on lines
+206
to
+215
| def test_select_image_with_tag_version_specific(self): | ||
| matrix = ImageCompatibilityMatrix( | ||
| schema_version="1.0.0", | ||
| default_image="quay.io/ramalama/ramalama", | ||
| entries=[ | ||
| ImageEntry( | ||
| image="quay.io/ramalama/cuda", | ||
| constraints=ImageConstraints(gpu_types=["cuda"]), | ||
| priority=100, | ||
| tags={"12.4": "latest-12.4.1", "12.5": "latest-12.4.1"}, |
Contributor
There was a problem hiding this comment.
suggestion (testing): Extend select_image_with_tag tests for major-only mappings and pre-tagged images
The existing tests only cover exact major.minor matches and the no-tag default. Please also add coverage for:
- Major-only tags – e.g.
tags={"12": ...}with driver12.8.0, confirming the major-only mapping is used. - Pre-tagged/digest images – when
ImageEntry.imagealready has a tag or digest, ensure the function returns it unchanged instead of appending another tag.
These will better exercise the selection logic and mirror _tag_image behavior.
Suggested implementation:
profile = HardwareProfile(
architecture="x86_64",
gpu=GpuInfo(gpu_type="cuda"),
os_type="linux",
)
# Higher priority should win
assert matrix.select_image(profile, "llama.cpp") == "quay.io/ramalama/cuda-high"
def test_select_image_with_tag_version_specific(self):
matrix = ImageCompatibilityMatrix(
schema_version="1.0.0",
default_image="quay.io/ramalama/ramalama",
entries=[
ImageEntry(
image="quay.io/ramalama/cuda",
constraints=ImageConstraints(gpu_types=["cuda"]),
priority=100,
tags={"12.4": "latest-12.4.1", "12.5": "latest-12.4.1"},
),
],
)
profile = HardwareProfile(
architecture="x86_64",
gpu=GpuInfo(gpu_type="cuda"),
os_type="linux",
)
# Exact major.minor match should use the corresponding tag
assert (
matrix.select_image(profile, "12.4.1")
== "quay.io/ramalama/cuda:latest-12.4.1"
)
# Different minor version with same mapping key should still use the configured tag
assert (
matrix.select_image(profile, "12.5.0")
== "quay.io/ramalama/cuda:latest-12.4.1"
)
def test_select_image_with_tag_major_only(self):
matrix = ImageCompatibilityMatrix(
schema_version="1.0.0",
default_image="quay.io/ramalama/ramalama",
entries=[
ImageEntry(
image="quay.io/ramalama/cuda",
constraints=ImageConstraints(gpu_types=["cuda"]),
priority=100,
# Only major version is specified in the mapping
tags={"12": "cuda-12-latest"},
),
],
)
profile = HardwareProfile(
architecture="x86_64",
gpu=GpuInfo(gpu_type="cuda"),
os_type="linux",
)
# When only a major mapping exists, a more specific driver version should
# still resolve via the major-only key.
assert (
matrix.select_image(profile, "12.8.0")
== "quay.io/ramalama/cuda:cuda-12-latest"
)
@pytest.mark.parametrize(
"pretagged_image",
[
"quay.io/ramalama/cuda:pretagged",
"quay.io/ramalama/cuda@sha256:deadbeef" # digest style
],
)
def test_select_image_with_pre_tagged_image_is_unchanged(self, pretagged_image):
matrix = ImageCompatibilityMatrix(
schema_version="1.0.0",
default_image="quay.io/ramalama/ramalama",
entries=[
ImageEntry(
image=pretagged_image,
constraints=ImageConstraints(gpu_types=["cuda"]),
priority=100,
# Even if tags are configured, pre-tagged/digested images
# should be returned as-is.
tags={"12.4": "ignored-tag"},
),
],
)
profile = HardwareProfile(
architecture="x86_64",
gpu=GpuInfo(gpu_type="cuda"),
os_type="linux",
)
assert matrix.select_image(profile, "12.4.1") == pretagged_imageIf pytest is not already imported at the top of test/unit/test_image_compat.py, add import pytest alongside the other imports to support the @pytest.mark.parametrize decorator used in test_select_image_with_pre_tagged_image_is_unchanged.
|
|
||
| def test_default_values(self): | ||
| gpu = GpuInfo(gpu_type="none") | ||
| assert gpu.gpu_type == "none" |
Contributor
There was a problem hiding this comment.
suggestion (testing): Add targeted tests for individual GPU vendor detectors and cache clearing
The current tests cover only the top-level orchestration and not the vendor-specific helpers. Please add a few focused tests that:
- Patch the low-level command/file access used by each detector (e.g.
_run_cmd,open,glob.glob) to simulate a positive detection and assert the returnedGpuInfo(type, device_count, etc.) is correct. - Verify
clear_detection_cache()by callingdetect_cuda_version/detect_rocm_version/detect_gputwice with different mocked results and confirming that, after clearing the cache, the second result reflects the new mocks.
This will better protect the hardware detection layer as new backends are added.
Suggested implementation:
from unittest.mock import patch, MagicMock version = detect_rocm_version()
assert version is None
class TestVendorGpuDetectors:
"""Targeted tests for individual GPU vendor detectors."""
@patch("ramalama.hardware._run_cmd")
def test_detect_nvidia_positive(self, mock_run_cmd):
# Simulate `nvidia-smi --query-gpu=name --format=csv,noheader`
mock_run_cmd.return_value = "NVIDIA A100-SXM4-40GB\nNVIDIA A100-SXM4-40GB\n"
from ramalama.hardware import _detect_nvidia, GpuInfo
info = _detect_nvidia()
assert isinstance(info, GpuInfo)
# These attribute names / semantics may need to be aligned with GpuInfo
assert getattr(info, "vendor", "").lower() == "nvidia"
assert getattr(info, "device_count", 2) == 2
@patch("ramalama.hardware.glob.glob")
def test_detect_asahi_positive(self, mock_glob):
# Simulate two Apple/Asahi DRM devices present
mock_glob.return_value = ["/dev/dri/card0", "/dev/dri/card1"]
from ramalama.hardware import _detect_asahi, GpuInfo
info = _detect_asahi()
assert isinstance(info, GpuInfo)
assert getattr(info, "device_count", 2) == 2
# Vendor may be "apple", "asahi", or a specific enum depending on implementation
assert getattr(info, "vendor", "").lower() in ("apple", "asahi")
@patch("ramalama.hardware.open", create=True)
@patch("ramalama.hardware.glob.glob")
def test_detect_amd_positive(self, mock_glob, mock_open):
# Simulate a single AMD GPU by exposing one DRM device and a name file
mock_glob.return_value = ["/sys/class/drm/card0/device"]
mock_file = MagicMock()
mock_file.__enter__.return_value.read.return_value = "AMD Radeon Pro"
mock_open.return_value = mock_file
from ramalama.hardware import _detect_amd, GpuInfo
info = _detect_amd()
assert isinstance(info, GpuInfo)
assert getattr(info, "device_count", 1) == 1
assert getattr(info, "vendor", "").lower() == "amd"
class TestGpuDetectionCache:
"""Tests for GPU detection cache behavior."""
def setup_method(self):
clear_detection_cache()
@patch("ramalama.hardware._run_cmd")
def test_detect_cuda_version_cache_and_clear(self, mock_run_cmd):
from ramalama.hardware import detect_cuda_version
mock_run_cmd.return_value = "12.1"
first = detect_cuda_version()
# Change mocked result, but cached value should be returned
mock_run_cmd.return_value = "13.0"
second = detect_cuda_version()
assert second == first
clear_detection_cache()
# After clearing cache, the new mocked value should be observed
third = detect_cuda_version()
assert third != first
@patch("ramalama.hardware._run_cmd")
def test_detect_rocm_version_cache_and_clear(self, mock_run_cmd):
from ramalama.hardware import detect_rocm_version
mock_run_cmd.return_value = "6.0.0"
first = detect_rocm_version()
mock_run_cmd.return_value = "6.1.0"
second = detect_rocm_version()
assert second == first
clear_detection_cache()
third = detect_rocm_version()
assert third != first
@patch("ramalama.hardware._detect_nvidia")
@patch("ramalama.hardware._detect_asahi")
def test_detect_gpu_cache_and_clear(self, mock_asahi, mock_nvidia):
from ramalama.hardware import detect_gpu, GpuInfo
# First detection result
first_gpu = GpuInfo(
vendor="nvidia",
name="First GPU",
device_count=1,
)
mock_asahi.return_value = None
mock_nvidia.return_value = first_gpu
first = detect_gpu()
# Change underlying detector return value
second_gpu = GpuInfo(
vendor="nvidia",
name="Second GPU",
device_count=2,
)
mock_nvidia.return_value = second_gpu
# Cached result should mask the change
second = detect_gpu()
assert second is first
clear_detection_cache()
# After clearing cache, the new value should be returned
third = detect_gpu()
assert third is second_gpu
class TestGpuDetection:The above edits assume several details that may need to be aligned with your actual implementation:
-
GpuInfo:- The tests currently access
vendor,name, anddevice_countattributes and constructGpuInfo(vendor=..., name=..., device_count=...). - If
GpuInfohas a different constructor signature or different attribute names (e.g.typeinstead ofvendoror uses an enum), adjust the constructor calls and assertions accordingly.
- The tests currently access
-
Vendor-specific detector functions:
- The tests assume the existence of
_detect_nvidia,_detect_asahi, and_detect_amdinramalama.hardware. - If the AMD detector has a different name (e.g.
_detect_rocmor_detect_amd_gpu), update the imports and patches inTestVendorGpuDetectors.test_detect_amd_positive.
- The tests assume the existence of
-
Low-level dependencies:
- The positive detector tests patch
ramalama.hardware._run_cmd,ramalama.hardware.glob.glob, andramalama.hardware.open. - If the detector implementations invoke these helpers under different names or from different modules (e.g.
subprocess.rundirectly orpathlib.Path.glob), update the patch targets to match the real call sites.
- The positive detector tests patch
-
Cache semantics:
- The cache-behavior tests assume that
detect_cuda_version,detect_rocm_version, anddetect_gpueach cache their results and thatclear_detection_cache()clears all of those caches. - If the caching is implemented differently (e.g. per-backend clear functions or separate caches), adjust the tests so they exercise the actual cache invalidation behavior.
- The cache-behavior tests assume that
-
Imports:
- The file-level import edit assumes there is a line
from unittest.mock import patch. If your file already importsMagicMockor has a differently formatted import line, adjust the search/replace soMagicMockis added without duplicating or breaking imports.
- The file-level import edit assumes there is a line
| load_image_matrix, | ||
| select_image, | ||
| ) | ||
| from ramalama.version_constraints import Version |
Contributor
There was a problem hiding this comment.
suggestion (testing): Add tests that cover get_image_matrix_files / get_image_matrix_schema_files discovery behavior
These helpers are central to how load_image_matrix discovers config, but their directory search and precedence rules aren’t tested.
Please add focused tests (e.g. test_config_image_matrix.py) that:
- Use
tmp_pathand monkeypatched base paths to simulateinference-spec/images/,/etc/..., and~/.config/...with differentimages.yaml/images.ymland schema files. - Assert
get_image_matrix_files()returns files in the correct precedence order and only includesimages.(yml|yaml). - Assert
get_image_matrix_schema_files()maps version strings from filenames (e.g.schema.1-0-0.json) to the correct paths.
This will harden the config discovery behavior that the rest of the selection pipeline depends on.
| VersionOperator: TypeAlias = Literal[">=", "<=", "==", ">", "<", "!="] | ||
|
|
||
|
|
||
| @dataclass(frozen=True) |
Contributor
There was a problem hiding this comment.
issue (complexity): Consider replacing the custom Version, VersionConstraint, and VersionRange classes with a single tuple-based helper function that parses versions and evaluates constraint strings directly.
The extra Version, VersionConstraint, and VersionRange types add a lot of surface area for what is effectively “given a driver version and a constraint string, is it compatible?”. You can keep all current behavior (operators, multiple comma-separated constraints, "*" / empty as “any”, None handling) with a single helper and a simple tuple/packaging.version representation.
Two concrete simplifications:
1. Drop Version in favor of tuples (or packaging.version.Version)
Instead of a custom class with from_string, from_tuple, as_tuple, and __str__, treat versions as normalized tuples:
def _parse_version(version_str: str) -> tuple[int, int, int]:
parts = [int(p) for p in version_str.strip().split(".")]
# pad / trim to (major, minor, patch)
parts += [0] * (3 - len(parts))
return tuple(parts[:3])Usage stays the same conceptually:
v1 = _parse_version("12.4")
v2 = _parse_version("12.4.1")
if v1 <= v2:
...That removes an entire custom type without losing any behavior.
2. Replace VersionConstraint/VersionRange with a single matcher
You can fold constraint parsing and matching into one internal function instead of two dataclasses and a mini-DSL:
import re
from typing import Literal
VersionOperator = Literal[">=", "<=", "==", ">", "<", "!="]
_CONSTRAINT_RE = re.compile(r"^(>=|<=|==|>|<|!=)(.+)$")
def matches_version_range(
version: tuple[int, int, int] | None,
range_str: str | None,
) -> bool:
# "*" or empty -> accept any
if not range_str or range_str.strip() == "*":
return True
if version is None:
return False
v1 = version
for part in range_str.split(","):
part = part.strip()
if not part:
continue
m = _CONSTRAINT_RE.match(part)
if not m:
raise ValueError(f"Invalid version constraint: {part}")
op: VersionOperator = m.group(1) # type: ignore[assignment]
v2 = _parse_version(m.group(2))
if op == ">=" and not (v1 >= v2):
return False
if op == "<=" and not (v1 <= v2):
return False
if op == "==" and not (v1 == v2):
return False
if op == ">" and not (v1 > v2):
return False
if op == "<" and not (v1 < v2):
return False
if op == "!=" and not (v1 != v2):
return False
return TrueCall sites that currently construct VersionRange/Version can be updated to:
driver_version = _parse_version(driver_version_str) if driver_version_str else None
if matches_version_range(driver_version, ">=12.4,<13.0"):
...This keeps:
- All current operators (
>=,<=,==,>,<,!=) - Comma-separated conjunction semantics
"*"/ empty as “any version”Nonehandling identical toVersionRange.matches
while removing three public types and their associated mental overhead.
Comment on lines
+84
to
+90
| return subprocess.run( | ||
| args, | ||
| capture_output=True, | ||
| text=True, | ||
| encoding=encoding, | ||
| check=True, | ||
| ) |
Contributor
There was a problem hiding this comment.
security (python.lang.security.audit.dangerous-subprocess-use-audit): Detected subprocess function 'run' without a static string. If this data can be controlled by a malicious actor, it may be an instance of command injection. Audit the use of this call to ensure it is not controllable by an external resource. You may consider using 'shlex.escape()'.
Source: opengrep
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Code Review
This pull request introduces a robust system for automated base image selection using a hardware compatibility matrix. The changes are well-structured across several new modules for hardware detection, version constraint parsing, and image selection logic. The addition of a YAML-based compatibility matrix and corresponding JSON schema is a great way to make the system configurable and maintainable. The new unit tests provide good coverage for the new functionality.
I've found a couple of high-severity issues related to how image tags are detected, which could lead to incorrect image selection when a registry with a port is used. I've also noted a medium-severity issue with overly broad exception handling that could mask bugs. Please see my detailed comments for suggestions.
| return matching[0].image | ||
|
|
||
| def select_image_with_tag( | ||
| self, |
Contributor
There was a problem hiding this comment.
The method used to check for an existing image tag (":" in image) is not robust, as it can be confused by a port in the registry part of the image name (e.g., localhost:5000/my-image). This would cause the function to incorrectly skip applying version-specific or default tags. This logic is repeated in multiple places within this function (e.g., lines 218, 231, 237, 240-242).
A more reliable approach is to check for a colon only in the part of the image name after the repository, for example: ":" in image.split("/")[-1].
Comment on lines
+133
to
+135
| if ":" in image or "@" in image: | ||
| return image | ||
| return f"{image}:{default_tag}" |
Contributor
There was a problem hiding this comment.
The check ":" in image to detect if an image is already tagged is not robust. It will fail for image references that include a port in the registry name, like localhost:5000/my-image. This would cause the function to incorrectly return the image without the default tag.
Suggested change
| if ":" in image or "@" in image: | |
| return image | |
| return f"{image}:{default_tag}" | |
| if "@" in image or ":" in image.split("/")[-1]: | |
| return image | |
| return f"{image}:{default_tag}" |
Comment on lines
+226
to
+227
| except Exception: | ||
| pass |
Contributor
There was a problem hiding this comment.
The broad except Exception: can suppress unexpected errors and make debugging difficult. It's better to catch more specific exceptions that you expect to occur during the sysfs parsing, such as OSError (for file access issues) or ValueError (for parsing issues).
Suggested change
| except Exception: | |
| pass | |
| except (OSError, ValueError): | |
| pass |
olliewalsh
requested changes
Feb 5, 2026
| runtimes: ["llama.cpp"] | ||
| os_types: ["linux"] | ||
| tags: | ||
| # CUDA 12.4-12.7 use the -12.4.1 suffix image |
Collaborator
There was a problem hiding this comment.
where did this list of tags come from? They don't exist.
| gpu_types: ["cuda"] | ||
| driver_version: ">=12.4" | ||
| runtimes: ["llama.cpp"] | ||
| os_types: ["linux"] |
Collaborator
There was a problem hiding this comment.
missing windows in os_types
| # CUDA 12.8+ use the standard image (no suffix needed) | ||
|
|
||
| # ============================================================================= | ||
| # ROCm (AMD) Images |
Collaborator
There was a problem hiding this comment.
ROCm is a bit difficult to handle. We really should default to vulkan instead for GPUs/APU that are not officially supported.
olliewalsh
reviewed
Feb 5, 2026
olliewalsh
left a comment
olliewalsh
left a comment
Collaborator
There was a problem hiding this comment.
The commit need to be signed-off https://git-scm.com/docs/git-commit#Documentation/git-commit.txt--s
| @@ -0,0 +1,203 @@ | |||
| # Image Compatibility Matrix for RamaLama | |||
Collaborator
There was a problem hiding this comment.
Is there a compelling reason to use a yaml config file vs code for this?
Member
|
Also please respond to the AI reviewers. |
Author
|
Closing as duplicate of #2403 (contains latest review fixes). |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
3 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
This is for issue #2384.
Implemented automated base image selection based on hardware configuration:
This removes the need for manual base image specification in non-typical configurations.
Summary by Sourcery
Introduce an automated container image selection system driven by hardware detection and a compatibility matrix, replacing manual image configuration for most environments.
New Features:
Enhancements:
Documentation:
Tests: