[Experimental Backline] Transport Layer Loader is added#3045
Conversation
rniczh
changed the title
| * @return `int` | ||
| */ | ||
| virtual int collect(void *const *outputs, std::size_t n) = 0; | ||
| virtual int collect(void *const *outputs, const std::size_t *output_bytes, std::size_t n) = 0; |
There was a problem hiding this comment.
should we use std::uint64_t maybe? for uniformity it doesn't really matter
|
|
||
| #include "Transport.hpp" | ||
|
|
||
| #define CATALYST_TRANSPORT_CONTROLLER_FACTORY_SYMBOL "CatalystTransportControllerFactory" |
There was a problem hiding this comment.
do we need one for coprocessor too? or can they be united?
There was a problem hiding this comment.
Actually I don't know how the coprocessor works in this design, do you have any idea of this?
There was a problem hiding this comment.
the coprocessor is just the same as the controller, it will also have a .so
josephleekl
left a comment
josephleekl
left a comment
There was a problem hiding this comment.
just some quick comments, looks good! tag me again when addressed. thanks @rniczh !
| void __catalyst__transport__close(CatalystTransportSession *s); | ||
| int __catalyst__transport__connect(CatalystTransportSession *s, const char *peer, | ||
| uint16_t oob_port); | ||
| int __catalyst__transport__alloc_reply(CatalystTransportSession *s, uint64_t size, int32_t mem_kind, |
There was a problem hiding this comment.
what is alloc_reply? should we add that to transport.hpp?
There was a problem hiding this comment.
oh, I forgot to remove this redundancy
| const CatalystTransportPeerRef *peer); | ||
| int __catalyst__transport__commit_work_item(CatalystTransportSession *s, uint32_t work_item_idx, | ||
| uint64_t in_bytes, uint64_t out_bytes); | ||
| void *__catalyst__transport__data_slot(CatalystTransportSession *s); |
There was a problem hiding this comment.
It's for return the data slot for cpu to place the data
There was a problem hiding this comment.
There was a problem hiding this comment.
But on my side, the implementation of start is empty now, would be useful in the future if we need something to setup like warmup
|
|
||
| // ibverbs access flags for the advertised reply region | ||
| enum { | ||
| CATALYST_TRANSPORT_ACCESS_REPLY = 7, |
There was a problem hiding this comment.
Redundant code, now the allocation was handled by the implementation: 060458b
| return guard([&] { return s->sess->kick(work_item_idx); }); | ||
| } | ||
|
|
||
| int __catalyst__transport__collect(CatalystTransportSession *s, void *correction, |
There was a problem hiding this comment.
the signature here seems a bit different than the frontend UI, is that intentional? see https://github.com/PennyLaneAI/catalyst/pull/3043/changes#diff-01e7d52b33c37ab016a8280744ff88e997d75b4aa2bcaa3d051c921c95742e3dR144
There was a problem hiding this comment.
Yes, the current API only supports 1 correction data
int __catalyst__transport__collect(CatalystTransportSession *s, void *correction,
std::uint64_t bytes)
{
if (!s || !s->sess) {
return CATALYST_TRANSPORT_ERR;
}
return guard([&] {
void *outputs[1] = {correction};
std::size_t caps[1] = {static_cast<std::size_t>(bytes)};
return s->sess->collect(outputs, caps, 1);
});
}
There was a problem hiding this comment.
I think it's also a design should be changed (as in offline discussion with @josephleekl ), the number of collection should be always 1, I'll update this later
Co-authored-by: Mehrdad Malek <39844030+mehrdad2m@users.noreply.github.com>
Co-authored-by: Mehrdad Malek <39844030+mehrdad2m@users.noreply.github.com>
Co-authored-by: Mehrdad Malek <39844030+mehrdad2m@users.noreply.github.com>
Co-authored-by: Mehrdad Malek <39844030+mehrdad2m@users.noreply.github.com>
3 participants
Context:
Description of the Change:
Adds a backend-agnostic transport loader to the runtime:
TransportCAPI.h/.cpp: The ABI the compiler/codegen can emit against.TransportBackend.h: the plugin factory contract; backends are separate.sothe runtime dlopens at session create, selected via backend_lib.make all -DENABLE_TRANSPORT=ON(default OFF) to buildBenefits:
Possible Drawbacks:
Related GitHub Issues:
[sc-123779]