add support for custom backend parameters in the rust bindings #900
Conversation
cheese-head
requested review from
gleon99,
mkhazraee and
roiedanino
as code owners
|
👋 Hi cheese-head! Thank you for contributing to ai-dynamo/nixl. Your PR reviewers will review your contribution then trigger the CI to test your changes. 🚀 |
|
Hi @cheese-head, Thanks! |
cheese-head
force-pushed
the
cheese-head/rust-param-bindings
branch
from
bd104b9 to
66688a3
Compare
|
/ok to test 66688a3 |
|
/build |
roiedanino
left a comment
roiedanino
left a comment
There was a problem hiding this comment.
Please make sure your PR only contains changes relevant to backend parameters, without formatting other parts of the code
…m-bindings # Conflicts: # src/bindings/rust/src/lib.rs
Signed-off-by: Patrick Riel <[email protected]>
cheese-head
force-pushed
the
cheese-head/rust-param-bindings
branch
from
66688a3 to
d03d96a
Compare
Signed-off-by: Patrick Riel <[email protected]>
|
@roiedanino @aranadive clang format changes have been reverted and fixed. |
|
/ok to test 7d8c614 |
|
/build |
| /// | ||
| /// 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> |
There was a problem hiding this comment.
Maybe it would be better to call this function from just like in HashMap, following language convention
There was a problem hiding this comment.
Since it can produce an error, would it be better to implement TryFrom?
There was a problem hiding this comment.
Most APIs can produce an error, that's what the Result<> return type is for; Result object forces the user code to handle errors.
| /// let mut modified_params = original_params.try_clone()?; | ||
| /// modified_params.set("bucket", "my-custom-bucket")?; | ||
| /// ``` | ||
| pub fn try_clone(&self) -> Result<Self, NixlError> { |
There was a problem hiding this comment.
Maybe we can name it clone?
| let params = Params::try_from_iter(map.iter().map(|(k, v)| (*k, *v))) | ||
| .expect("Failed to create params from iterator"); |
There was a problem hiding this comment.
| let params = Params::try_from_iter(map.iter().map(|(k, v)| (*k, *v))) | |
| .expect("Failed to create params from iterator"); | |
| let params = Params::from(&map) | |
| .expect("Failed to create params from iterator"); |
should work as well
| } | ||
|
|
||
| #[test] | ||
| fn test_params_try_clone() { |
There was a problem hiding this comment.
| fn test_params_try_clone() { | |
| fn test_params_clone() { |
| for param in copied_params.iter().unwrap() { | ||
| let param = param.unwrap(); | ||
| copied_map.insert(param.key.to_string(), param.value.to_string()); | ||
| } |
There was a problem hiding this comment.
Maybe we can do HashMap::from(copied_params) if Params iterator provides a key and a value?
| } | ||
|
|
||
| #[test] | ||
| fn test_params_try_from_iter() { |
There was a problem hiding this comment.
| fn test_params_try_from_iter() { | |
| fn test_params_from_iter() { |
| // 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(); |
There was a problem hiding this comment.
I'm refering to line 61 (Ok(ParamPair { key, value })) it could be nicer to just return Ok((key, value)) which will enable iterating for key, value:
for (key, value) in params.iter() {
...
}It will also allow us to remove the ParamPair object, and use HashMap::from(params.iter())
There was a problem hiding this comment.
Do you want the entire ParamPair object removed?
There was a problem hiding this comment.
Why would we want that?
3 participants
What?
This PR adds support for custom backend parameters in the NIXL Rust bindings, enabling users to pass arbitrary key-value configuration parameters to NIXL backends.
Why?
This PR will allow from Rust to
How?
It exposes to APIs outside of the crate.