Add detailed GPU hardware detection reporting and consolidate logic - #2782
Add detailed GPU hardware detection reporting and consolidate logic#2782olliewalsh wants to merge 3 commits into
Conversation
Signed-off-by: Oliver Walsh <owalsh@redhat.com>
Consolidate duplicated GPU detection logic so hw_detect.py is the single source of truth for hardware detection, and extract all acceleration selection code (check_*, get_accel, CDI validation, accel_image) from common.py into a focused accel.py module. - hw_detect.py: extend DeviceInfo with index/uuid fields, add per-detector cached functions, use Vulkan API directly via ctypes instead of parsing vulkaninfo output, add ARM guard to AmdRocmDetector, add Intel iGPU device IDs - accel.py: new module with GPU selection, env var setup, CDI validation, and container image selection (moved from common.py) - common.py: shed ~250 lines of acceleration code, now general utilities - Update 14 import sites across the codebase - Split acceleration tests into test_accel.py Signed-off-by: Oliver Walsh <owalsh@redhat.com>
GitHub Actions
Inactive
|
Important Review skippedDraft detected. Please check the settings in the CodeRabbit UI or the ⚙️ Run configurationConfiguration used: Repository UI Review profile: CHILL Plan: Pro Run ID: You can disable this status message by setting the Use the checkbox below for a quick retry:
✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request refactors hardware detection and accelerator configuration by moving accelerator logic from ramalama/common.py to ramalama/accel.py and hardware detection to ramalama/hw_detect.py. It also enhances the CLI to display detailed accelerator information. The review feedback highlights several robustness improvements and potential crash fixes, including wrapping get_podman_machine_cdi_config in a try...except block, validating JSON structures in MthreadsDetector to prevent AttributeError crashes, stripping whitespace from CUDA_VISIBLE_DEVICES, specifying explicit file encodings, handling UnicodeDecodeError when reading /proc/cpuinfo, and using globals() for cleaner dynamic function lookups.
Important
The consumer version of Gemini Code Assist on GitHub is being sunset. Starting June 18, 2026, new organization installations will be blocked, and all code review activity will officially cease on July 17, 2026.
For more details on the timeline and next steps, please review the Help Documentation.
| try: | ||
| import json | ||
|
|
||
| data = json.loads(result.stdout) | ||
| except (ValueError, KeyError): | ||
| return [] |
There was a problem hiding this comment.
If data is not a dictionary (e.g., if mthreads-gmi returned a JSON array or a JSON string/number), data.get will raise an AttributeError outside the try...except block and crash the program. Validate that data is a dictionary before proceeding.
| try: | |
| import json | |
| data = json.loads(result.stdout) | |
| except (ValueError, KeyError): | |
| return [] | |
| try: | |
| import json | |
| data = json.loads(result.stdout) | |
| if not isinstance(data, dict): | |
| return [] | |
| except (ValueError, KeyError): | |
| return [] |
| cuda_visible_devices = os.environ.get("CUDA_VISIBLE_DEVICES", "") | ||
| visible_devices = cuda_visible_devices.split(',') if cuda_visible_devices else [] | ||
| for device in visible_devices: | ||
| if device not in indices and not any(uuid.startswith(device) for uuid in uuids): | ||
| perror(f"{device} not found") | ||
| return None |
There was a problem hiding this comment.
If CUDA_VISIBLE_DEVICES contains spaces (e.g., "0, 1"), splitting by "," will result in leading/trailing whitespace in the device identifiers (e.g., " 1"). This will cause the device lookup to fail. Strip whitespace from each device identifier to make the environment variable parsing more robust.
| cuda_visible_devices = os.environ.get("CUDA_VISIBLE_DEVICES", "") | |
| visible_devices = cuda_visible_devices.split(',') if cuda_visible_devices else [] | |
| for device in visible_devices: | |
| if device not in indices and not any(uuid.startswith(device) for uuid in uuids): | |
| perror(f"{device} not found") | |
| return None | |
| cuda_visible_devices = os.environ.get("CUDA_VISIBLE_DEVICES", "") | |
| visible_devices = [d.strip() for d in cuda_visible_devices.split(',')] if cuda_visible_devices else [] | |
| for device in visible_devices: | |
| if device not in indices and not any(uuid.startswith(device) for uuid in uuids): | |
| perror(f"{device} not found") | |
| return None |
| with open(file_path, "r") as stream: | ||
| if ext == ".json": | ||
| config = json.load(stream) | ||
| else: | ||
| config = yaml.safe_load(stream) |
There was a problem hiding this comment.
When opening files, it is highly recommended to specify an explicit encoding (such as encoding="utf-8") to ensure consistent behavior across different platforms (especially Windows), rather than relying on the system default locale encoding.
| with open(file_path, "r") as stream: | |
| if ext == ".json": | |
| config = json.load(stream) | |
| else: | |
| config = yaml.safe_load(stream) | |
| with open(file_path, "r", encoding="utf-8") as stream: | |
| if ext == ".json": | |
| config = json.load(stream) | |
| else: | |
| config = yaml.safe_load(stream) |
| try: | ||
| with open("/proc/cpuinfo", "r") as f: | ||
| for line in f: | ||
| if line.startswith("model name"): | ||
| return line.split(":", 1)[1].strip() |
There was a problem hiding this comment.
Reading /proc/cpuinfo can raise a UnicodeDecodeError if it contains non-ASCII characters on systems with non-UTF-8 default encodings. Since UnicodeDecodeError inherits from ValueError (not OSError), it will not be caught by except OSError. Use errors="replace" to prevent potential crashes.
with open("/proc/cpuinfo", "r", errors="replace") as f:
for line in f:
if line.startswith("model name") and ":" in line:
return line.split(":", 1)[1].strip()| for gpu in data.get("GPU", []): | ||
| name = gpu.get("Product Name", "Moore Threads GPU") | ||
| total_str = gpu.get("FB Memory Usage", {}).get("Total", "") | ||
| total_bytes = self._parse_memory_string(total_str) | ||
| if total_bytes > 0: | ||
| devices.append(DeviceInfo(name=name or "Moore Threads GPU", memory_bytes=total_bytes)) |
There was a problem hiding this comment.
If gpu.get("FB Memory Usage") returns None or a non-dictionary value (e.g., if the field is null or malformed in the JSON output), calling .get("Total") on it will raise an AttributeError and crash the detector. Add a type check to ensure it is a dictionary before calling .get().
fb_usage = gpu.get("FB Memory Usage")
total_str = fb_usage.get("Total", "") if isinstance(fb_usage, dict) else ""
total_bytes = self._parse_memory_string(total_str)
if total_bytes > 0:
devices.append(DeviceInfo(name=name or "Moore Threads GPU", memory_bytes=total_bytes))| def detect_all_hardware() -> tuple[AcceleratorInfo, ...]: | ||
| import ramalama.hw_detect as _self | ||
|
|
||
| results: list[AcceleratorInfo] = [] | ||
| for accel_type, fn_name in _DETECTOR_NAMES: | ||
| try: | ||
| fn: Callable[[], list[DeviceInfo]] = getattr(_self, fn_name) | ||
| devices = fn() | ||
| if devices: | ||
| total = sum(d.memory_bytes for d in devices) | ||
| results.append(AcceleratorInfo(accel_type=accel_type, devices=devices, total_memory_bytes=total)) | ||
| except Exception as e: | ||
| logger.debug(f"Hardware detection failed for {accel_type}: {e}") |
There was a problem hiding this comment.
Instead of dynamically looking up detector functions by name using getattr on a self-imported module reference, you can reference the function objects directly from globals(). This is much cleaner, more idiomatic, and avoids unnecessary self-imports.
| def detect_all_hardware() -> tuple[AcceleratorInfo, ...]: | |
| import ramalama.hw_detect as _self | |
| results: list[AcceleratorInfo] = [] | |
| for accel_type, fn_name in _DETECTOR_NAMES: | |
| try: | |
| fn: Callable[[], list[DeviceInfo]] = getattr(_self, fn_name) | |
| devices = fn() | |
| if devices: | |
| total = sum(d.memory_bytes for d in devices) | |
| results.append(AcceleratorInfo(accel_type=accel_type, devices=devices, total_memory_bytes=total)) | |
| except Exception as e: | |
| logger.debug(f"Hardware detection failed for {accel_type}: {e}") | |
| @lru_cache(maxsize=1) | |
| def detect_all_hardware() -> tuple[AcceleratorInfo, ...]: | |
| results: list[AcceleratorInfo] = [] | |
| for accel_type, fn_name in _DETECTOR_NAMES: | |
| try: | |
| fn: Callable[[], list[DeviceInfo]] = globals()[fn_name] | |
| devices = fn() | |
| if devices: | |
| total = sum(d.memory_bytes for d in devices) | |
| results.append(AcceleratorInfo(accel_type=accel_type, devices=devices, total_memory_bytes=total)) | |
| except Exception as e: | |
| logger.debug(f"Hardware detection failed for {accel_type}: {e}") |
|
Output on f44 RX9070: |
|
f43 AMD iGPU: |
|
f44 NVIDIA: |
|
@bmahabirbu WDYT? |
|
W11 9070XT: |
Signed-off-by: Oliver Walsh <owalsh@redhat.com>
GitHub Actions
Inactive
|
Win11 NVIDIA: |
|
Gave it a test works great on linux too detectred by cpu and gpu 9800x3d and gpu 9070xt |
|
|
||
| devices = detect_intel() | ||
| if devices: | ||
| os.environ["INTEL_VISIBLE_DEVICES"] = str(len(devices)) |
There was a problem hiding this comment.
according to claude "check_intel sets INTEL_VISIBLE_DEVICES to the count (accel.py:196), not a device index. This was
the old behavior too, but it's inconsistent with every other check_* which sets an index or device
list.
"
|
Helped from claude for the review No vulkan check in the get_accel() chain. If a system has a GPU only detectable via vulkan (no |
|
A friendly reminder that this PR had no activity for 30 days. |
|
@olliewalsh still working on this one? |
3 participants
Add multi-device hardware detection for ramalama info, reporting all GPU devices/interfaces with memory details
Unify GPU detection behind hw_detect.py as the single source of truth, remove detection logic in common.py
Extract acceleration selection code into focused accel.py module (check_*, get_accel, CDI validation, accel_image)
Use the Vulkan C API directly via ctypes instead of parsing vulkaninfo text output
Look up AMD GPU product names from the system PCI IDs database when the KFD driver reports a generic name
The ultimate goal is to use this data to help find suitable models/quants for the available hardware.