adding the MPS version of running_compute_processes for >= Volta. #112
Conversation
nvml-wrapper/src/device.rs
Outdated
|
|
||
| nvml_try(sym(self.device, &mut hints, ptr::null_mut()))?; | ||
|
|
||
| hints.checked_shr(1); |
There was a problem hiding this comment.
This seems like a typo. Did you mean to do this instead? I'm not familiar enough with this method to know if we should be shifting the hint flags here.
hints = hints.checked_shr(1);There was a problem hiding this comment.
No I just forgot to treat this part. You re almost right, since it s the checked flavor it Option<Self> return type
devnexen
force-pushed
the
mps_compute_running_processes
branch
2 times, most recently
from
a97ff1b to
3b221e2
Compare
swlynch99
left a comment
swlynch99
left a comment
There was a problem hiding this comment.
After looking at bit more closely this seems completely wrong. See my review comments for what needs to be fixed.
The errors here are a bit weird to me. If you're using AI to generate these PRs that's completely fine, but please validate that the resulting code is using the APIs correctly by looking at the docs before submitting them.
| unsafe { | ||
| let mut hints: c_uint = 0; | ||
|
|
||
| nvml_try(sym(self.device, &mut hints, ptr::null_mut()))?; | ||
|
|
||
| let mut hints_shr = match hints.checked_shr(1) { | ||
| Some(h) => h, | ||
| None => hints, | ||
| }; | ||
| let mut processes: Vec<nvmlProcessInfo_t> = Vec::with_capacity(hints_shr as usize); | ||
|
|
||
| nvml_try(sym(self.device, &mut hints_shr, processes.as_mut_ptr()))?; | ||
|
|
||
| processes.truncate(hints_shr as usize); | ||
| Ok(processes.into_iter().map(ProcessInfo::from).collect()) | ||
| } |
There was a problem hiding this comment.
The bit with checked_shr confused me so I went and took a look at the docs to confirm. This section is wrong.
The way this method is supposed to be used seems to be like this:
- Call the function with
countset to 0 andinfosset to NULL - If the call doesn't error then we're done and know there are no processes, so can return an empty
Vec - Otherwise it should return
NVML_ERROR_INSUFFICIENT_SIZEand updatecountto the number of processes - You should then be using count to allocate an array and passing that in for a second call so that it stores the data into the array.
- Finally, truncate does not extend vector, you need
set_lenfor that.
So in order:
hintsis absolutely the wrong variable name. Uselen.- Drop all the shifting, it makes no sense.
- You need to handle the relevant errors from the first call to
nvmlDeviceGetMPSComputeRunningProcesses_v3. If it succeeds you should just be returning an emptyVec. truncatedoes not do what you want here, useset_leninstead.
nvml-wrapper/src/device.rs
Outdated
| Supports Volta or newer fully supported devices. | ||
| */ | ||
| #[doc(alias = "nvmlDeviceGetiMPSComputeRunningProcesses_v3")] |
There was a problem hiding this comment.
You've got a typo here
| #[doc(alias = "nvmlDeviceGetiMPSComputeRunningProcesses_v3")] | |
| #[doc(alias = "nvmlDeviceGetMPSComputeRunningProcesses_v3")] |
devnexen
force-pushed
the
mps_compute_running_processes
branch
from
3b221e2 to
94f8239
Compare
2 participants
No description provided.