attrs: require that values implement AttrValue (#1288)

Summary:
Pull Request resolved: #1288

We introduce `AttrValue`, which must be implemented by all value types in attribute dictionary. AttrValue 1) gathers all the requirements (Send, Serialize, Named, ...) of attribute values, and 2) introduces `AttrValue::display` and `AttrValue::parse`, to be used when attribute values are presented to humans.

In most cases, `display` and `parse` can be derived from the type's `ToString` and `FromStr` implementations. The change includes a derive macro to bridge these.
ghstack-source-id: 311304360
exported-using-ghexport

Reviewed By: shayne-fletcher

Differential Revision: D82921321

fbshipit-source-id: bc1c6ea9872db4852f61b833e08dfc54951f2c2d

Author: mariusae

hyperactor/Cargo.toml

@@ -42,6 +42,7 @@ async-trait = "0.1.86"
 backoff = { version = "0.4.0", features = ["futures", "tokio"] }
 bincode = "1.3.3"
 bytes = { version = "1.10", features = ["serde"] }
+chrono = { version = "0.4.41", features = ["clock", "serde", "std"], default-features = false }
 cityhasher = "0.1.0"
 clap = { version = "4.5.42", features = ["derive", "env", "string", "unicode", "wrap_help"] }
 crc32fast = "1.4"
@@ -54,6 +55,7 @@ erased-serde = "0.3.27"
 fastrand = "2.1.1"
 futures = { version = "0.3.31", features = ["async-await", "compat"] }
 hostname = "0.3"
+humantime = "2.1"
 hyperactor_macros = { version = "0.0.0", path = "../hyperactor_macros" }
 hyperactor_telemetry = { version = "0.0.0", path = "../hyperactor_telemetry" }
 inventory = "0.3.8"

hyperactor/src/attrs.rs

@@ -10,7 +10,7 @@
 //!
 //! This module provides `Attrs`, a type-safe dictionary that can store heterogeneous values
 //! and serialize/deserialize them using serde. All stored values must implement
-//! `Serialize + DeserializeOwned` to ensure the entire dictionary can be serialized.
+//! `AttrValue` to ensure the entire dictionary can be serialized.
 //!
 //! Keys are automatically registered at compile time using the `declare_attrs!` macro and the
 //! inventory crate, eliminating the need for manual registry management.
@@ -100,6 +100,8 @@ use std::ops::Index;
 use std::ops::IndexMut;
 use std::sync::LazyLock;
 
+use chrono::DateTime;
+use chrono::Utc;
 use erased_serde::Deserializer as ErasedDeserializer;
 use erased_serde::Serialize as ErasedSerialize;
 use serde::Deserialize;
@@ -125,8 +127,12 @@ pub struct AttrKeyInfo {
     /// Deserializer function that deserializes directly from any deserializer
     pub deserialize_erased:
         fn(&mut dyn ErasedDeserializer) -> Result<Box<dyn SerializableValue>, erased_serde::Error>,
-    // Meta-attributes.
+    /// Meta-attributes.
     pub meta: &'static LazyLock<Attrs>,
+    /// Display an attribute value using AttrValue::display.
+    pub display: fn(&dyn SerializableValue) -> String,
+    /// Parse an attribute value using AttrValue::parse.
+    pub parse: fn(&str) -> Result<Box<dyn SerializableValue>, anyhow::Error>,
 }
 
 inventory::collect!(AttrKeyInfo);
@@ -192,7 +198,7 @@ impl<T: 'static> Clone for Key<T> {
 impl<T: 'static> Copy for Key<T> {}
 
 // Enable attr[key] syntax.
