fix(detector): stop AMD SMI's "N/A" sentinel from reaching numeric device fields - #23
Conversation
- add a _get_reading helper mapping an AMD SMI reading to a value or a default, so the library's in-band "N/A" sentinel is handled in one place - route a sentinel used power to the ROCm SMI fallback, the same way a failed AMD SMI call already does, instead of reporting the string - take the sentinel as absent for the cores utilization and the temperature, letting the existing "unreadable utilization is 0" guard do its job Task 1 of amd-na-sentinel-leaks-into-numeric-fields. Signed-off-by: thxCode <thxcode0824@gmail.com>
- fall back to the ROCm SMI power cap when the power limit is unreadable, instead of floor-dividing a string and raising a TypeError that no AmdSmiException handler catches, which costs the host every AMD card - report no driver version, rather than the sentinel, when AMD SMI cannot read it - stop naming a board after the sentinel, the ASIC market name being the last link of the name chain Task 2 of amd-na-sentinel-leaks-into-numeric-fields. Signed-off-by: thxCode <thxcode0824@gmail.com>
- wrap the busy percent, the temperature and the used power on their own, mirroring the AMD path: ROCm SMI raises on a reading it cannot serve, and an unwrapped call cost the whole sweep rather than the reading, leaving every card on the host with its information-query values Task 4a of amd-na-sentinel-leaks-into-numeric-fields. Signed-off-by: thxCode <thxcode0824@gmail.com>
There was a problem hiding this comment.
Code Review
This pull request improves the robustness of AMD and Hygon GPU detectors by safely handling sentinel values (like "N/A") and isolating individual telemetry queries to prevent a single failure from crashing the entire detection sweep. The review feedback suggests defensive type-checking for power limit values to avoid a potential TypeError and ensuring the _get_reading helper handles non-dictionary inputs to prevent AttributeError crashes.
| dev_power_limit = _get_reading(dev_power_info, "power_limit") | ||
| if dev_power_limit is not None: | ||
| dev_power = dev_power_limit // 1000000 # uW to W |
There was a problem hiding this comment.
Although the code checks if dev_power_limit is not None, it does not verify that the returned value is actually a numeric type (int or float). If dev_power_limit is an unexpected string or other non-numeric type, performing floor division (//) will raise a TypeError. Since TypeError is not suppressed by contextlib.suppress(pyamdsmi.AmdSmiException), it will propagate and crash the detector. Checking the type defensively prevents this potential crash.
| dev_power_limit = _get_reading(dev_power_info, "power_limit") | |
| if dev_power_limit is not None: | |
| dev_power = dev_power_limit // 1000000 # uW to W | |
| dev_power_limit = _get_reading(dev_power_info, "power_limit") | |
| if isinstance(dev_power_limit, (int, float)): | |
| dev_power = int(dev_power_limit) // 1000000 # uW to W |
| def _get_reading(dev_info: dict, key: str, default: Any = None) -> Any: | ||
| """ | ||
| Read one value out of an AMD SMI answer, treating its sentinel as absent. | ||
|
|
||
| AMD SMI reports an unavailable reading in-band: it rewrites the 0xFFFF (or | ||
| max-uint) the driver answered into the string "N/A" before returning, so a | ||
| dict otherwise holding numbers can carry a string at any key. A VF is where | ||
| this surfaces, its host-side telemetry being invisible to the guest. | ||
|
|
||
| Args: | ||
| dev_info: | ||
| The answer AMD SMI returned. | ||
| key: | ||
| The reading to take. | ||
| default: | ||
| What an absent or unavailable reading becomes. | ||
|
|
||
| Returns: | ||
| The reading, or the default. | ||
|
|
||
| """ | ||
| dev_reading = dev_info.get(key, "N/A") | ||
| return default if dev_reading == "N/A" else dev_reading |
There was a problem hiding this comment.
The _get_reading helper function assumes that dev_info is always a dictionary and calls .get() on it. If dev_info is None or not a dictionary (which can happen if the underlying SMI call fails or returns an unexpected type), it will raise an AttributeError. Since this exception is not caught by specific handlers, it can propagate and crash the entire detector, causing the host to lose all AMD cards. Making this helper defensive against non-dictionary inputs improves robustness.
def _get_reading(dev_info: dict | None, key: str, default: Any = None) -> Any:
"""
Read one value out of an AMD SMI answer, treating its sentinel as absent.
AMD SMI reports an unavailable reading in-band: it rewrites the 0xFFFF (or
max-uint) the driver answered into the string "N/A" before returning, so a
dict otherwise holding numbers can carry a string at any key. A VF is where
this surfaces, its host-side telemetry being invisible to the guest.
Args:
dev_info:
The answer AMD SMI returned.
key:
The reading to take.
default:
What an absent or unavailable reading becomes.
Returns:
The reading, or the default.
"""
if not isinstance(dev_info, dict):
return default
dev_reading = dev_info.get(key, "N/A")
return default if dev_reading == "N/A" else dev_reading
What
gpustack-runtime detect --format jsonon an AMD Instinct MI300X VF emitted"power_used": "N/A", which the consumer cannot parse:pyamdsmiis not an in-house ctypes binding — it isfrom amdsmi import *, a bridge over AMD's official Python library. That library reports an unavailable reading in-band, rewriting the0xFFFF/ max-uint the driver answered into the string"N/A"inside dicts that otherwise hold numbers (amdsmi_interface.py:2594for the whole power dict,:698_validate_if_max_uintfor the metrics). On a VF the host-side power telemetry is not exposed to the guest, the driver answers0xFFFF, and amdsmi hands over"N/A". The detector defended against this convention at only 2 of 8 read sites and took every other reading at face value._get_reading()— one private helper inamd.pymapping an AMD SMI reading to a value or a default, so the convention lives in one place instead of at six call sites. It is deliberately notsafe_int/safe_str: those coerce, and this must be able to answer "absent" so a fallback can run.current_socket_power/average_socket_powernow routes to the ROCm SMI fallback exactly the way a failed AMD SMI call already did (both mean "AMD SMI cannot tell the used power"); a sentinelaverage_gfx_activity/temperature_hotspotcounts as absent, letting the existing "unreadable utilization is 0" guard do its job. Usesis None, so a genuine 0 W reading is not mistaken for a missing one.power_limitfalls back torsmi_dev_power_cap_getinstead of being floor-divided. This was worse than the reported symptom:"N/A" // 1000000raisesTypeError, which is not anAmdSmiException, so it escaped the handler and dropped the vendor — a host loses every AMD card. This machine's limit happened to be valid (750 W), which is the only reason it surfaced as a badpower_usedinstead. A sentineldriver_version/market_nameis likewise no longer stored as a version or a board name.pyrocmsmichecks every return code and raises instead of answering a sentinel, but the busy-percent, temperature and used-power reads were unwrapped where the AMD path wraps each equivalent call, so one unreadable metric raised out ofdetect_usageand cost the whole batch. Each is now isolated on its own. Isolation only — no sentinel checks added.Not a regression.
git log -Sputs every one of these readings inf64a307 refactor: support amd detection(2025-09-23), the commit that introduced AMD support, in the form they still had.518db70(#17) only moved the block fromdetect()intodetect_usage().Other vendors are unaffected. Of the twelve bindings only three bridge a third-party library:
pyamdsmi(the sentinel),pynvmlandpymtml. pynvml's"N/A"occurrences all live inpynvml_utils/smi.py, the nvidia-smi table renderer; the core API this project calls signals unavailability by raisingNVMLError. The other nine are in-housectypes/CDLLbindings that return numbers and raise on failure, so they cannot produce a string sentinel.Test plan
uv run pytest tests/— 613 passed, 20 skipped (was 605/20; 8 new guards). All new guards were re-run againstmain's source and all 7 code-path guards fail there, so none is vacuous.test_amd.py— the four leak paths: both socket readings"N/A"(falls back, then reportsNonewhen ROCm SMI also cannot read it),power_limit"N/A"(noTypeError, falls back to the 700 W cap), metrics"N/A"(cores_utilization == 0,temperature is None),driver_version/market_name"N/A".test_hygon.py— an unreadable power is bounded to its own card while the rest of that card's usage and the healthy card are untouched; an unreadable utilization does not carry off the temperature.Nonefor a reading stands for the one the binding cannot answer, and raises.Confirmed on the reported hardware (
root@134.199.204.73, container02132c6df882, AMD Instinct MI300X VF, driver 6.19.14.31400000). The container'samd.pywas byte-identical to this branch's base, so the fixed file was dropped in and the container restarted:detect --format json"power_used": "N/A""power_used": nullgpustack.worker.collectorfloat_parsingfailures, one per 5 s pollworker_node_gpu_info,gpu_cores304,gpu_power_limit_watts750ROCm SMI cannot read the power on this VF either, which is why the field lands on
nullrather than a number — the intended outcome. The host was left running the patch; it does not survive recreating the container from the image.Notes
The operator carries the same defect against the same hardware fact, in its own shape:
binding/amdsmiis an in-house cgo binding, so it sees the raw integer and never a"N/A"string — quieter, not safer. Nothing crashes and65535is reported as a real measurement into monitoring and scheduling (PowerUsageW,CoresUtilization%,Temperature°C). That fix is out of scope here and is being made separately ingpustack.ai/gpustack, where the same block sits atpkg/devicemanager/detector/amd/device.go:315,316,323-325. Its Hygon detector already checksretper reading and needs no change.One asymmetry worth recording: the Python side gets an accidental layer of cover the Go side lacks — amdsmi's
_validate_if_max_uint(..., isActivity=True)also rejects activity readings above 100.