From e9ebb1a2ec4412963896da0b15582d78ebea94d3 Mon Sep 17 00:00:00 2001 From: Marius Eriksen Date: Sun, 21 Sep 2025 12:50:25 -0700 Subject: [PATCH] [hyperactor] attrs: require that values implement AttrValue 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. Differential Revision: [D82921321](https://our.internmc.facebook.com/intern/diff/D82921321/) [ghstack-poisoned] --- hyperactor/src/attrs.rs | 191 ++++++++++++++++++++++++++++------- hyperactor/src/config.rs | 24 +---- hyperactor/src/data.rs | 26 ++--- hyperactor/src/lib.rs | 2 + hyperactor_macros/src/lib.rs | 55 +++++++++- 5 files changed, 219 insertions(+), 79 deletions(-) diff --git a/hyperactor/src/attrs.rs b/hyperactor/src/attrs.rs index bb57b7290..1e25ff226 100644 --- a/hyperactor/src/attrs.rs +++ b/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, erased_serde::Error>, - // Meta-attributes. + /// Meta-attributes. pub meta: &'static LazyLock, + /// 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, anyhow::Error>, } inventory::collect!(AttrKeyInfo); @@ -192,7 +198,7 @@ impl Clone for Key { impl Copy for Key {} // Enable attr[key] syntax. -impl Index> for Attrs { +impl Index> for Attrs { type Output = T; fn index(&self, key: Key) -> &Self::Output { @@ -202,14 +208,110 @@ impl Index IndexMut> - for Attrs -{ +impl IndexMut> for Attrs { fn index_mut(&mut self, key: Key) -> &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; +} + +/// 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 { + 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, +); + +impl AttrValue for std::time::Duration { + fn display(&self) -> String { + humantime::format_duration(*self).to_string() + } + + fn parse(value: &str) -> Result { + Ok(humantime::parse_duration(value)?) + } +} + +impl AttrValue for std::time::SystemTime { + fn display(&self) -> String { + let datetime: DateTime = self.clone().into(); + datetime.to_rfc3339() + } + + fn parse(value: &str) -> Result { + 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 +325,7 @@ pub trait SerializableValue: Send + Sync { fn cloned(&self) -> Box; } -impl SerializableValue for T { +impl SerializableValue for T { fn as_any(&self) -> &dyn Any { self } @@ -245,7 +347,7 @@ impl 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 +373,11 @@ impl Attrs { } /// Set a value for the given key. - pub fn set( - &mut self, - key: Key, - value: T, - ) { + pub fn set(&mut self, key: Key, 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, - ) { + fn maybe_set_from_default(&mut self, key: Key) { if self.contains_key(key) { return; } @@ -294,10 +387,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( - &self, - key: Key, - ) -> Option<&T> { + pub fn get(&self, key: Key) -> Option<&T> { self.values .get(key.name) .and_then(|value| value.as_any().downcast_ref::()) @@ -306,10 +396,7 @@ 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( - &mut self, - key: Key, - ) -> Option<&mut T> { + pub fn get_mut(&mut self, key: Key) -> Option<&mut T> { self.maybe_set_from_default(key); self.values .get_mut(key.name) @@ -317,19 +404,13 @@ impl Attrs { } /// Remove a value for the given key, returning it if present. - pub fn remove( - &mut self, - key: Key, - ) -> bool { + pub fn remove(&mut self, key: Key) -> 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( - &self, - key: Key, - ) -> bool { + pub fn contains_key(&self, key: Key) -> bool { self.values.contains_key(key.name) } @@ -533,6 +614,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() {} + check::<$ty>(); + }; + }; +} + /// Declares attribute keys using a lazy_static! style syntax. /// /// # Syntax @@ -581,7 +675,6 @@ macro_rules! declare_attrs { ($( $(#[$attr:meta])* $(@meta($($meta_key:ident = $meta_value:expr),* $(,)?))* - $(#[$attr:meta])* $vis:vis attr $name:ident: $type:ty $(= $default:expr)?; )*) => { $( @@ -596,6 +689,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; @@ -612,6 +707,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! { @@ -636,12 +733,22 @@ macro_rules! declare_attrs { Ok(Box::new(value) as Box) }, 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) + }, } } }; // 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(|| { @@ -677,6 +784,14 @@ macro_rules! declare_attrs { Ok(Box::new(value) as Box) }, 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) + }, } } }; @@ -692,9 +807,9 @@ mod tests { declare_attrs! { attr TEST_TIMEOUT: Duration; - attr TEST_COUNT: u32; + attr TEST_COUNT: u32 = 23; @meta(TEST_COUNT = 42) - attr TEST_NAME: String; + pub attr TEST_NAME: String; } #[test] diff --git a/hyperactor/src/config.rs b/hyperactor/src/config.rs index 269074c83..34e14cf2c 100644 --- a/hyperactor/src/config.rs +++ b/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 { + pub fn get(key: Key) -> 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, value: T, diff --git a/hyperactor/src/data.rs b/hyperactor/src/data.rs index 3f9c46424..5dabdb9de 100644 --- a/hyperactor/src/data.rs +++ b/hyperactor/src/data.rs @@ -20,7 +20,6 @@ use serde::Deserialize; use serde::Serialize; use serde::de::DeserializeOwned; -// use strum::EnumIter; use crate as hyperactor; use crate::config; @@ -107,6 +106,12 @@ impl_basic!(usize); impl_basic!(f32); impl_basic!(f64); impl_basic!(String); +impl_basic!(std::net::IpAddr); +impl_basic!(std::net::Ipv4Addr); +impl_basic!(std::net::Ipv6Addr); +impl_basic!(std::time::Duration); +impl_basic!(std::time::SystemTime); +impl_basic!(bytes::Bytes); impl Named for &'static str { fn typename() -> &'static str { @@ -114,24 +119,6 @@ impl Named for &'static str { } } -impl Named for std::time::Duration { - fn typename() -> &'static str { - "std::time::Duration" - } -} - -impl Named for std::time::SystemTime { - fn typename() -> &'static str { - "std::time::SystemTime" - } -} - -impl Named for bytes::Bytes { - fn typename() -> &'static str { - "bytes::Bytes" - } -} - // A macro that implements type-keyed interning of typenames. This is useful // for implementing [`Named`] for generic types. #[doc(hidden)] // not part of the public API @@ -324,6 +311,7 @@ macro_rules! register_type { Deserialize, PartialEq, Eq, + crate::AttrValue, crate::Named, strum::EnumIter, strum::Display, diff --git a/hyperactor/src/lib.rs b/hyperactor/src/lib.rs index 4cddd5c43..aa438a826 100644 --- a/hyperactor/src/lib.rs +++ b/hyperactor/src/lib.rs @@ -113,6 +113,8 @@ pub use data::Named; #[doc(hidden)] pub use hyperactor_macros::Actor; #[doc(inline)] +pub use hyperactor_macros::AttrValue; +#[doc(inline)] pub use hyperactor_macros::Bind; #[doc(inline)] pub use hyperactor_macros::HandleClient; diff --git a/hyperactor_macros/src/lib.rs b/hyperactor_macros/src/lib.rs index bc4b1e1a3..0b7895e7f 100644 --- a/hyperactor_macros/src/lib.rs +++ b/hyperactor_macros/src/lib.rs @@ -1246,7 +1246,7 @@ pub fn instrument_infallible(args: TokenStream, input: TokenStream) -> TokenStre /// path. The name may be overridden by providing a string value for the /// `name` attribute. #[proc_macro_derive(Named, attributes(named))] -pub fn named_derive(input: TokenStream) -> TokenStream { +pub fn derive_named(input: TokenStream) -> TokenStream { // Parse the input struct or enum let input = parse_macro_input!(input as DeriveInput); let struct_name = &input.ident; @@ -2267,3 +2267,56 @@ pub fn observe_async(attr: TokenStream, item: TokenStream) -> TokenStream { expanded.into() } + +/// Derive the [`hyperactor::attrs::AttrValue`] trait for a struct or enum. +/// +/// This macro generates an implementation that uses the type's `ToString` and `FromStr` +/// implementations for the `display` and `parse` methods respectively. +/// +/// The type must already implement the required super-traits: +/// `Named + Sized + Serialize + DeserializeOwned + Send + Sync + Clone + 'static` +/// as well as `ToString` and `FromStr`. +/// +/// # Example +/// +/// ``` +/// #[derive(AttrValue, Named, Serialize, Deserialize, Clone)] +/// struct MyCustomType { +/// value: String, +/// } +/// +/// impl std::fmt::Display for MyCustomType { +/// fn fmt(&self, f: &mut std::fmt::Formatter) -> std::fmt::Result { +/// write!(f, "{}", self.value) +/// } +/// } +/// +/// impl std::str::FromStr for MyCustomType { +/// type Err = std::io::Error; +/// +/// fn from_str(s: &str) -> Result { +/// Ok(MyCustomType { +/// value: s.to_string(), +/// }) +/// } +/// } +/// ``` +#[proc_macro_derive(AttrValue)] +pub fn derive_attr_value(input: TokenStream) -> TokenStream { + let input = parse_macro_input!(input as DeriveInput); + let name = &input.ident; + + let (impl_generics, ty_generics, where_clause) = input.generics.split_for_impl(); + + TokenStream::from(quote! { + impl #impl_generics hyperactor::attrs::AttrValue for #name #ty_generics #where_clause { + fn display(&self) -> String { + self.to_string() + } + + fn parse(value: &str) -> Result { + value.parse().map_err(|e| anyhow::anyhow!("failed to parse {}: {}", stringify!(#name), e)) + } + } + }) +}