Fix NVML tests for machines with more than one device - #1352
Conversation
Sorry, something went wrong.
|
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. |
Sorry, something went wrong.
|
/ok to test |
Sorry, something went wrong.
|
Sorry, something went wrong.
rwgk
left a comment
There was a problem hiding this comment.
LGTM although I'd try to preserve the skip information. I think it's worth a small effort, so that we can reason about what we're actually testing. E.g. where you have the continue, maybe build a defaultdict(int) with the current skip messages as keys; when the end of a subtest is reached, check if the dict is empty, if not call pytest.skip with the key/counts in the message.
I'd try to give that as a prompt to Cursor. Good chance it'll "just do it".
Sorry, something went wrong.
|
/ok to test |
Sorry, something went wrong.
|
Good idea.
I did one of these, and VSCode/Copilot was able to infer the rest. |
Sorry, something went wrong.
rwgk
left a comment
There was a problem hiding this comment.
The TestIpcReexport.test_main segfault is interesting. @Andy-Jost for awareness:
https://github.com/NVIDIA/cuda-python/actions/runs/20083999706/job/57617779182?pr=1352
I expect a rerun will deflake that.
Sorry, something went wrong.
Removed preview folders for the following PRs: - PR #1352
* Add experimental NVML bindings to 12.9.x branch * Add tests from main to 12.9.x branch * Remove newer APIs * Remove hand-written 13.0 bindings * Queue up 'skip reasons' (#1352) * Handle backport correctly * Fix cimport * More versioned struct fixes * Fix test for 12.9 * Add 13.1 bindings
Description
As @rwgk discovered, all of the tests using the for_all_devices fixture fail with fixture function has more than one 'yield'. I didn't realize that wasn't allowed. This changes the fixture to just yield the whole list of devices.