General: C++ API updates #19
Conversation
mkhazraee
commented
Mar 13, 2025
mkhazraee
commented
- Based on internal discussions within the group and external feedback.
mkhazraee
force-pushed
the
mkhazraee/cpp_api_updates
branch
from
39fc2ee to
06234ce
Compare
* Based on internal discussions within the group and external feedback. Signed-off-by: Moein Khazraee <[email protected]>
mkhazraee
force-pushed
the
mkhazraee/cpp_api_updates
branch
from
06234ce to
a9ce101
Compare
mkhazraee
force-pushed
the
mkhazraee/cpp_api_updates
branch
from
0b4ee5d to
c91b91b
Compare
…nd post to get extra_params.
| // There are two steps, initial preparations for each side, followed by a | ||
| // selection by indices to prepare a full transfer request. | ||
|
|
||
| // Prepares descriptors for one side of a transfer request. For local |
There was a problem hiding this comment.
I'd suggest to add some text formatting here to improve readability. I.e.:
Prepares descriptors for one side of a transfer request.
* For local initiator side, remote_agent should be passed as NIXL_INIT_AGENT.
* For local target side in local transfers agent's own name is passed as remote_agent.
Also, I don't follow the description here. What is local target or initiator side?
There was a problem hiding this comment.
Agreed about formatting, will do.
So for ucx we should pass local metadata for the initiator side, and remote metadata for the target, even if target is the same agent. You mentioned better to do it once, instead of each time parse the local metadata to extract the remote metadata.
| const nixl_xfer_op_t &operation, | ||
| const nixl_xfer_dlist_t &local_descs, | ||
| const nixl_xfer_dlist_t &remote_descs, | ||
| const std::string &remote_agent, |
There was a problem hiding this comment.
so the agent stays string? We had discussions it turns blob as well.
There was a problem hiding this comment.
Kapil made this comment in the meeting that for logging, string makes more sense, and that's what users want. And if they pass something that has \0, anyways we process it properly. That's more intuitive too.
| nixl_xfer_state_t postXferReq (nixlXferReqH* req); | ||
| /*** Transfer Request Prepration ***/ | ||
|
|
||
| // Method 1, for when memory addresses of the transfer is not known |
There was a problem hiding this comment.
Why phrasing it this way?
Maybe its the opposite - all the addresses are known a prior, and you create this handle at app initialization and use it throughout. it's actually the best scenario that may happen!
There was a problem hiding this comment.
I see, it's both, should update the comment, known priori or the time of transfer. But that kind of sounds all the time. I'll think about it how to make it say there are no blocks.
| // Makes a full transfer request by selecting indices from already prepared sides. | ||
| // NIXL automatically determines the backend that can perform the transfer. | ||
| // The notification message can be given in optional extra_params. | ||
| nixl_status_t selectXferSides ( |
There was a problem hiding this comment.
I don't like select word - there are 2 sides passed into this function and they are already "selected"
I think it should be something like "buildXferReq"
There was a problem hiding this comment.
Not sure about this myself either. Previously it was makeXferReq, very similar to buildXferReq. But the point being these are preprations, the post actually makes it. So not sure what else to call it.
There was a problem hiding this comment.
Select sounded like there are some preps, we're selecting from them, it's still prep. But agreed there should be a better word.
mkhazraee
force-pushed
the
mkhazraee/cpp_api_updates
branch
from
d8653e9 to
747f93e
Compare
