docs(user): wire the Distributed Programming chapter into the manual - #2221
Conversation
|
Important Review skippedAuto incremental reviews are disabled on this repository. Please check the settings in the CodeRabbit UI or the ⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Plus Run ID: You can disable this status message by setting the Use the checkbox below for a quick retry:
📝 WalkthroughWalkthroughThe user manual adds distributed programming examples, collective-operation references, benchmarking guidance, and updated navigation in English and Chinese. ChangesUser manual documentation
Estimated code review effort: 2 (Simple) | ~10 minutes Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches 💡 1🛠️ Fix failing CI checks 💡
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
a95bd94 to
d7944f0
Compare
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@docs/en/user/00-getting_started.md`:
- Around line 271-320: Clarify before or after the HelloAllReduce snippet that
it is only the device-side InCore function and is not a complete runnable
program. State that a host orchestrator, ir.compile, and distributed worker
setup are omitted, matching the limitation documented in the Chinese
counterpart.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro Plus
Run ID: 41f3f0b4-e821-4ea6-85ff-41d4f6f7c769
📒 Files selected for processing (6)
docs/en/user/00-getting_started.mddocs/en/user/02-operation_reference.mddocs/en/user/index.mddocs/zh/user/00-getting_started.mddocs/zh/user/02-operation_reference.mddocs/zh/user/index.md
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: d7944f0209
ℹ️ 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".
|
|
||
| ### Benchmarking (`benchmark`) | ||
|
|
||
| The full benchmarking guide is at [performance/00-methodology.md](performance/00-methodology.md) |
There was a problem hiding this comment.
In this worktree there is no docs/en/user/performance/ or docs/zh/user/performance/ page set (rg --files docs | rg '/performance/' returns nothing), but this commit adds links to performance/00-methodology.md, performance/index.md, and performance/02-distributed.md from both EN and ZH user docs. Until the performance chapter is added in the same branch or these links are kept as non-links, the user manual has dead links and strict docs builds will fail on this commit by itself.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Addressed: the dangling performance/* links were removed from the en/zh index and getting-started pages (the Performance chapter PR #2220 was closed unmerged). mkdocs build --strict now passes on this branch alone.
|
|
||
| | Operation | API | Modes | ReduceOp | Atomic | Supported dtypes | Notes | | ||
| | --------- | --- | ----- | -------- | ------ | ---------------- | ----- | | ||
| | AllReduce | `pld.tensor.allreduce` | `mesh` (InCore + HOST), `ring` (InCore only) | `Sum`, `Max`, `Min`, `Prod` | — | FP16, FP32 — a hard compile-time check (`allreduce.cpp`), both InCore and HOST | Mesh: O(N) remote traffic. Ring: O(N/P) traffic, 2(P-1) steps; currently InCore-only. | |
There was a problem hiding this comment.
For HOST orchestrator code, pld.tensor.allreduce(..., mode="ring") is supported and lowers to builtin.tensor.allreduce_ring (covered by tests/ut/ir/transforms/test_lower_host_tensor_collectives.py::test_host_allreduce_ring_lowers_to_ring_builtin), with explicit signal and Sum/FP32 constraints documented in the DSL docstring. This new operation matrix instead says ring is InCore-only, so users consulting the operation reference will incorrectly conclude valid HOST ring allreduce programs are unsupported.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Addressed: the AllReduce matrix row now lists ring as InCore + HOST, notes the HOST ring variant is Sum+FP32 only (4-byte) with signal [2*(NR-1)+1, NR], and references #2094.
georgebisbas
changed the title
YunjiQin
left a comment
YunjiQin
left a comment
There was a problem hiding this comment.
Docs-only review. I verified the rest of the diff against origin/main sources and it holds up: the HOST ring Sum+FP32 narrowing matches CheckSupportedSumFp32BuiltinVariant, the [2*(NR-1)+1, NR] signal shape matches DeduceBuiltinTensorAllReduceRingType, the per-collective builtin dtype claims match the builtin.tensor.* deducers in src/ir/op/distributed/collective.cpp, the NotifyOp / WaitCmp / AtomicType values match ir.pyi, and the Hello World matches tests/st/distributed/collectives/test_l3_allreduce.py verbatim. test_l3_host_tensor_allreduce_ring.py and the whole user/distributed/ chapter are on main, so every link here resolves.
Three things to fix before merge.
1. index.md drops the performance-chapter placeholder (en + zh)
The "What is not here yet" intro went from "tutorials, distributed programming, performance optimization, and accuracy debugging each get their own chapter" to "tutorials and accuracy debugging". Dropping distributed programming is right; dropping performance optimization is not — #2220 was closed unmerged, so that chapter still does not exist, and the reading path higher up in the same file still says "The dedicated performance chapter is not written yet". As it stands the file contradicts itself.
docs/en/user/index.md:— tutorials and accuracy debugging each get their own chapter→— tutorials, performance optimization, and accuracy debugging each get their own chapterdocs/zh/user/index.md:—— 教程、精度定位各自成章→—— 教程、性能优化、精度定位各自成章
2. Capability matrix drops the "per step" qualifier on the traffic figures (en + zh)
The new AllReduce row in 02-operation_reference.md says:
Mesh: O(N) remote traffic. Ring: O(N/P) traffic, 2(P-1) steps.
distributed/01-collectives.md — the page this matrix links into — qualifies both as per-step (O(N) remote traffic per step, O(N/P) remote traffic per step, and the table header Remote traffic per step). Without the qualifier, O(N/P) sitting immediately next to "2(P-1) steps" reads as ring's total traffic, which is really ~2N per rank. Please match the chapter's wording so the two pages agree:
Mesh: O(N) per step. Ring: O(N/P) per step, 2(P-1) steps.
3. The user-manual matrix cites an internal C++ file and a PR number (en + zh)
Same AllReduce row: hard compile-time check in `allreduce.cpp` and HOST ring builtin merged (#2094). A user-manual reader can neither verify nor act on either, and both go stale — the file on a refactor, the PR reference once the change is just part of a release. Suggest FP16, FP32 (mesh; hard compile-time check) and dropping the (#2094) clause; the implementation-side detail already lives in the dev docs.
Separately, and not blocking this PR: now that the HOST column advertises ring, the manual has no place stating the remaining host-ring constraints — NR ≤ 16, a statically-known src shape (no dynamic extents), and numel divisible by NR, all enforced in collective.cpp / lower_host_tensor_collectives_pass.cpp. The existing "compile-time-known NR" note in 01-collectives.md sits in the InCore-composite context only. That reads like a gap in the distributed chapter itself rather than in this wiring PR, so I'll raise it on its own.
|
Addressed — commit
Thanks for verifying the rest of the diff against |
3b7883d to
9c22265
Compare
- Add the distributed reading path plus Contents and capability rows to the user-manual index (en + zh), replacing the dev-doc distributed pointer - Point the getting-started distributed section at the chapter with a mesh allreduce Hello World (InCore kernel form) - Add the pld.* collective capability matrix to the ops catalog's Distributed section (en + zh), cross-linking the distributed chapter - Fix HOST-builtin ring allreduce claims: Sum + FP32 only with signal [2*(NR-1)+1, NR]; ring is no longer InCore-only - Drop dangling performance/* links (the performance chapter PR closed unmerged) and keep the performance placeholder in "What is not here yet"
9c22265 to
40db420
Compare
2 participants
Summary
Split out of #2162 — the final piece of the split, wiring the Distributed Programming chapter into the existing manual:
index.md(+ zh): new Contents/capability-table rows for the distributed chapter, a 4th reading path ("I want to run across multiple devices"), dropped the now-superseded dev-doc pointer for distributed collectives in favor of the real chapter.00-getting_started.md(+ zh): short pointer paragraphs from the existing benchmark/distributed sections into the distributed chapter.02-operation_reference.md(+ zh): new "Distributed / Collective Operations" capability matrix, cross-linking into the distributed chapter.distributed/01-collectives.md(+ zh): accuracy fixes to the merged chapter — the HOST builtin path now also lowersmode="ring"(merged in feat(distributed): host-orchestrated ring allreduce with builtin.tensor.allreduce_ring #2094), so the "HOST builtin is mesh-only / ring is InCore-only" claims are corrected; HOST ring isSum+FP32-only with signal[2*(NR-1)+1, NR], andtest_l3_host_tensor_allreduce_ring.pyis listed as the runnable host-ring example.Scope note: Distributed chapter only
This PR originally also wired the Performance & Benchmarking chapter. That chapter's PR #2220 was closed unmerged (2026-07-31), so the
performance/*pages do not exist onmain. The danglingperformance/*links introduced here were dropped (bothindex.mdand00-getting_started.md), somkdocs build --strictpasses on this branch alone. The Performance chapter can be re-wired in a follow-up once it lands under a new vehicle.Verification
npx markdownlint-cli2 --config tests/lint/.markdownlint.yaml— passpython3 tests/lint/check_docs_en_zh_parity.py— passpython3 tests/lint/check_docs_nav.py— passpre-commit run --files <changed files>— passmkdocs build --strict— passes on this branch alone: allperformance/*links removed and every remaining link in the touched files resolves (verified with a link scan; thedistributed/chapter exists onmainvia docs(user): add the Distributed Programming chapter #2219).