nvml-wrapper icon indicating copy to clipboard operation
nvml-wrapper copied to clipboard

Support for querying scoped fields

Open bsteinb opened this issue 2 years ago • 2 comments

The current implementation of Device::field_values_for is too limiting. The caller can only pass in a slice of field IDs to populate nvmlFieldValue_t::fieldId while all other struct members are set to zero, while nvmlDeviceGetFieldValues actually also uses nvmlFieldValue_t::scopeId as input when querying several fields such as NVLink remote IDs, NVLink ECC counters, or power draw and power limits.

I would like to contribute a function that allows passing in both fieldId and scopeId and was wondering whether that should replace Device::field_values_for or be a separate function, e.g. Device::scoped_field_values_for.

bsteinb avatar Jan 05 '24 14:01 bsteinb

Thanks for the heads up, looks like scopeId was added after I initially created that wrapper 🙂. We should definitely take that into account.

What do you think about changing:

pub fn field_values_for(
    &self,
    id_slice: &[FieldId],
) -> Result<Vec<Result<FieldValueSample, NvmlError>>, NvmlError> {

to:

pub fn field_values_for(
    &self,
    requests_slice: &[FieldValueRequest],
) -> Result<Vec<Result<FieldValueSample, NvmlError>>, NvmlError> {

Where FieldValueRequest is:

/// Specify a field ID and an optional scope ID for requesting data samples
/// from a device.
///
/// Used in [`crate::struct_wrappers::device::FieldValueSample`] and
/// [`crate::device::Device::field_values_for()`].
#[derive(Debug, Clone, Eq, PartialEq, Hash)]
#[cfg_attr(feature = "serde", derive(Serialize, Deserialize))]
pub struct FieldValueRequest {
    /// Populate this newtype with the constants `nvml_wrapper::sys_exports::field_id::*`.
    pub id: FieldId,
    /// Optionally populate this with a `scopeId` appropriate for the associated [`FieldId`].
    ///
    /// See NVIDIA's field ID constant docs (`NVML_FI_*`) to understand what scope
    /// IDs may be valid for which field IDs.
    pub scope_id: Option<ScopeId>,
}

impl FieldValueRequest {
    pub fn id(id: FieldId) -> Self {
        Self { id, scope_id: None }
    }

    pub fn id_with_scope(id: FieldId, scope_id: ScopeId) -> Self {
        Self {
            id,
            scope_id: Some(scope_id),
        }
    }
}

Cldfire avatar Feb 10 '24 04:02 Cldfire

That makes sense. I have submitted PR #55 which changes field_values_for to accept an Iterator of FieldValueRequests. I have also changed FieldValueSample to contain the ScopeId as well to avoid ambiguities.

bsteinb avatar Feb 22 '24 07:02 bsteinb