-
Notifications
You must be signed in to change notification settings - Fork 191
add support for custom backend parameters in the rust bindings #900
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: main
Are you sure you want to change the base?
Changes from all commits
ccc354b
c4e25b7
bc47446
d03d96a
7d8c614
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -52,9 +52,9 @@ impl<'a> Iterator for ParamIterator<'a> { | |
| }; | ||
|
|
||
| match status { | ||
| 0 if !has_next => None, | ||
| 0 if key_ptr.is_null() => None, | ||
| 0 => { | ||
| // SAFETY: If status is 0, both pointers are valid null-terminated strings | ||
| // SAFETY: If status is 0 and key_ptr is not null, both pointers are valid null-terminated strings | ||
| let result = unsafe { | ||
| let key = CStr::from_ptr(key_ptr).to_str().unwrap(); | ||
| let value = CStr::from_ptr(value_ptr).to_str().unwrap(); | ||
|
|
@@ -82,6 +82,86 @@ impl Params { | |
| Self { inner } | ||
| } | ||
|
|
||
| /// Creates a new empty Params object | ||
| pub(crate) fn create() -> Result<Self, NixlError> { | ||
| let mut params = ptr::null_mut(); | ||
|
|
||
| let status = unsafe { nixl_capi_create_params(&mut params) }; | ||
|
|
||
| match status { | ||
| 0 => { | ||
| let inner = unsafe { NonNull::new_unchecked(params) }; | ||
| Ok(Self { inner }) | ||
| } | ||
| -1 => Err(NixlError::InvalidParam), | ||
| _ => Err(NixlError::BackendError), | ||
| } | ||
| } | ||
|
|
||
| /// Creates a new Params object from an iteratable | ||
| /// | ||
| /// # Example | ||
| /// ```ignore | ||
| /// use std::collections::HashMap; | ||
| /// | ||
| /// let map = HashMap::from([ | ||
| /// ("access_key", "*********"), | ||
| /// ("secret_key", "*********"), | ||
| /// ("bucket", "my-bucket"), | ||
| /// ]); | ||
| /// | ||
| /// let params = Params::try_from_iter(map.iter().map(|(k, v)| (*k, *v)))?; | ||
| /// ``` | ||
| pub fn try_from_iter<I, K, V>(iter: I) -> Result<Self, NixlError> | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Maybe it would be better to call this function
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Since it can produce an error, would it be better to implement TryFrom?
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Most APIs can produce an error, that's what the |
||
| where | ||
| I: IntoIterator<Item = (K, V)>, | ||
| K: AsRef<str>, | ||
| V: AsRef<str>, | ||
| { | ||
| let mut params = Self::create()?; | ||
| for (key, value) in iter { | ||
| params.set(key.as_ref(), value.as_ref())?; | ||
| } | ||
| Ok(params) | ||
| } | ||
|
|
||
| /// Creates a new Params object by copying from another Params | ||
| /// | ||
| /// # Example | ||
| /// ```ignore | ||
| /// let original_params = agent.get_plugin_params("OBJ")?.1; | ||
| /// let mut modified_params = original_params.try_clone()?; | ||
| /// modified_params.set("bucket", "my-custom-bucket")?; | ||
| /// ``` | ||
| pub fn try_clone(&self) -> Result<Self, NixlError> { | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Maybe we can name it |
||
| let mut params = Self::create()?; | ||
|
|
||
| if let Ok(iter) = self.iter() { | ||
| for pair in iter { | ||
| let pair = pair?; | ||
| params.set(pair.key, pair.value)?; | ||
| } | ||
| } | ||
|
|
||
| Ok(params) | ||
| } | ||
|
|
||
| /// Sets a key-value pair in the parameters (overwrites if exists) | ||
| pub fn set(&mut self, key: &str, value: &str) -> Result<(), NixlError> { | ||
| let c_key = CString::new(key)?; | ||
| let c_value = CString::new(value)?; | ||
|
|
||
| let status = unsafe { | ||
| nixl_capi_params_add(self.inner.as_ptr(), c_key.as_ptr(), c_value.as_ptr()) | ||
| }; | ||
|
|
||
| match status { | ||
| 0 => Ok(()), | ||
| -1 => Err(NixlError::InvalidParam), | ||
| _ => Err(NixlError::BackendError), | ||
| } | ||
| } | ||
|
|
||
| /// Returns true if the parameters are empty | ||
| pub fn is_empty(&self) -> Result<bool, NixlError> { | ||
| let mut is_empty = false; | ||
|
|
||
| Original file line number | Diff line number | Diff line change | ||||||||
|---|---|---|---|---|---|---|---|---|---|---|
|
|
@@ -220,6 +220,64 @@ fn test_params_iteration() { | |||||||||
| } | ||||||||||
| } | ||||||||||
|
|
||||||||||
| #[test] | ||||||||||
| fn test_params_try_from_iter() { | ||||||||||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
Suggested change
|
||||||||||
| use std::collections::HashMap; | ||||||||||
|
|
||||||||||
| let map = HashMap::from([ | ||||||||||
| ("key1", "value1"), | ||||||||||
| ("key2", "value2"), | ||||||||||
| ("key3", "value3"), | ||||||||||
| ]); | ||||||||||
|
|
||||||||||
| let params = Params::try_from_iter(map.iter().map(|(k, v)| (*k, *v))) | ||||||||||
| .expect("Failed to create params from iterator"); | ||||||||||
|
Comment on lines
+233
to
+234
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
Suggested change
should work as well |
||||||||||
|
|
||||||||||
| assert!(!params.is_empty().unwrap(), "Params should not be empty"); | ||||||||||
|
|
||||||||||
| let mut found_keys = HashMap::new(); | ||||||||||
| for param in params.iter().unwrap() { | ||||||||||
| let param = param.unwrap(); | ||||||||||
| found_keys.insert(param.key.to_string(), param.value.to_string()); | ||||||||||
| } | ||||||||||
|
|
||||||||||
| assert_eq!(found_keys.len(), 3, "Should have 3 key-value pairs"); | ||||||||||
| assert_eq!(found_keys.get("key1"), Some(&"value1".to_string())); | ||||||||||
| assert_eq!(found_keys.get("key2"), Some(&"value2".to_string())); | ||||||||||
| assert_eq!(found_keys.get("key3"), Some(&"value3".to_string())); | ||||||||||
| } | ||||||||||
|
|
||||||||||
| #[test] | ||||||||||
| fn test_params_try_clone() { | ||||||||||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
Suggested change
|
||||||||||
| let agent = Agent::new("test_agent").expect("Failed to create agent"); | ||||||||||
| let (_mems, original_params) = agent | ||||||||||
| .get_plugin_params("UCX") | ||||||||||
| .expect("Failed to get plugin params"); | ||||||||||
|
|
||||||||||
| let copied_params = original_params.try_clone() | ||||||||||
| .expect("Failed to copy params"); | ||||||||||
|
|
||||||||||
| assert_eq!( | ||||||||||
| original_params.is_empty().unwrap(), | ||||||||||
| copied_params.is_empty().unwrap(), | ||||||||||
| "Copied params should have same empty state" | ||||||||||
| ); | ||||||||||
|
|
||||||||||
| let mut original_map = std::collections::HashMap::new(); | ||||||||||
| for param in original_params.iter().unwrap() { | ||||||||||
| let param = param.unwrap(); | ||||||||||
| original_map.insert(param.key.to_string(), param.value.to_string()); | ||||||||||
| } | ||||||||||
|
|
||||||||||
| let mut copied_map = std::collections::HashMap::new(); | ||||||||||
| for param in copied_params.iter().unwrap() { | ||||||||||
| let param = param.unwrap(); | ||||||||||
| copied_map.insert(param.key.to_string(), param.value.to_string()); | ||||||||||
| } | ||||||||||
|
Comment on lines
+273
to
+276
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Maybe we can do |
||||||||||
|
|
||||||||||
| assert_eq!(original_map, copied_map, "Copied params should match original"); | ||||||||||
| } | ||||||||||
|
|
||||||||||
| // #[test] | ||||||||||
| // fn test_get_backend_params() { | ||||||||||
| // let agent = Agent::new("test_agent").unwrap(); | ||||||||||
|
|
||||||||||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm refering to line 61 (
Ok(ParamPair { key, value })) it could be nicer to just returnOk((key, value))which will enable iterating for key, value:It will also allow us to remove the
ParamPairobject, and useHashMap::from(params.iter())There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Do you want the entire ParamPair object removed?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why would we want that?