-impl<T: Send + Sync + Serialize + DeserializeOwned + Named + 'static> Index<Key<T>> for Attrs {
+impl<T: AttrValue> Index<Key<T>> for Attrs {
     type Output = T;
 
     fn index(&self, key: Key<T>) -> &Self::Output {
@@ -202,14 +208,113 @@ impl<T: Send + Sync + Serialize + DeserializeOwned + Named + 'static> Index<Key<
 
 // TODO: separately type keys with defaults, so that we can statically enforce that indexmut is only
 // called on keys with defaults.
-impl<T: Send + Sync + Serialize + DeserializeOwned + Named + Clone + 'static> IndexMut<Key<T>>
-    for Attrs
-{
+impl<T: AttrValue> IndexMut<Key<T>> for Attrs {
     fn index_mut(&mut self, key: Key<T>) -> &mut Self::Output {
         self.get_mut(key).unwrap()
     }
 }
 
+/// This trait must be implemented by all attribute values. In addition to enforcing
+/// the supertrait `Named + Sized + Serialize + DeserializeOwned + Send + Sync + Clone`,
+/// `AttrValue` requires that the type be representable in "display" format.
+///
+/// `AttrValue` includes its own `display` and `parse` so that behavior can be tailored
+/// for attribute purposes specifically, allowing common types like `Duration` to be used
+/// without modification.
+///
+/// This crate includes a derive macro for AttrValue, which uses the type's
+/// `std::string::ToString` for display, and `std::str::FromStr` for parsing.
+pub trait AttrValue:
+    Named + Sized + Serialize + DeserializeOwned + Send + Sync + Clone + 'static
+{
+    /// Display the value, typically using [`std::fmt::Display`].
+    /// This is called to show the output in human-readable form.
+    fn display(&self) -> String;
+
+    /// Parse a value from a string, typically using [`std::str::FromStr`].
+    fn parse(value: &str) -> Result<Self, anyhow::Error>;
+}
+
+/// Macro to implement AttrValue for types that implement ToString and FromStr.
+///
+/// This macro provides a convenient way to implement AttrValue for types that already
+/// have string conversion capabilities through the standard ToString and FromStr traits.
+///
+/// # Usage
+///
+/// ```ignore
+/// impl_attrvalue!(i32, u64, f64);
+/// ```
+///
+/// This will generate AttrValue implementations for i32, u64, and f64 that use
+/// their ToString and FromStr implementations for display and parsing.
+#[macro_export]
+macro_rules! impl_attrvalue {
+    ($($ty:ty),+ $(,)?) => {
+        $(
+            impl $crate::attrs::AttrValue for $ty {
+                fn display(&self) -> String {
+                    self.to_string()
+                }
+
+                fn parse(value: &str) -> Result<Self, anyhow::Error> {
+                    value.parse().map_err(|e| anyhow::anyhow!("failed to parse {}: {}", stringify!($ty), e))
+                }
+            }
+        )+
+    };
+}
+
+// pub use impl_attrvalue;
+
+// Implement AttrValue for common standard library types
+impl_attrvalue!(
+    bool,
+    i8,
+    i16,
+    i32,
+    i64,
+    i128,
+    isize,
+    u8,
+    u16,
+    u32,
+    u64,
+    u128,
+    usize,
+    f32,
+    f64,
+    String,
+    std::net::IpAddr,
+    std::net::Ipv4Addr,
+    std::net::Ipv6Addr,
+    crate::ActorId,
+    ndslice::Shape,
+    ndslice::Point,
+);
+
+impl AttrValue for std::time::Duration {
+    fn display(&self) -> String {
+        humantime::format_duration(*self).to_string()
+    }
+
+    fn parse(value: &str) -> Result<Self, anyhow::Error> {
+        Ok(humantime::parse_duration(value)?)
+    }
+}
+
+impl AttrValue for std::time::SystemTime {
+    fn display(&self) -> String {
+        let datetime: DateTime<Utc> = self.clone().into();
+        datetime.to_rfc3339()
+    }
+
+    fn parse(value: &str) -> Result<Self, anyhow::Error> {
+        let datetime = DateTime::parse_from_rfc3339(value)?;
+        Ok(datetime.into())
+    }
+}
+
 // Internal trait for type-erased serialization
 #[doc(hidden)]
 pub trait SerializableValue: Send + Sync {
@@ -223,7 +328,7 @@ pub trait SerializableValue: Send + Sync {
     fn cloned(&self) -> Box<dyn SerializableValue>;
 }
 
-impl<T: Serialize + Send + Sync + Clone + 'static> SerializableValue for T {
+impl<T: AttrValue> SerializableValue for T {
     fn as_any(&self) -> &dyn Any {
         self
     }
@@ -245,7 +350,7 @@ impl<T: Serialize + Send + Sync + Clone + 'static> SerializableValue for T {
 ///
 /// This dictionary stores key-value pairs where:
 /// - Keys are type-safe and must be predefined with their associated types
-/// - Values must implement `Send + Sync + Serialize + DeserializeOwned + Named + 'static`
+/// - Values must implement [`AttrValue`]
 /// - The entire dictionary can be serialized to/from JSON automatically
 ///
 /// # Type Safety
@@ -271,20 +376,11 @@ impl Attrs {
     }
 
     /// Set a value for the given key.
-    pub fn set<T: Send + Sync + Serialize + DeserializeOwned + Named + Clone + 'static>(
-        &mut self,
-        key: Key<T>,
-        value: T,
-    ) {
+    pub fn set<T: AttrValue>(&mut self, key: Key<T>, value: T) {
         self.values.insert(key.name, Box::new(value));
     }
 
-    fn maybe_set_from_default<
-        T: Send + Sync + Serialize + DeserializeOwned + Named + Clone + 'static,
-    >(
-        &mut self,
-        key: Key<T>,
-    ) {
+    fn maybe_set_from_default<T: AttrValue>(&mut self, key: Key<T>) {
         if self.contains_key(key) {
             return;
         }
@@ -294,10 +390,7 @@ impl Attrs {
 
     /// Get a value for the given key, returning None if not present. If the key has a default value,
     /// that is returned instead.
-    pub fn get<T: Send + Sync + Serialize + DeserializeOwned + Named + 'static>(
-        &self,
-        key: Key<T>,
-    ) -> Option<&T> {
+    pub fn get<T: AttrValue>(&self, key: Key<T>) -> Option<&T> {
         self.values
             .get(key.name)
             .and_then(|value| value.as_any().downcast_ref::<T>())
@@ -306,30 +399,21 @@ impl Attrs {
 
     /// Get a mutable reference to a value for the given key. If the key has a default value, it is
     /// first set, and then returned as a mutable reference.
-    pub fn get_mut<T: Send + Sync + Serialize + DeserializeOwned + Named + Clone + 'static>(
-        &mut self,
-        key: Key<T>,
-    ) -> Option<&mut T> {
+    pub fn get_mut<T: AttrValue>(&mut self, key: Key<T>) -> Option<&mut T> {
         self.maybe_set_from_default(key);
         self.values
             .get_mut(key.name)
             .and_then(|value| value.as_any_mut().downcast_mut::<T>())
     }
 
     /// Remove a value for the given key, returning it if present.
-    pub fn remove<T: Send + Sync + Serialize + DeserializeOwned + Named + 'static>(
-        &mut self,
-        key: Key<T>,
-    ) -> bool {
+    pub fn remove<T: AttrValue>(&mut self, key: Key<T>) -> bool {
         // TODO: return value (this is tricky because of the type erasure)
         self.values.remove(key.name).is_some()
     }
 
     /// Checks if the given key exists in the dictionary.
-    pub fn contains_key<T: Send + Sync + Serialize + DeserializeOwned + Named + 'static>(
-        &self,
-        key: Key<T>,
-    ) -> bool {
+    pub fn contains_key<T: AttrValue>(&self, key: Key<T>) -> bool {
         self.values.contains_key(key.name)
     }
 
@@ -533,6 +617,19 @@ macro_rules! const_ascii_lowercase {
     }};
 }
 
+/// Macro to check check that a trait is implemented, generating a
+/// nice error message if it isn't.
+#[doc(hidden)]
+#[macro_export]
+macro_rules! assert_impl {
+    ($ty:ty, $trait:path) => {
+        const _: fn() = || {
+            fn check<T: $trait>() {}
+            check::<$ty>();
+        };
+    };
+}
+
 /// Declares attribute keys using a lazy_static! style syntax.
 ///
 /// # Syntax
@@ -595,6 +692,8 @@ macro_rules! declare_attrs {
 
     // Handle single attribute key with default value and meta attributes
     (@single $(@meta($($meta_key:ident = $meta_value:expr),* $(,)?))* $(#[$attr:meta])* ; $vis:vis attr $name:ident: $type:ty = $default:expr;) => {
+        $crate::assert_impl!($type, $crate::attrs::AttrValue);
+
         // Create a static default value
         $crate::paste! {
             static [<$name _DEFAULT>]: $type = $default;
@@ -611,6 +710,8 @@ macro_rules! declare_attrs {
 
         $(#[$attr])*
         $vis static $name: $crate::attrs::Key<$type> = {
+            $crate::assert_impl!($type, $crate::attrs::AttrValue);
+
             const FULL_NAME: &str = concat!(std::module_path!(), "::", stringify!($name));
             const LOWER_NAME: &str = $crate::const_ascii_lowercase!(FULL_NAME);
             $crate::paste! {
@@ -635,12 +736,22 @@ macro_rules! declare_attrs {
                     Ok(Box::new(value) as Box<dyn $crate::attrs::SerializableValue>)
                 },
                 meta: $crate::paste! { &[<$name _META_ATTRS>] },
+                display: |value: &dyn $crate::attrs::SerializableValue| {
+                    let value = value.as_any().downcast_ref::<$type>().unwrap();
+                    $crate::attrs::AttrValue::display(value)
+                },
+                parse: |value: &str| {
+                    let value: $type = $crate::attrs::AttrValue::parse(value)?;
+                    Ok(Box::new(value) as Box<dyn $crate::attrs::SerializableValue>)
+                },
             }
         }
     };
 
     // Handle single attribute key without default value but with meta attributes
     (@single $(@meta($($meta_key:ident = $meta_value:expr),* $(,)?))* $(#[$attr:meta])* ; $vis:vis attr $name:ident: $type:ty;) => {
+        $crate::assert_impl!($type, $crate::attrs::AttrValue);
+
         $crate::paste! {
             static [<$name _META_ATTRS>]: std::sync::LazyLock<$crate::attrs::Attrs> =
             std::sync::LazyLock::new(|| {
@@ -676,6 +787,14 @@ macro_rules! declare_attrs {
                     Ok(Box::new(value) as Box<dyn $crate::attrs::SerializableValue>)
                 },
                 meta: $crate::paste! { &[<$name _META_ATTRS>] },
+                display: |value: &dyn $crate::attrs::SerializableValue| {
+                    let value = value.as_any().downcast_ref::<$type>().unwrap();
+                    $crate::attrs::AttrValue::display(value)
+                },
+                parse: |value: &str| {
+                    let value: $type = $crate::attrs::AttrValue::parse(value)?;
+                    Ok(Box::new(value) as Box<dyn $crate::attrs::SerializableValue>)
+                },
             }
         }
     };
@@ -693,7 +812,7 @@ mod tests {
         attr TEST_TIMEOUT: Duration;
         attr TEST_COUNT: u32;
         @meta(TEST_COUNT = 42)
-        attr TEST_NAME: String;
+        pub attr TEST_NAME: String;
     }
 
     #[test]

hyperactor/src/config.rs

@@ -217,6 +217,7 @@ pub mod global {
     use std::marker::PhantomData;
 
     use super::*;
+    use crate::attrs::AttrValue;
     use crate::attrs::Key;
 
     /// Global configuration instance, initialized from environment variables.
@@ -259,17 +260,7 @@ pub mod global {
 
     /// Get a key from the global configuration. Currently only available for Copy types.
     /// `get` assumes that the key has a default value.
-    pub fn get<
-        T: Send
-            + Sync
-            + Copy
-            + serde::Serialize
-            + serde::de::DeserializeOwned
-            + crate::data::Named
-            + 'static,
-    >(
-        key: Key<T>,
-    ) -> T {
+    pub fn get<T: AttrValue + Copy>(key: Key<T>) -> T {
         *CONFIG.read().unwrap().get(key).unwrap()
     }
 
@@ -300,16 +291,7 @@ pub mod global {
         /// Create a configuration override that will be restored when the guard is dropped.
         ///
         /// The returned guard must not outlive this ConfigLock.
-        pub fn override_key<
-            'a,
-            T: Send
-                + Sync
-                + serde::Serialize
-                + serde::de::DeserializeOwned
-                + crate::data::Named
-                + Clone
-                + 'static,
-        >(
+        pub fn override_key<'a, T: AttrValue>(
             &'a self,
             key: crate::attrs::Key<T>,
             value: T,