Fix Rust query_xfer_backend API #827
Conversation
evgeny-leksikov
requested review from
roiedanino,
gleon99,
mkhazraee and
a team
as code owners
|
👋 Hi evgeny-leksikov! Thank you for contributing to ai-dynamo/nixl. Your PR reviewers will review your contribution then trigger the CI to test your changes. 🚀 |
- return string instead of raw handle - add nixl_capi_query_xfer_backend_type and use it in Rust implementation - add C++ Agent::getBackendType helper API and use it in C wrapper
evgeny-leksikov
force-pushed
the
rust_query_xfer_backend_fix
branch
from
5ca8729 to
9db57bc
Compare
There was a problem hiding this comment.
We should not add APIs to nixl just for this. I don't understand why in our cpp wrapper we can't call getType on the nixlBackendH.
There was a problem hiding this comment.
Because nixl API defines nixlBackendH as an opaque type.
Dependency of the wrapper on internal core/agent_data.h may help here but IMO "get backend type" public API looks useful.
There was a problem hiding this comment.
I think @evgeny-leksikov is right here, I don't see how it can be done w/o adding a new API, because of the opaque handle type.
There was a problem hiding this comment.
Ok, let's ignore this one API then for now, as it's not used by users we know of as of today. Adding an API to C++ and Python just to handle Rust doesn't sound the best, while they can easily handle it. Also we might do a large refactor and add methods to the xferReqH object itself, will see, and we can decide on it then.
There was a problem hiding this comment.
@mkhazraee do you have any users of this API?
Otherwise we'll close this PR..
| * @return nixl_status_t Error code if call was not successful | ||
| */ | ||
| nixl_status_t | ||
| getBackendType(const nixlBackendH *backend, nixl_backend_t &type) const; |
| * @brief Get the type of a backend | ||
| * | ||
| * @param backend Backend handle | ||
| * @param type [out] Backend type |
There was a problem hiding this comment.
I think @evgeny-leksikov is right here, I don't see how it can be done w/o adding a new API, because of the opaque handle type.
| return NIXL_ERR_INVALID_PARAM; | ||
| } | ||
|
|
||
| try { |
There was a problem hiding this comment.
Let's remove the try.
It is doesn't exist anywhere else in this file.
And it is never used for getType()..
| nixl_capi_backend_t *backend); | ||
|
|
||
| nixl_capi_status_t | ||
| nixl_capi_query_xfer_backend_type(nixl_capi_agent_t agent, |
| } | ||
|
|
||
| static thread_local nixl_backend_t type; | ||
| auto nixl_ret = agent->inner->getBackendType(backend_handle->backend, type); |
| return ret; | ||
| } | ||
|
|
||
| static thread_local nixl_backend_t type; |
There was a problem hiding this comment.
Is static thread_local really needed? It doesn't appear anywhere else in this file..
There was a problem hiding this comment.
it's needed to avoid memory management in rust layer and thread safety in wrapper
| assert!(result.is_ok(), "query_xfer_backend failed with error: {:?}", result.err()); | ||
| let backend = result.unwrap(); | ||
| println!("Transfer will use backend: {:?}", backend); | ||
| let backend_name = result.unwrap(); |
There was a problem hiding this comment.
name -> type? (consistency)
| /// | ||
| /// # Returns | ||
| /// A handle to the backend used for the transfer | ||
| /// Name of the backend used for the transfer |
|
|
||
| // Extract the backend type string from the binary data | ||
| let backend_type = unsafe { | ||
| let slice = std::slice::from_raw_parts( |
There was a problem hiding this comment.
Maybe
CStr::from_ptr(backend_type_ptr as *const 8)
.to_string_lossy()
.into_owned()
```?
There was a problem hiding this comment.
c-str may lose data because it's null-teminated. Need to keep c++ like string with length
What?
Fix Rust query_xfer_backend API
Why?
return a string, as rest of the API doesn't return the raw pointer
How?
nixl_capi_query_xfer_backend_typeand use it in Rust implementationAgent::getBackendTypehelper API and use it in C wrapper