perf(glm52): connect remote rank hosts concurrently#712
Conversation
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 7a230cd986
ℹ️ About Codex in GitHub
Codex has been enabled to automatically review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".
| model_path: &Path, | ||
| moe_topo: Glm52MoeTopo, | ||
| first_remote_rank: usize, | ||
| ) -> Result<Vec<Glm52RemoteRankWorker>> { |
There was a problem hiding this comment.
Qualify the remote worker return type
When building the GLM5.2 crate, this new signature refers to Glm52RemoteRankWorker without bringing it into scope; the module only imports remote::Glm52RemoteNode near the top, while the worker type remains under crate::remote. As a result, any build that includes openinfer-glm52 fails name resolution before the new startup path or tests can run; qualify this as remote::Glm52RemoteRankWorker or add it to the existing import.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Fixed in 547f4bd: the return type is now explicitly Vec<remote::Glm52RemoteRankWorker>, matching the owning module and avoiding the missing import. I also rewrote the PR commits to add valid DCO trailers without changing the tree. The new head passes all 15 checks, including Rust formatting, CPU Clippy/tests, E2E, and Qwen3 CUDA compile/Clippy. The separate EP16/32/64 startup A/B requested on the PR is still pending suitable multi-rank-host hardware.
|
Validation update:
|
|
Do you have an end-to-end startup speedup measurement for EP16/EP32/EP64? The current CI does not exercise the GLM5.2 rank-host startup path, so green CI alone does not validate this performance change. |
Commit attribution noticeThis pull request contains commits attributed to an account other than the PR author @Nyvo-io without a verified signature from that account:
If you are mentioned above and did not take part in this pull request, please say so here. Attributing a commit to someone else normally requires that person to be involved; maintainers should treat unacknowledged attribution as a review blocker. This check never fails the build. |
Signed-off-by: CAICAII <3360776475@qq.com>
Signed-off-by: CAICAII <3360776475@qq.com>
Signed-off-by: CAICAII <3360776475@qq.com>
Signed-off-by: CAICAII <3360776475@qq.com>
52a16d8 to
547f4bd
Compare
Summary
Part of #663. Connect independent remote GLM5.2 rank-hosts concurrently during startup while preserving the CLI-defined global rank order.
What changed
(host, first_rank, rank_count)spans in CLI order;This leaves the worker/load protocol unchanged. Local worker spawn and pinned staging remain separate follow-ups from #663.
Validation
git diff --checkpassed locally.Learning focus
This change exercises scoped concurrency, deterministic rank assignment, failure cleanup, and distributed inference startup critical paths.