From 0c1da2392ae69a809768c981d570fd494b6f3528 Mon Sep 17 00:00:00 2001 From: shuhuay Date: Fri, 3 Oct 2025 13:29:21 -0700 Subject: [PATCH 1/5] Add Extent::concat 1. Addressing Github issue: https://fburl.com/ns9ekdb7. Add a method to Extent so it can concatenate another Extent. This method will check duplicate labels and return error if the sizes are conflicting, otherwise it will merge duplicate labels in the new Extent. 2. Add tests for several cases of concatenation, including overlapping labels with or without conflicting label sizes. 3. Simplify an manual concatenation in host_mesh by calling this new method. Differential Revision: [D83869447](https://our.internmc.facebook.com/intern/diff/D83869447/) **NOTE FOR REVIEWERS**: This PR has internal Meta-specific changes or comments, please review them on [Phabricator](https://our.internmc.facebook.com/intern/diff/D83869447/)! [ghstack-poisoned] --- hyperactor_mesh/src/v1/host_mesh.rs | 18 +----- ndslice/src/view.rs | 89 +++++++++++++++++++++++++++++ 2 files changed, 92 insertions(+), 15 deletions(-) diff --git a/hyperactor_mesh/src/v1/host_mesh.rs b/hyperactor_mesh/src/v1/host_mesh.rs index 416235c7f..6e763e142 100644 --- a/hyperactor_mesh/src/v1/host_mesh.rs +++ b/hyperactor_mesh/src/v1/host_mesh.rs @@ -505,23 +505,11 @@ impl HostMeshRef { ))); } - let labels = self - .region - .labels() - .to_vec() - .into_iter() - .chain(per_host.labels().to_vec().into_iter()) - .collect(); - let sizes = self + let extent = self .region .extent() - .sizes() - .to_vec() - .into_iter() - .chain(per_host.sizes().to_vec().into_iter()) - .collect(); - let extent = - Extent::new(labels, sizes).map_err(|err| v1::Error::ConfigurationError(err.into()))?; + .concat(&per_host) + .map_err(|err| v1::Error::ConfigurationError(err.into()))?; let mesh_name = Name::new(name); let mut procs = Vec::new(); diff --git a/ndslice/src/view.rs b/ndslice/src/view.rs index 643d269d0..4f32ec64e 100644 --- a/ndslice/src/view.rs +++ b/ndslice/src/view.rs @@ -307,6 +307,32 @@ impl Extent { pub fn points(&self) -> ExtentPointsIterator<'_> { ExtentPointsIterator::new(self) } + + /// Creates a new `Extent` by concatenating the labels and sizes of two input extents. + pub fn concat(&self, other: &Extent) -> Result { + let mut labels = self.labels().to_vec(); + let mut sizes = self.sizes().to_vec(); + + // Check for conflicting labels with different sizes + for (other_label, &other_size) in other.labels().iter().zip(other.sizes().iter()) { + if let Some(existing_pos) = labels.iter().position(|l| l == other_label) { + // Label already exists, check if sizes match + if sizes[existing_pos] != other_size { + return Err(ExtentError::DimMismatch { + num_labels: sizes[existing_pos], + num_sizes: other_size, + }); + } + // Sizes match, skip adding duplicate + } else { + // New label, add it + labels.push(other_label.clone()); + sizes.push(other_size); + } + } + + Extent::new(labels, sizes) + } } /// Label formatting utilities shared across `Extent`, `Region`, and @@ -2111,6 +2137,69 @@ mod test { assert!(it.next().is_none()); // fused } + #[test] + fn test_extent_concat() { + // Test basic concatenation of two extents with preserved order of labels + let extent1 = extent!(x = 2, y = 3); + let extent2 = extent!(z = 4, w = 5); + + let result = extent1.concat(&extent2).unwrap(); + assert_eq!(result.labels(), &["x", "y", "z", "w"]); + assert_eq!(result.sizes(), &[2, 3, 4, 5]); + assert_eq!(result.num_ranks(), 2 * 3 * 4 * 5); + + // Test concatenating with empty extent + let empty = extent!(); + let result = extent1.concat(&empty).unwrap(); + assert_eq!(result.labels(), &["x", "y"]); + assert_eq!(result.sizes(), &[2, 3]); + + let result = empty.concat(&extent1).unwrap(); + assert_eq!(result.labels(), &["x", "y"]); + assert_eq!(result.sizes(), &[2, 3]); + + // Test concatenating two empty extents + let result = empty.concat(&empty).unwrap(); + assert_eq!(result.labels(), &[] as &[String]); + assert_eq!(result.sizes(), &[] as &[usize]); + assert_eq!(result.num_ranks(), 1); // 0-dimensional extent has 1 rank + + // Test self-concatenation (duplicate labels with same sizes should be merged) + let result = extent1.concat(&extent1).unwrap(); + assert_eq!(result.labels(), &["x", "y"]); // Duplicates merged + assert_eq!(result.sizes(), &[2, 3]); // Original sizes preserved + assert_eq!(result.num_ranks(), 2 * 3); // Based on merged extent + + // Test concatenation creates valid points + let result = extent1.concat(&extent2).unwrap(); + let point = result.point(vec![1, 2, 3, 4]).unwrap(); + assert_eq!(point.coords(), vec![1, 2, 3, 4]); + assert_eq!(point.extent(), &result); + + // Test merge behavior with overlapping labels + let extent_a = extent!(x = 2, y = 3); + let extent_b = extent!(y = 3, z = 4); // y has same size, should merge + let result = extent_a.concat(&extent_b).unwrap(); + assert_eq!(result.labels(), &["x", "y", "z"]); // y merged, not duplicated + assert_eq!(result.sizes(), &[2, 3, 4]); + assert_eq!(result.num_ranks(), 2 * 3 * 4); + + // Test error case: conflicting label sizes + let extent_conflict1 = extent!(x = 2, y = 3); + let extent_conflict2 = extent!(y = 4, z = 5); // y has different size + let result = extent_conflict1.concat(&extent_conflict2); + assert!(result.is_err(), "Should error on conflicting label sizes"); + match result.unwrap_err() { + ExtentError::DimMismatch { + num_labels, + num_sizes, + } => { + assert_eq!(num_labels, 3); // existing size of y + assert_eq!(num_sizes, 4); // conflicting size of y + } + } + } + #[test] fn extent_unity_equiv_to_0d() { let e = Extent::unity(); From ea280e26fbda39d0fefc6a8c867830d6bd66f729 Mon Sep 17 00:00:00 2001 From: shuhuay Date: Fri, 3 Oct 2025 16:48:05 -0700 Subject: [PATCH 2/5] Update on "Add Extent::concat" 1. Addressing Github issue: https://fburl.com/ns9ekdb7. Add a method to Extent so it can concatenate another Extent. This method will check duplicate labels and return error if the sizes are conflicting, otherwise it will merge duplicate labels in the new Extent. 2. Add tests for several cases of concatenation, including overlapping labels with or without conflicting label sizes. 3. Simplify an manual concatenation in host_mesh by calling this new method. Differential Revision: [D83869447](https://our.internmc.facebook.com/intern/diff/D83869447/) **NOTE FOR REVIEWERS**: This PR has internal Meta-specific changes or comments, please review them on [Phabricator](https://our.internmc.facebook.com/intern/diff/D83869447/)! [ghstack-poisoned] --- ndslice/src/view.rs | 82 ++++++++++++++++++++++++--------------------- 1 file changed, 44 insertions(+), 38 deletions(-) diff --git a/ndslice/src/view.rs b/ndslice/src/view.rs index 4f32ec64e..40f64026e 100644 --- a/ndslice/src/view.rs +++ b/ndslice/src/view.rs @@ -59,6 +59,17 @@ pub enum ExtentError { /// Number of dimension sizes provided. num_sizes: usize, }, + + /// An overlapping label was found when concatenating extents. + /// + /// This occurs when attempting to concatenate two extents that + /// share one or more dimension labels, which is not allowed in + /// the "append dimensions" view of concatenation. + #[error("overlapping label found during concatenation: {label}")] + OverlappingLabel { + /// The label that appears in both extents. + label: String, + }, } /// `Extent` defines the logical shape of a multidimensional space by @@ -313,24 +324,20 @@ impl Extent { let mut labels = self.labels().to_vec(); let mut sizes = self.sizes().to_vec(); - // Check for conflicting labels with different sizes - for (other_label, &other_size) in other.labels().iter().zip(other.sizes().iter()) { - if let Some(existing_pos) = labels.iter().position(|l| l == other_label) { - // Label already exists, check if sizes match - if sizes[existing_pos] != other_size { - return Err(ExtentError::DimMismatch { - num_labels: sizes[existing_pos], - num_sizes: other_size, - }); - } - // Sizes match, skip adding duplicate - } else { - // New label, add it - labels.push(other_label.clone()); - sizes.push(other_size); + // Check for any duplicate labels and return error if found + for other_label in other.labels().iter() { + if let Some(_existing_pos) = labels.iter().position(|l| l == other_label) { + // Label already exists, return error regardless of size match + return Err(ExtentError::OverlappingLabel { + label: other_label.clone(), + }); } } + // Add all new labels and sizes + labels.extend(other.labels().iter().cloned()); + sizes.extend(other.sizes().iter().copied()); + Extent::new(labels, sizes) } } @@ -2164,11 +2171,18 @@ mod test { assert_eq!(result.sizes(), &[] as &[usize]); assert_eq!(result.num_ranks(), 1); // 0-dimensional extent has 1 rank - // Test self-concatenation (duplicate labels with same sizes should be merged) - let result = extent1.concat(&extent1).unwrap(); - assert_eq!(result.labels(), &["x", "y"]); // Duplicates merged - assert_eq!(result.sizes(), &[2, 3]); // Original sizes preserved - assert_eq!(result.num_ranks(), 2 * 3); // Based on merged extent + // Test self-concatenation (overlapping labels should cause error) + let result = extent1.concat(&extent1); + assert!( + result.is_err(), + "Self-concatenation should error due to overlapping labels" + ); + match result.unwrap_err() { + ExtentError::OverlappingLabel { label } => { + assert!(label == "x" || label == "y"); // Either overlapping label + } + other => panic!("Expected OverlappingLabel error, got {:?}", other), + } // Test concatenation creates valid points let result = extent1.concat(&extent2).unwrap(); @@ -2176,27 +2190,19 @@ mod test { assert_eq!(point.coords(), vec![1, 2, 3, 4]); assert_eq!(point.extent(), &result); - // Test merge behavior with overlapping labels + // Test error case: overlapping labels with same size (should error) let extent_a = extent!(x = 2, y = 3); - let extent_b = extent!(y = 3, z = 4); // y has same size, should merge - let result = extent_a.concat(&extent_b).unwrap(); - assert_eq!(result.labels(), &["x", "y", "z"]); // y merged, not duplicated - assert_eq!(result.sizes(), &[2, 3, 4]); - assert_eq!(result.num_ranks(), 2 * 3 * 4); - - // Test error case: conflicting label sizes - let extent_conflict1 = extent!(x = 2, y = 3); - let extent_conflict2 = extent!(y = 4, z = 5); // y has different size - let result = extent_conflict1.concat(&extent_conflict2); - assert!(result.is_err(), "Should error on conflicting label sizes"); + let extent_b = extent!(y = 3, z = 4); // y overlaps with same size + let result = extent_a.concat(&extent_b); + assert!( + result.is_err(), + "Should error on overlapping labels even with same size" + ); match result.unwrap_err() { - ExtentError::DimMismatch { - num_labels, - num_sizes, - } => { - assert_eq!(num_labels, 3); // existing size of y - assert_eq!(num_sizes, 4); // conflicting size of y + ExtentError::OverlappingLabel { label } => { + assert_eq!(label, "y"); // the overlapping label } + other => panic!("Expected OverlappingLabel error, got {:?}", other), } } From 14b0ca663ce05d547c50ce3275e1a548f9fd0c78 Mon Sep 17 00:00:00 2001 From: shuhuay Date: Fri, 3 Oct 2025 17:09:05 -0700 Subject: [PATCH 3/5] Update on "Add Extent::concat" 1. Addressing Github issue: https://fburl.com/ns9ekdb7. Add a method to Extent so it can concatenate another Extent. This method will check duplicate labels and return error if the sizes are conflicting, otherwise it will merge duplicate labels in the new Extent. 2. Add tests for several cases of concatenation, including overlapping labels with or without conflicting label sizes. 3. Simplify an manual concatenation in host_mesh by calling this new method. Differential Revision: [D83869447](https://our.internmc.facebook.com/intern/diff/D83869447/) **NOTE FOR REVIEWERS**: This PR has internal Meta-specific changes or comments, please review them on [Phabricator](https://our.internmc.facebook.com/intern/diff/D83869447/)! [ghstack-poisoned] --- ndslice/src/view.rs | 9 ++++----- 1 file changed, 4 insertions(+), 5 deletions(-) diff --git a/ndslice/src/view.rs b/ndslice/src/view.rs index 40f64026e..9fc3e2760 100644 --- a/ndslice/src/view.rs +++ b/ndslice/src/view.rs @@ -60,12 +60,11 @@ pub enum ExtentError { num_sizes: usize, }, - /// An overlapping label was found when concatenating extents. + /// An overlapping label was found. /// - /// This occurs when attempting to concatenate two extents that - /// share one or more dimension labels, which is not allowed in - /// the "append dimensions" view of concatenation. - #[error("overlapping label found during concatenation: {label}")] + /// This occurs when attempting to combine extents that + /// share one or more dimension labels, which is not allowed. + #[error("overlapping label found: {label}")] OverlappingLabel { /// The label that appears in both extents. label: String, From eb3d22254fea07cbba44ff0beccf921e46bdcff5 Mon Sep 17 00:00:00 2001 From: shuhuay Date: Fri, 3 Oct 2025 17:50:49 -0700 Subject: [PATCH 4/5] Update on "Add Extent::concat" 1. Addressing Github issue: https://fburl.com/ns9ekdb7. Add a method to Extent so it can concatenate another Extent. This method will check duplicate labels and return error if the sizes are conflicting, otherwise it will merge duplicate labels in the new Extent. 2. Add tests for several cases of concatenation, including overlapping labels with or without conflicting label sizes. 3. Simplify an manual concatenation in host_mesh by calling this new method. Differential Revision: [D83869447](https://our.internmc.facebook.com/intern/diff/D83869447/) **NOTE FOR REVIEWERS**: This PR has internal Meta-specific changes or comments, please review them on [Phabricator](https://our.internmc.facebook.com/intern/diff/D83869447/)! [ghstack-poisoned] --- ndslice/src/view.rs | 42 ++++++++++++++++++++++++++++-------------- 1 file changed, 28 insertions(+), 14 deletions(-) diff --git a/ndslice/src/view.rs b/ndslice/src/view.rs index 9fc3e2760..21d0fe62c 100644 --- a/ndslice/src/view.rs +++ b/ndslice/src/view.rs @@ -318,25 +318,27 @@ impl Extent { ExtentPointsIterator::new(self) } - /// Creates a new `Extent` by concatenating the labels and sizes of two input extents. + /// Append the dimensions of `other` to this extent, preserving order. + /// + /// Duplicate labels are not allowed: if any label in `other` already appears + /// in `self`, this returns `ExtentError::OverlappingLabel`. + /// + /// This operation is not commutative: `a.concat(&b)` may differ from + /// `b.concat(&a)`. pub fn concat(&self, other: &Extent) -> Result { + use std::collections::HashSet; + // Check for any overlapping labels in linear time using hash map + let lhs: HashSet<&str> = self.labels().iter().map(|s| s.as_str()).collect(); + if let Some(dup) = other.labels().iter().find(|l| lhs.contains(l.as_str())) { + return Err(ExtentError::OverlappingLabel { label: dup.clone() }); + } + // Combine labels and sizes from both extents with pre-allocated memory let mut labels = self.labels().to_vec(); let mut sizes = self.sizes().to_vec(); - - // Check for any duplicate labels and return error if found - for other_label in other.labels().iter() { - if let Some(_existing_pos) = labels.iter().position(|l| l == other_label) { - // Label already exists, return error regardless of size match - return Err(ExtentError::OverlappingLabel { - label: other_label.clone(), - }); - } - } - - // Add all new labels and sizes + labels.reserve(other.labels().len()); + sizes.reserve(other.sizes().len()); labels.extend(other.labels().iter().cloned()); sizes.extend(other.sizes().iter().copied()); - Extent::new(labels, sizes) } } @@ -2203,6 +2205,18 @@ mod test { } other => panic!("Expected OverlappingLabel error, got {:?}", other), } + + // Test that Extent::concat preserves order and is not commutative + let extent_x = extent!(x = 2, y = 3); + let extent_y = extent!(z = 4); + assert_eq!( + extent_x.concat(&extent_y).unwrap().labels(), + &["x", "y", "z"] + ); + assert_eq!( + extent_y.concat(&extent_x).unwrap().labels(), + &["z", "x", "y"] + ); } #[test] From ae8cbc64d1c800723c92f749517b2938ad78d66b Mon Sep 17 00:00:00 2001 From: shuhuay Date: Fri, 3 Oct 2025 18:20:29 -0700 Subject: [PATCH 5/5] Update on "Add Extent::concat" 1. Addressing Github issue: https://fburl.com/ns9ekdb7. Add a method to Extent so it can concatenate another Extent. This method will check duplicate labels and return error if the sizes are conflicting, otherwise it will merge duplicate labels in the new Extent. 2. Add tests for several cases of concatenation, including overlapping labels with or without conflicting label sizes. 3. Simplify an manual concatenation in host_mesh by calling this new method. Differential Revision: [D83869447](https://our.internmc.facebook.com/intern/diff/D83869447/) **NOTE FOR REVIEWERS**: This PR has internal Meta-specific changes or comments, please review them on [Phabricator](https://our.internmc.facebook.com/intern/diff/D83869447/)! [ghstack-poisoned] --- ndslice/src/view.rs | 23 +++++++++++++++++++++-- 1 file changed, 21 insertions(+), 2 deletions(-) diff --git a/ndslice/src/view.rs b/ndslice/src/view.rs index 21d0fe62c..4c2555f68 100644 --- a/ndslice/src/view.rs +++ b/ndslice/src/view.rs @@ -327,7 +327,7 @@ impl Extent { /// `b.concat(&a)`. pub fn concat(&self, other: &Extent) -> Result { use std::collections::HashSet; - // Check for any overlapping labels in linear time using hash map + // Check for any overlapping labels in linear time using hash set let lhs: HashSet<&str> = self.labels().iter().map(|s| s.as_str()).collect(); if let Some(dup) = other.labels().iter().find(|l| lhs.contains(l.as_str())) { return Err(ExtentError::OverlappingLabel { label: dup.clone() }); @@ -2180,7 +2180,7 @@ mod test { ); match result.unwrap_err() { ExtentError::OverlappingLabel { label } => { - assert!(label == "x" || label == "y"); // Either overlapping label + assert!(label == "x"); // Overlapping label should be "x" } other => panic!("Expected OverlappingLabel error, got {:?}", other), } @@ -2217,6 +2217,25 @@ mod test { extent_y.concat(&extent_x).unwrap().labels(), &["z", "x", "y"] ); + + // Test associativity: (a ⊕ b) ⊕ c == a ⊕ (b ⊕ c) for disjoint labels + let extent_m = extent!(x = 2); + let extent_n = extent!(y = 3); + let extent_o = extent!(z = 4); + + let left_assoc = extent_m + .concat(&extent_n) + .unwrap() + .concat(&extent_o) + .unwrap(); + let right_assoc = extent_m + .concat(&extent_n.concat(&extent_o).unwrap()) + .unwrap(); + + assert_eq!(left_assoc, right_assoc); + assert_eq!(left_assoc.labels(), &["x", "y", "z"]); + assert_eq!(left_assoc.sizes(), &[2, 3, 4]); + assert_eq!(left_assoc.num_ranks(), 2 * 3 * 4); } #[test]