Repository navigation
RPC: add -sm tensor - #26610
RPC: add -sm tensor#26610
-sm tensor#26610Conversation
github-actions
Bot
added
testing
|
confirmed working on metal(RDMA 2 x M3 Ultra with #26421), testing same model (ds4 MXFP4): it does however break with dspark applied because some ops it depends on appear to not be supported with TP (add across the split), so this is without any mtp. |
|
@ryan5rdx yeah I know it doesn't work with dspark, but what do you get when for just |
|
Sorry it is on top of the that |
command for reference, let me know happy to test an alternate config: and yup just confirmed - with |
|
@ryan5rdx try using two rpc servers, one on each machine and connect via
|
|
It might be worth to add I don't know if this would be observable with RDNA, but sometimes manually moving tensors instead of standard |
|
Yeah I messed up, I think #26490 should be okay to merge though |
Don't we want to fix the DSpark support first? |
|
The support is broken over RPC I think(i.e. this PR), not in general. But I can check |
Sorry - to clarify - is this RPC servers on two RDMA linked nodes, and then a llama-server instance on one of them(if so I suppose this will just connect to the localhost RPC server)? Sorry I've never run a llama-cli/server instance where it's not also doing compute ha |
|
@ryan5rdx yes, the llama-server is just a client of these two. Basically we're trying to activate the RPC<>RPC all-reduce path rather than the one you probably got (Metal<>RPC) |
Currently on master |
Tested with Qwen327B, 0.6B and ds4. no obvious error on rpc servers or llama-server, I see the tensors copy on the RPC nodes then nothing. final client logs, llama server web UI never comes up(hangs here): command: |
for the all-reduce to work - don't RPC nodes need direct(RDMA) connections to each other in addition to (at least)TCP to the client? with an A - B(just client) - C topology where A<>B and B<>C are RDMA links, but there is no A <> C link, can this work? (in MLX they achieve this with a mesh + rdma, but here RPC servers don't know about peers yet) |
rgerganov
left a comment
rgerganov
left a comment
There was a problem hiding this comment.
Please create a mermaid sequence diagram (similar to this one ) which describe how peers communicate when -sm tensor is being used, I am still trying to understand the new flows being added and that would be very helpful. In fact, I think this should be part of our dev documentation (feel free to create an .md file) so we can maintain this in the long term.
| return true; | ||
| } | ||
|
|
||
| // minor protocol version of each connected server, used to gate newer commands (comm collectives) |
There was a problem hiding this comment.
no need to do this, we don't care about backward compatibility and we prefer to keep the code simple; just bump the version to 6.0.0 and expect all peers to be running this version
|
@ryan5rdx I run this using the directly the RDMA interfaces, for example in my case it is |
in my case the RPC nodes cannot talk to each other via the Possible I'm misunderstanding here - or maybe a mac difference because it only supports p2p (TB)rdma without routing vs the spark? |
|
@ryan5rdx not sure, you can probably ask an LLM to debug? |
Ok yup confirmed - so if I use if I instead I use the LAN IPs - it still works becuase each worker can discover the other using the ip from the client, just super slow as expected (>>10s/token). However - if we want this to work on metal, with worker<>worker RDMA we need a mesh and to allow for passing the TB IP of other workers to each rpc server(because peer addresses are not derivable from the addr we get from the client in a TB p2p network). I'm not familiar with sparks but apparently it works because "all three nodes sit on one routable RDMA fabric" |
|
@rgerganov I added a mermaid diagram. Note that the Meta backend creates a lot of subgraphs so that's why graph caching is vital. |
rgerganov
left a comment
rgerganov
left a comment
There was a problem hiding this comment.
you can improve the diagram by making a clear separation between messages sent to the standard RPC port and messages sent to the new "comm" port
| return nullptr; | ||
| } | ||
| ggml_backend_rpc_context * rpc_ctx = (ggml_backend_rpc_context *) backends[i]->context; | ||
| // one rank per endpoint: a server processes its socket sequentially, so a second |
There was a problem hiding this comment.
"one rank per endpoint" -- why having this limitation? i can have an endpoint with two devices which communicate very fast (because they are on the same physical host)
There was a problem hiding this comment.
In that case you can just create two rpc servers, one for each device?
There was a problem hiding this comment.
On local MoE models with -cmoe weights are copied to GPU during PP for faster compute. Utilizing separate processes per device would break this optimization.
The example is using local, but same optimization should be possible on rpc with multiple backends.
Utilizing same process can also be useful in general for AllReduce - 2 servers 2 GPU each could merge local results lowering the amount of network hops needed.
| if (!parse_endpoint(ranks[0].endpoint, host0, port0)) { | ||
| return nullptr; | ||
| } | ||
| const uint32_t comm_port = (uint32_t) port0 + 1000; |
There was a problem hiding this comment.
expect this port to be specified by the user via command line flag
| } | ||
|
|
||
| static void * ggml_backend_rpc_comm_init(ggml_backend_t * backends, size_t n_backends) { | ||
| if (n_backends != 2 || std::getenv("GGML_RPC_NO_COMM") != nullptr) { |
There was a problem hiding this comment.
is this going to work with more than 2 backends or it will require major redesign?
There was a problem hiding this comment.
it's going to fall-back to meta backend's all-reduce, which is slow but works
|
@am17an have you looked at implementing https://github.com/mk1-project/quickreduce/ QuickReduce seems pretty cool and it could probably be of huge benefit for RDMA performance. |
|
@rgerganov do you have time to review this PR? I'm pretty confident of the changes and I volunteer to maintain them |
cf00d3b to
d09d1c7
Compare
9a70593 to
267d466
Compare
|
I'd suggest bumping RPC_PROTO_MAJOR_VERSION so that you can't mix different server and rpc node versions like I just did while testing. Also using tensor mode over gigabit network works but is unsurprisingly super slow. I got about 8 tokens per second for prompt processing on Qwen3.8-27B-UD-IQ3_S with a 4060Ti on one box and two BC250 RPC nodes (all Vulkan). Generation is relatively much faster at 2.5/s. Layer mode on the same config give about 100/s on prompt processing and 5/s on generation. |
There was a problem hiding this comment.
I've been testing the implementation for a while and seems solid (at least on 2x DGX Sparks).
I don't follow completely all the implemented logic, but IMO it's better to get things moving a bit and hopefully start getting feedback from users. I'm also working on adding more detailed logs in #29544 to help understand and debug RPC issues. Thinking about adding some RPC tooling as well (benchmarking, connectivity testing, etc.).
| // Aux graph contents are rewritten on every compute but are identical across calls while the subgraphs are reused, | ||
| // so they can get stable uids on rebuild. Only safe without a comm backend, where the fallback usage is deterministic. | ||
| if (backend_ctx->comm_ctx == nullptr) { | ||
| for (ggml_cgraph * cgraph_aux : backend_ctx->cgraphs_aux) { | ||
| cgraph_aux->uid = ggml_graph_next_uid(); | ||
| } | ||
| } |
There was a problem hiding this comment.
It's not clear to me - should we remove this change then?
| } | ||
|
|
||
| bool rpc_server::get_tensor(const rpc_msg_get_tensor_req & request, std::vector<uint8_t> & response) { | ||
| sync_all_backends(); |
There was a problem hiding this comment.
Is this sync actually necessary?
There was a problem hiding this comment.
Yes, since everything is running async now (graph_compute, graph_recompute, comm_allreduce)
3a10a77 to
f6ae027
Compare
JohannesGaessler
left a comment
JohannesGaessler
left a comment
There was a problem hiding this comment.
The meta backend changes as they are right now LGTM.
+1 to this suggestion. There is no change in |
ggerganov
added
the
highlight
* rpc: allow -sm tensor * fix flush for apple rdma * move graph_uids to rpc_dispatcher * cont : fix conflict * cont: stop spinning dispatcher thread * remove meta backend change * add TODO to simplify logic * rpc: bump major version --------- Co-authored-by: Georgi Gerganov <ggerganov@gmail.com> (cherry picked from commit a46709b)
13 participants
Overview
Add RPC
-sm tensor. This is on 2x Sparks connected via RDMA.For RPC following changes are required:
all_reduceset_tensor_2d,get_tensor_2dLooking for feedback @ggerganov @rgerganov
Additional information
sequenceDiagram participant C as Client (RPC backend) box Server A (rank 0) participant A as A: rpc port participant Ac as A: comm port end box Server B (rank 1) participant Bc as B: comm port participant B as B: rpc port end Note over C,B: initialization C->>A: COMM_INIT (rank 0) C->>B: COMM_INIT (rank 1) Note over Ac: listen on comm port Bc-->>Ac: connect + caps negotiation (transport upgrade, e.g. RDMA) A->>C: response (ok) B->>C: response (ok) Note over C,B: for each subgraph (split at reduction boundaries) C->>A: GRAPH_COMPUTE (uid) [GRAPH_RECOMPUTE on reuse] C->>B: GRAPH_COMPUTE (uid) Note over A: compute subgraph async Note over B: compute subgraph async C->>A: COMM_ALLREDUCE (partial tensor) [fire and forget, no response] C->>B: COMM_ALLREDUCE (partial tensor) Note over A: sync pending graph<br/>cast F32→BF16 if ne ≥ 32768<br/>copy partial to send buffer Note over B: sync pending graph<br/>cast F32→BF16 if ne ≥ 32768<br/>copy partial to send buffer Ac-->>Bc: partial A (rank 0 sends first) Bc-->>Ac: partial B (rank 1 receives first) Note over A: upload partial B<br/>dst = dst + partial B (async ADD) Note over B: upload partial A<br/>dst = dst + partial A (async ADD) Note over C,B: read back the output C->>A: GET_TENSOR (output) Note over A: sync all backends A->>C: dataRequirements
Stack created with GitHub Stacks CLI • Give Feedback 💬