Skip to content

Commit 68d7147

Browse files
committed
fix(nvml,core/test): code review fixes
- moves points_by_metric_and_consumer to nvml test module - points_by_metric_and_consumer now returns a sorted vector of (attribute name, attribute value). The order of this vector depends on the arguments of the function - adds tests for cmp_by_key_list utilitary function and for points_by_metric_and_consumer - fixes clock speed conversion from MHz to Hz - fixes some comments & documentation
1 parent febeb49 commit 68d7147

7 files changed

Lines changed: 237 additions & 227 deletions

File tree

‎Cargo.lock‎

Lines changed: 22 additions & 21 deletions
Some generated files are not rendered by default. Learn more about customizing how changed files appear on GitHub.

‎core/alumet/src/test/runtime.rs‎

Lines changed: 3 additions & 186 deletions
Original file line numberDiff line numberDiff line change
@@ -711,40 +711,9 @@ impl<'a> SourceCheckOutputContext<'a> {
711711
self.metrics
712712
}
713713

714-
/// Maps measurements to an `IndexMap`.
715-
///
716-
/// If multiple measurements have the same name but different attribute values,
717-
/// you should include the name of the attribute in `attribute_key_list` so that each
718-
/// measurement maps to a distinct key/value pair in the `IndexMap`.
719-
///
720-
/// ### Notes
721-
/// * The order of elements in the resulting `Vec<AttributeValue>` is not guaranteed.
722-
/// * Requesting keys that do not exist in the attributes has no impact on the results.
723-
///
724-
/// ### Usage Examples
725-
///
726-
/// ```text
727-
/// Input:
728-
/// [
729-
/// {"clock_speed", "ResourceConsumer", {clock_type = Memory, domain = GPU}},
730-
/// {"clock_speed", "ResourceConsumer", {clock_type = SM}}
731-
/// ]
732-
///
733-
/// Case 1: attribute_key_list = []
734-
/// Output keys -> [("clock_speed", "ResourceConsumer", [])]
735-
///
736-
/// Case 2: attribute_key_list = ["clock_type"]
737-
/// Output keys -> [
738-
/// ("clock_speed", "ResourceConsumer", [Memory]),
739-
/// ("clock_speed", "ResourceConsumer", [SM])
740-
/// ]
741-
///
742-
/// Case 3: attribute_key_list = ["domain"]
743-
/// Output keys -> [
744-
/// ("clock_speed", "ResourceConsumer", [GPU]),
745-
/// ("clock_speed", "ResourceConsumer", [])
746-
/// ]
747-
/// ```
714+
/// ## Deprecated
715+
/// Former utility function for tests for the NVML plugin.
716+
/// Moved to plugins/nvidia-nvml/src/lib.rs
748717
pub fn points_by_metric_and_consumer(
749718
&'a self,
750719
attribute_key_list: &[&str],
@@ -816,155 +785,3 @@ impl<'a> OutputCheckInputContext<'a> {
816785
self.metrics.deref()
817786
}
818787
}
819-
820-
#[cfg(test)]
821-
mod tests {
822-
use crate::{
823-
measurement::{self, Timestamp, WrappedMeasurementValue},
824-
units::{Unit, UnitPrefix::Plain},
825-
};
826-
827-
use super::*;
828-
829-
fn setup_mock_data<'a>() -> (MeasurementBuffer, MetricRegistry) {
830-
let mut measurements = MeasurementBuffer::new();
831-
let mut metrics = MetricRegistry::new();
832-
833-
let now = Timestamp::now();
834-
835-
let metric = Metric {
836-
name: "clock_speed".to_string(),
837-
description: "".to_string(),
838-
unit: PrefixedUnit {
839-
base_unit: Unit::Hertz,
840-
prefix: Plain,
841-
},
842-
value_type: measurement::WrappedMeasurementType::U64,
843-
};
844-
845-
let id = metrics
846-
.register(metric, DuplicateCriteria::Different, DuplicateReaction::Error)
847-
.unwrap();
848-
849-
let point1: MeasurementPoint = MeasurementPoint::new_untyped(
850-
now,
851-
id,
852-
Resource::LocalMachine,
853-
ResourceConsumer::LocalMachine,
854-
WrappedMeasurementValue::U64(123),
855-
)
856-
.with_attr("clock_type", "Memory")
857-
.with_attr("domain", "GPU");
858-
859-
let point2: MeasurementPoint = MeasurementPoint::new_untyped(
860-
now,
861-
id,
862-
Resource::LocalMachine,
863-
ResourceConsumer::LocalMachine,
864-
WrappedMeasurementValue::U64(456),
865-
)
866-
.with_attr("clock_type", "SM");
867-
868-
measurements.push(point1);
869-
measurements.push(point2);
870-
871-
(measurements, metrics)
872-
}
873-
874-
#[test]
875-
fn test_attribute_key_list_empty() {
876-
let (measurement, metrics) = setup_mock_data();
877-
let source = SourceCheckOutputContext {
878-
measurements: &measurement,
879-
metrics: &metrics,
880-
};
881-
let attribute_key_list: &[&str] = &[];
882-
883-
let result = source.points_by_metric_and_consumer(attribute_key_list);
884-
885-
// If attribute_key_list is empty,
886-
// the duplicate key collisions collapse into a single key with an empty vec![]
887-
assert_eq!(result.len(), 1);
888-
889-
let expected_key = ("clock_speed", ResourceConsumer::LocalMachine, vec![]);
890-
assert!(result.contains_key(&expected_key));
891-
}
892-
893-
#[test]
894-
fn test_attribute_key_list_with_clock_type() {
895-
let (measurement, metrics) = setup_mock_data();
896-
let source = SourceCheckOutputContext {
897-
measurements: &measurement,
898-
metrics: &metrics,
899-
};
900-
let attribute_key_list = &["clock_type"];
901-
902-
let result = source.points_by_metric_and_consumer(attribute_key_list);
903-
904-
// Should produce 2 distinct entries because both have the requested attribute,
905-
// mapping to their full list of attribute values.
906-
assert_eq!(result.len(), 2);
907-
908-
let key1 = (
909-
"clock_speed",
910-
ResourceConsumer::LocalMachine,
911-
vec![AttributeValue::from("Memory"), AttributeValue::from("GPU")],
912-
);
913-
let key2 = (
914-
"clock_speed",
915-
ResourceConsumer::LocalMachine,
916-
vec![AttributeValue::from("SM")],
917-
);
918-
919-
assert!(result.contains_key(&key1));
920-
assert!(result.contains_key(&key2));
921-
}
922-
923-
#[test]
924-
fn test_attribute_key_list_with_domain() {
925-
let (measurement, metrics) = setup_mock_data();
926-
let source = SourceCheckOutputContext {
927-
measurements: &measurement,
928-
metrics: &metrics,
929-
};
930-
let attribute_key_list = &["domain"];
931-
932-
let result = source.points_by_metric_and_consumer(attribute_key_list);
933-
934-
// Measurement 1 contains "domain" -> returns all its attributes [Memory, GPU]
935-
// Measurement 2 does NOT contain "domain" -> returns []
936-
assert_eq!(result.len(), 2);
937-
938-
let key_with_domain = (
939-
"clock_speed",
940-
ResourceConsumer::LocalMachine,
941-
vec![AttributeValue::from("Memory"), AttributeValue::from("GPU")],
942-
);
943-
let key_without_domain = ("clock_speed", ResourceConsumer::LocalMachine, vec![]);
944-
945-
assert!(result.contains_key(&key_with_domain));
946-
assert!(result.contains_key(&key_without_domain));
947-
}
948-
949-
#[test]
950-
fn test_multiple_requested_keys_returns_all_attributes() {
951-
let (measurement, metrics) = setup_mock_data();
952-
let source = SourceCheckOutputContext {
953-
measurements: &measurement,
954-
metrics: &metrics,
955-
};
956-
let attribute_key_list = &["clock_type", "non_existent_key"];
957-
958-
let result = source.points_by_metric_and_consumer(attribute_key_list);
959-
960-
let expected_key = (
961-
"clock_speed",
962-
ResourceConsumer::LocalMachine,
963-
vec![AttributeValue::from("Memory"), AttributeValue::from("GPU")],
964-
);
965-
966-
// Even though "non_existent_key" wasn't found, "clock_type" matched,
967-
// so ALL attributes are exported to the key.
968-
assert!(result.contains_key(&expected_key));
969-
}
970-
}

‎plugins/nvidia-nvml/Cargo.toml‎

Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -8,6 +8,7 @@ repository.workspace = true
88
alumet.workspace = true
99
anyhow.workspace = true
1010
humantime-serde.workspace = true
11+
indexmap = "2.14.0"
1112
log.workspace = true
1213
nvml-wrapper = { version = "0.12.0", features = ["legacy-functions"] }
1314
nvml-wrapper-sys = { version = "0.9.0" }

‎plugins/nvidia-nvml/README.md‎

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -47,7 +47,7 @@ The kind of the memory is the type of the allocated memory space reserved by the
4747

4848
#### Clock_type
4949

50-
The speed of the clock may vary depending on the domain. These are the available domains:
50+
The speed of the clock may vary depending on the type. These are the available types:
5151

5252
|Value|Description|
5353
|-----|-----------|

0 commit comments

Comments
 (0)