Repository navigation
Conversation
Seed the field status with an invalid sentinel so a successful native no-op cannot accidentally pass on recycled memory. Xfail when Orin leaves the sentinel unchanged, preserving normal checks on fixed drivers and other devices. Label the workaround explicitly in the source comment and xfail reason.
|
Auto-sync is disabled for draft pull requests in this repository. Workflows must be run manually. Contributors can view more details about this message here. |
github-actions
Bot
added
the
cuda.bindings
|
Important Review skippedReview was skipped as selected files did not have any reviewable changes. ⚙️ Run configuration
You can disable this status message by setting the Use the checkbox below for a quick retry:
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info
📝 Summary
Priority: ⬇️ Low Change: Bug fix Merge Risk: ⚪ Minimal · up to
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
ℹ️ Review info
⚙️ Run configuration
- Configuration used: Repository: NVIDIA/cuda-python/.coderabbit.yaml
- Review profile: CHILL
- Plan: Enterprise
- Run ID:
85c60964-6f71-40be-90e0-cf913a2bf0c1
📒 Files selected for processing: 1
cuda_bindings/tests/nvml/test_nvlink.py
Included review availability: This review used your included allowance. Your plan provides up to 12 included reviews per hour; 11 remain after this review.
| fields[0].nvml_return = -1 | ||
| value = nvml.device_get_field_values(device, fields)[0] | ||
| if value.nvml_return == -1 and "Orin" in nvml.device_get_name(device): | ||
| pytest.xfail( |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
🔎 Supported by static analysis
🏁 Script executed:
set -eu
printf '%s\n' '--- reviewed head test ---'
git show 412659b856b8c1d7086bdef9351f66e3b4cecabd:cuda_bindings/tests/nvml/test_nvlink.py | nl -ba | sed -n '1,90p'
printf '%s\n' '--- base test ---'
git show 576ef68874f2fcbbb00f7f2302960c3b20597164:cuda_bindings/tests/nvml/test_nvlink.py | nl -ba | sed -n '1,90p'
printf '%s\n' '--- diff ---'
git diff --unified=30 576ef68874f2fcbbb00f7f2302960c3b20597164 412659b856b8c1d7086bdef9351f66e3b4cecabd -- cuda_bindings/tests/nvml/test_nvlink.pyRepository: NVIDIA/cuda-python
Length of output: 5440
important: Defer the Orin xfail only after bypassing its sentinel assertion.
When the Orin sentinel remains, moving pytest.xfail() after the loop still reaches the first return-code assertion, which rejects -1 before the alternate query and later devices run. Record the Orin condition, skip only that assertion, keep the alternate query active, and call pytest.xfail() after the loop.
Suggested fix
def test_nvlink_get_link_count(all_devices):
"""
Checks that the link count of the device is same.
"""
+ orin_nvml_bug = False
for device in all_devices:
...
value = nvml.device_get_field_values(device, fields)[0]
if value.nvml_return == -1 and "Orin" in nvml.device_get_name(device):
- pytest.xfail(
- "Orin NVML nvbug 6952048: nvmlDeviceGetFieldValues returns success without populating the result"
- )
- assert value.nvml_return == nvml.Return.SUCCESS or value.nvml_return == nvml.Return.ERROR_NOT_SUPPORTED, (
- f"Unexpected return {value.nvml_return} for link count field query"
- )
+ orin_nvml_bug = True
+ else:
+ assert value.nvml_return == nvml.Return.SUCCESS or value.nvml_return == nvml.Return.ERROR_NOT_SUPPORTED, (
+ f"Unexpected return {value.nvml_return} for link count field query"
+ )
...
assert value.nvml_return == nvml.Return.SUCCESS or value.nvml_return == nvml.Return.ERROR_NOT_SUPPORTED, (
f"Unexpected return {value.nvml_return} for link count field query"
)
+
+ if orin_nvml_bug:
+ pytest.xfail(
+ "Orin NVML nvbug 6952048: nvmlDeviceGetFieldValues returns success without populating the result"
+ )
|
mdboom
left a comment
mdboom
left a comment
There was a problem hiding this comment.
I don't think we should just workaround this in a test. Seems like a bug that we should protect our users from. That is, we should set an error code inside of the device_get_field_values function and populate the error back out.
rwgk
changed the title
|
/ok to test fbf8f40 |
|
@mdboom is this the direction you had in mind? |
|
Orin @ fbf8f40: |
mdboom
left a comment
mdboom
left a comment
There was a problem hiding this comment.
This LGTM, but let's merge the generator side first then this.
I wouldn't include the test_field_values.py file at all. It's mostly just testing that a mock works and isn't really testing anything in practice about upstream NVML. I worry it will just fail at some unrelated moment in the future. The changes to the existing test in test_nvlink.py looks much better and I think covers what we need to.
Generated from cybind commit adf423398b177ab19e017b33173178ceee36157f. Use the FieldValue array property to initialize per-query errors, incorporating the review suggestion from cybind MR 591.
…→work_around_orin_nvml_field_values_bug # Conflicts: # cuda_bindings_13/tests/nvml/test_field_values.py
|
Orin @ 19089f0: |
|
Attempting to deflake: Orin @ 19089f0 attempt 2: |
|
Attempting to deflake: Orin @ 19089f0 attempt 3: |
|
Both reruns reproduce the same IPC export failure, and each also has one teardown error. The teardown error is very likely fallout from the export failure.
The original graph-capture failure passes in both reruns. Both NVLink input forms also continue to pass in both stream modes. The primary failure has a revealing exception chain:
This strongly suggests a native CUDA export error-reporting defect under descriptor exhaustion, alongside a cuda-core cleanup bug. A small native reproducer should confirm the exact CUDA return/output pair. The teardown errors have a consistent explanation. The unexpected exception bypasses the FD-limit test’s buffer-closing loop, leaving three virtual-memory reservations alive. That cleanup sits after the In all three runs, the first later test that explicitly invokes garbage collection releases exactly those three older reservations. The fixture then reports a baseline-resource release during teardown. Changing the random seed changes which test receives the error. Traceback retention is the likely mechanism, although the logs do not identify the retaining references directly. I would investigate this separately from PR 3148: the IPC code is unchanged by that PR. The useful next steps are an isolated Orin reproduction of the FD-limit test, a native export probe, and cleanup fixes that preserve the original exception and close buffers on every exit. Further whole-suite reruns are unlikely to resolve this repeatable failure. |
|
@mdboom Both MR 591 and this PR are merge-ready. Unfortunately there is a new, unrelated failure on Orin. I'll work on that separately. |
This branch has not been deployed
2 participants
Description
Protect
nvml.device_get_field_valuescallers when native NVML returns aggregate success without populating an output entry. Initialize each entry's status toERROR_UNKNOWNbefore every query, including queries that reuse a caller-provided buffer. Preserve native-written results and statuses, partial-batch results, and existing aggregate-error exceptions.Default the scope to zero for plain integer field IDs while preserving explicit scopes. Regenerate the NVML binding from the cybind template.
Update the existing hardware NVLink test to exercise both caller-provided buffers and integer field-ID lists, including empty requests. Accept the defined fallback only for the known Orin case.
Validation
Jetson Orin hardware retest at
19089f0db52909b7b78684c9a6611db72c4162b0on 2026-10-21 UTC: Linux aarch64, CUDA 13.4 / nvcc 13.4.59, driver 630.03, CPython 3.12.3. Posted run summary.The build from
cuda_bindings_13completed successfully (exit status 0).test_nvlink_get_link_countpassed with both caller-provided buffers and integer field-ID lists in both stream modes.The full QA command exited with status 1 because of these cuda-core results:
TestFileDescriptorLimit::test_export_near_the_limit:OSError: [Errno 9] Bad file descriptor.test_dmr_check_capture_state[global]:CUDA_ERROR_STREAM_CAPTURE_INVALIDATED.TestManyChunkBuffer::test_mainteardown:BaselineMemoryResourceReleasedWarningfor release of three pre-existing virtual-address reservations.The failing traces contain no NVML frames; this run alone does not establish whether the failures are pre-existing.
Additional local validation on NVIDIA A10G / driver 620.43 / CUDA 13.4 / CPython 3.12.3: successful rebuild from the renamed source root; 6 remaining NVLink/core tests and 2 subtests passed. Cybind template/output tests: 35 passed. Repository-wide pre-commit checks passed.
References: