NVML: Change Inforom-related APIs from str to bytes #1570
Conversation
|
Auto-sync is disabled for ready for review pull requests in this repository. Workflows must be run manually. Contributors can view more details about this message here. |
mdboom
added
cuda.bindings
|
/ok to test |
There was a problem hiding this comment.
Pull request overview
This PR changes three NVML Inforom-related APIs from returning str to bytes based on guidance from the NVML team that these APIs should preserve embedded NULL characters.
Changes:
- Modified return types from
strtobytesfor Inforom version and board part number APIs - Updated low-level bindings to use
PyBytes_FromStringAndSizeinstead ofPyUnicode_FromString - Updated type hints and test assertions to reflect the new
bytesreturn type
Reviewed changes
Copilot reviewed 4 out of 4 changed files in this pull request and generated 3 comments.
| File | Description |
|---|---|
| cuda_bindings/cuda/bindings/nvml.pxd | Updated function signatures to return bytes instead of str |
| cuda_bindings/cuda/bindings/nvml.pyx | Changed implementation to use PyBytes_FromStringAndSize with full buffer sizes |
| cuda_core/cuda/core/system/_inforom.pxi | Updated type hints to indicate bytes return type |
| cuda_core/tests/system/test_system_device.py | Updated test assertions to check for bytes type instead of str |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| __status__ = nvmlDeviceGetBoardPartNumber(<Device>device, part_number, length) | ||
| check_status(__status__) | ||
| return cpython.PyUnicode_FromString(part_number) | ||
| return cpython.PyBytes_FromStringAndSize(part_number, 80) |
There was a problem hiding this comment.
Using PyBytes_FromStringAndSize with the full buffer size (80) may include uninitialized or garbage data beyond the actual string content returned by the NVML API. Consider using the actual string length (e.g., via strlen) instead of the hardcoded buffer size, unless the NVML API guarantees that all 80 bytes are meaningful. If embedded NULLs need to be preserved, you could use strlen to find the length up to the first NULL, or modify the approach based on confirmed NVML API behavior.
| __status__ = nvmlDeviceGetInforomVersion(<Device>device, <_InforomObject>object, version, length) | ||
| check_status(__status__) | ||
| return cpython.PyUnicode_FromString(version) | ||
| return cpython.PyBytes_FromStringAndSize(version, 16) |
There was a problem hiding this comment.
Using PyBytes_FromStringAndSize with the full buffer size (16) may include uninitialized or garbage data beyond the actual string content returned by the NVML API. Consider using the actual string length (e.g., via strlen) instead of the hardcoded buffer size, unless the NVML API guarantees that all 16 bytes are meaningful. If embedded NULLs need to be preserved, you could use strlen to find the length up to the first NULL, or modify the approach based on confirmed NVML API behavior.
| __status__ = nvmlDeviceGetInforomImageVersion(<Device>device, version, length) | ||
| check_status(__status__) | ||
| return cpython.PyUnicode_FromString(version) | ||
| return cpython.PyBytes_FromStringAndSize(version, 16) |
There was a problem hiding this comment.
Using PyBytes_FromStringAndSize with the full buffer size (16) may include uninitialized or garbage data beyond the actual string content returned by the NVML API. Consider using the actual string length (e.g., via strlen) instead of the hardcoded buffer size, unless the NVML API guarantees that all 16 bytes are meaningful. If embedded NULLs need to be preserved, you could use strlen to find the length up to the first NULL, or modify the approach based on confirmed NVML API behavior.
|
cpcloud
left a comment
cpcloud
left a comment
There was a problem hiding this comment.
LGTM, where are the specific sizes coming from?
3 participants
Based on a discussion elsewhere with the NVML team, it seems that these APIs should really be
bytes(containing embedded NULL characters), notstr.