Skip to content

examples: distributed all-reduce tutorials (steps 08-11) + walkthroughs + CI P=4 legs - #2330

Merged
lyfne123 merged 5 commits into
hw-native-sys:mainfrom
georgebisbas:feat/examples-distributed-allreduce
Aug 26, 2026
Merged

examples: distributed all-reduce tutorials (steps 08-11) + walkthroughs + CI P=4 legs#2330
lyfne123 merged 5 commits into
hw-native-sys:mainfrom
georgebisbas:feat/examples-distributed-allreduce

Conversation

@georgebisbas

@georgebisbas georgebisbas commented Aug 10, 2026

Copy link
Copy Markdown
Contributor

Summary

Second PR of the distributed teaching ladder: the all-reduce in
depth. Four small, golden-validated programs (steps 08–11) build the
collective by hand three ways — mesh, two-phase, ring — and then reveal the
builtin, each with a walkthrough page (en + zh) and a CI P=4 leg so the
comparisons are actually observable.

Rebased on current main (2026-08-11) and review-fixed. #2317 is merged
(squash 016e69a8); this branch is main + the steps 08–11 work, squashed
into one commit (head 659def1e). Review round addressed: the ring uses
neighbour-ready handshakes instead of per-round full-mesh barriers (all
pl.stores rebound), the reveal docs state the InCore composite accepts the
full ReduceOp family + FP16/FP32 in both modes (the Sum+FP32 limit
is the HOST-builtin ring path), and the quickstart/index wording is
corrected. Ring + reveal re-sim-validated at P=2/P=4 on a2a3sim.

Tutorial steps (this PR)

Step Program Teaches
08 08_allreduce_mesh.py All-reduce v1 (mesh): every rank reads every peer's slice and sums locally, behind a dedicated-row notify/wait barrier
09 09_allreduce_two_phase.py All-reduce v2: reduce-scatter + all-gather (SIZE // P chunks, SIZE % P == 0)
10 10_allreduce_ring.py All-reduce v3 (ring): chunked rotation around the ring, 2·(P−1) rounds, neighbour-ready handshakes
11 11_allreduce_reveal.py The reveal: pld.tensor.allreduce (mesh + ring modes); the walkthrough diffs the lowered IR against the hand-rolled versions

Each program uses the class-form factory (build_* (nr)@pl.program) so
the rank count folds into static window shapes — the same source serves any P
via -d (window shapes must be statically known; see the plan's implementation
note).

Docs

  • docs/en+zh/user/distributed/13-allreduce_mesh.md16-allreduce_reveal.md
    — one walkthrough per step (The idea → Run it → Walkthrough → Edge cases →
    See also, cost card + three-column debugging table each; step 16 ships a
    guide to the IR diff as the teaching artifact).
  • docs/en+zh/user/distributed/05-tutorials.md — steps 08–11 marked ✅ shipped;
    steps 12–15 (the remaining collectives) and step 16 (composition) remain
    planned.
  • Cross-links: 01-collectives.md §Runnable Examples (allreduce row → the four
    walkthroughs), 00-model.md Quickstart ↔ the mesh walkthrough (the
    quickstart is the mesh pattern).
  • mkdocs.yml — nav entries for 13–16.

CI

Verification

  • Steps 08–11 sim-validated on a2a3sim at P=2/3/4 (reveal in both modes),
    goldens vs torch with tolerance.
  • Developer gate: 4-card NPU run
    (-p a2a3 -d 0,1,2,3) via task-submit where available; else 2-card +
    sim-P=4 evidence.

@coderabbitai

coderabbitai Bot commented Aug 10, 2026

Copy link
Copy Markdown

Review Change Stack

Important

Review skipped

Auto incremental reviews are disabled on this repository.

Please check the settings in the CodeRabbit UI or the .coderabbit.yaml file in this repository. To trigger a single review, invoke the @coderabbitai review command.

⚙️ Run configuration

Configuration used: Organization UI

Review profile: CHILL

Plan: Pro Plus

Run ID: 2ec2ba9c-1ac1-49c4-afd9-aa6aef06a5ba

You can disable this status message by setting the reviews.review_status to false in the CodeRabbit configuration file.

Use the checkbox below for a quick retry:

  • 🔍 Trigger review
📝 Walkthrough

Walkthrough

This PR adds mesh, two-phase, ring, and reveal all-reduce examples. It adds English and Chinese tutorials, updates tutorial navigation, and runs the examples in CI with four simulated ranks.

Changes

Distributed all-reduce

Layer / File(s) Summary
Mesh all-reduce implementation
examples/distributed/08_allreduce_mesh.py, docs/{en,zh}/user/distributed/13-allreduce_mesh.md
Adds staged peer loading, synchronization, accumulation, configurable rank counts, CLI execution, validation, and matching tutorials.
Two-phase all-reduce implementation
examples/distributed/09_allreduce_two_phase.py, docs/{en,zh}/user/distributed/14-allreduce_two_phase.md
Adds reduce-scatter and all-gather phases with two barriers, chunk validation, host orchestration, result checks, and matching tutorials.
Ring and reveal all-reduce implementations
examples/distributed/10_allreduce_ring.py, examples/distributed/11_allreduce_reveal.py, docs/{en,zh}/user/distributed/{15,16}-allreduce_*.md
Adds rotating ring phases and a pld.tensor.allreduce example with mesh and ring modes, constraints, execution options, and validation guidance.
Tutorial, navigation, and CI integration
.github/workflows/ci.yml, docs/{en,zh}/user/distributed/{00-model,01-collectives,05-tutorials}.md, mkdocs.yml
Marks steps 08–11 as shipped, links the new tutorials, exposes them in navigation, and validates the examples on four ranks.

Estimated code review effort: 3 (Moderate) | ~25 minutes

Sequence Diagram(s)

sequenceDiagram
  participant CLI
  participant RevealExample
  participant AllreduceAPI
  participant Ranks
  CLI->>RevealExample: choose devices and mode
  RevealExample->>AllreduceAPI: pass staged data and signals
  AllreduceAPI->>Ranks: execute mesh or ring reduction
  Ranks-->>RevealExample: return reduced output
  RevealExample-->>CLI: validate and print OK
Loading

Possibly related PRs

Poem

A rabbit hops through ranks in flight,
Mesh and ring now sum just right.
Two phases pass, then reveal sings,
Four devices test the gathered things.
New tutorials bloom in view—
“OK!” says Bunny, hopping through.

🚥 Pre-merge checks | ✅ 5
✅ Passed checks (5 passed)
Check name Status Explanation
Docstring Coverage ✅ Passed Docstring coverage is 83.33% which is sufficient. The required threshold is 80.00%.
Linked Issues check ✅ Passed Check skipped because no linked issues were found for this pull request.
Out of Scope Changes check ✅ Passed Check skipped because no linked issues were found for this pull request.
Description check ✅ Passed The description clearly summarizes the all-reduce tutorials, documentation, CI coverage, and validation included in the changeset.
Title check ✅ Passed The title clearly identifies the distributed all-reduce tutorials, walkthroughs, and P=4 CI coverage added by the pull request.

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.

❤️ Share

Comment @coderabbitai help to get the list of available commands.

@coderabbitai coderabbitai Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actionable comments posted: 5

🤖 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/distributed/00-model.md`:
- Around line 16-20: The quickstart description incorrectly calls it the same
program as the ring walkthrough. In docs/en/user/distributed/00-model.md lines
16-20, describe it as a simplified 2-rank equivalent or link to
13-allreduce_mesh.md; in docs/zh/user/distributed/00-model.md lines 14-17, apply
equivalent Chinese wording and avoid saying it is the same program.

In `@docs/en/user/distributed/05-tutorials.md`:
- Around line 4-6: The tutorial overview incorrectly classifies step 16 as a
remaining collective. In docs/en/user/distributed/05-tutorials.md lines 4-6 and
docs/zh/user/distributed/05-tutorials.md lines 3-5, update the wording to state
that steps 12–15 cover the remaining collectives and step 16 covers composition.

In `@examples/distributed/10_allreduce_ring.py`:
- Around line 79-82: Rebind every pl.store result so subsequent operations use
the updated tensor expression: update the four stores in
examples/distributed/10_allreduce_ring.py at lines 79-82, 111-120, and 148-160
to assign to scratch or y as appropriate; in
docs/en/user/distributed/15-allreduce_ring.md lines 52-70 and
docs/zh/user/distributed/15-allreduce_ring.md lines 46-64, assign each
reduce-scatter store result back to scratch.
- Around line 92-109: Replace the full-mesh barriers in
examples/distributed/10_allreduce_ring.py at lines 92-109 and 129-146 with
neighbor-ready handshakes: notify the right neighbor after stage-in or the
preceding local store, then wait for the left neighbor before remote_load.
Update the module documentation at
examples/distributed/10_allreduce_ring.py:92-109 and the descriptions in
docs/en/user/distributed/15-allreduce_ring.md:3-6 and
docs/zh/user/distributed/15-allreduce_ring.md:3-5 to state that payload
transfers read from the left neighbor while synchronization uses both adjacent
neighbors; if full-mesh barriers remain, document their O(P²) signal cost and
remove the claim that communication only uses the left neighbor.

In `@examples/distributed/11_allreduce_reveal.py`:
- Around line 129-142: Update the --device argument help text in the parser
configuration to remove the “dividing SIZE” requirement, using wording that
states only that any device count of at least two is supported.
🪄 Autofix

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: 7c87b0f1-e387-42a1-b39c-93066470a640

📥 Commits

Reviewing files that changed from the base of the PR and between 0be6431 and 5c9c43d.

📒 Files selected for processing (20)
  • .github/workflows/ci.yml
  • docs/en/user/distributed/00-model.md
  • docs/en/user/distributed/01-collectives.md
  • docs/en/user/distributed/05-tutorials.md
  • docs/en/user/distributed/13-allreduce_mesh.md
  • docs/en/user/distributed/14-allreduce_two_phase.md
  • docs/en/user/distributed/15-allreduce_ring.md
  • docs/en/user/distributed/16-allreduce_reveal.md
  • docs/zh/user/distributed/00-model.md
  • docs/zh/user/distributed/01-collectives.md
  • docs/zh/user/distributed/05-tutorials.md
  • docs/zh/user/distributed/13-allreduce_mesh.md
  • docs/zh/user/distributed/14-allreduce_two_phase.md
  • docs/zh/user/distributed/15-allreduce_ring.md
  • docs/zh/user/distributed/16-allreduce_reveal.md
  • examples/distributed/08_allreduce_mesh.py
  • examples/distributed/09_allreduce_two_phase.py
  • examples/distributed/10_allreduce_ring.py
  • examples/distributed/11_allreduce_reveal.py
  • mkdocs.yml

Comment thread docs/en/user/distributed/00-model.md Outdated
Comment thread docs/en/user/distributed/05-tutorials.md Outdated
Comment thread examples/distributed/10_allreduce_ring.py Outdated
Comment thread examples/distributed/10_allreduce_ring.py Outdated
Comment thread examples/distributed/11_allreduce_reveal.py

@chatgpt-codex-connector chatgpt-codex-connector Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

💡 Codex Review

Here are some automated review suggestions for this pull request.

Reviewed commit: 5c9c43de98

ℹ️ 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".

Comment thread docs/en/user/distributed/16-allreduce_reveal.md Outdated
@georgebisbas
georgebisbas force-pushed the feat/examples-distributed-allreduce branch 2 times, most recently from 3185ef9 to 659def1 Compare August 11, 2026 06:58

@lyfne123 lyfne123 left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The hand-written algorithms are internally consistent and CI is green, but the new walkthroughs currently contain two blocking mismatches with the implementation: the supported reduction operators and the builtin ring synchronization protocol. I also found three smaller accuracy issues in the signal-shape and scaling explanations. Please update both the English pages and their Chinese mirrors.

Comment on lines +62 to +67
- **What the builtin narrows (read this twice):** the InCore composite this
example calls (explicit signal) accepts the full `ReduceOp` family
(`Sum`/`Max`/`Min`/`Prod`) and `FP16`/`FP32` in **both** modes — the
`Sum`+`FP32`-only restriction belongs to the **HOST builtin** ring path, not
to this InCore form. The ring intrinsic ST suite exercises `Max`/`Min`/`Prod`
and `FP16`.

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

[P1] The explicit-signal InCore composite does not currently support the full ReduceOp family. DeduceTensorAllReduceType accepts only ReduceOp.Sum, both mesh and ring lowerings assert the same restriction, and the existing unit tests require Max to fail. The ring intrinsic ST also covers only FP32 + Sum. Please document the current Sum-only contract, remove the unsupported ST-coverage claim, and update the Chinese mirror and the related troubleshooting row below.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks — I verified this against the current code, and the walkthrough's claim already matches the implementation: the builtin accepts the full ReduceOp family and FP16/FP32 in both modes.

  • DeduceTensorAllReduceType (src/ir/op/distributed/allreduce.cpp:111-113) validates kSum <= op <= kProd — Sum/Max/Min/Prod are all accepted.
  • tests/ut/ir/test_distributed_ops.py (test_tensor_allreduce_accepts_every_reduce_op, ~236-246) parametrizes Sum/Max/Min/Prod and asserts all are accepted (Max does not fail).
  • The composite lowering (src/ir/transforms/lower_composite_ops_pass.cpp) maps Sum→tile.add, Max→tile.maximum, Min→tile.minimum, Prod→tile.mul in mesh mode, and the ring lowering accepts the same [kSum..kProd] range.
  • Codegen (src/codegen/distributed/distributed_ops_codegen.cpp:54-65) emits TADD/TMAX/TMIN/TMUL for the four variants.
  • The ring intrinsic ST (tests/st/distributed/collectives/test_l3_tensor_allreduce_ring_intrinsic.py:230-246, test_ring_allreduce_reduce_ops) exercises every operator through the real ring pipeline with no op-level skip, and test_ring_allreduce_fp16_arbitrary_lengths covers FP16.

I also removed the one remaining unsupported carve-out (the "HOST builtin is Sum+FP32-only" line) since the host codegen path supports the same four variants, and aligned the zh mirror and the troubleshooting row. If there is a specific path (a backend, an op, a shape) that still restricts the family, point me at it and I'll document that case.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Following up after re-verifying everything against current main (fef2831c).

First, you were right about one thing and I was wrong. My earlier reply said I had removed the "HOST builtin is Sum+FP32-only" carve-out "since the host codegen path supports the same four variants". That claim was incorrect. The gate is still there and still live:

  • src/ir/op/distributed/collective.cpp:62CheckSupportedSumFp32BuiltinVariant
  • called at :194 and :909

So the HOST builtin ring path (builtin.tensor.allreduce_ring) genuinely is Sum + FP32-only. The docs say exactly that and are correct as written — docs/en/user/distributed/01-collectives.md:103 and 16-allreduce_reveal.md:67. Nothing needs changing there; my sentence about removing it was the error, not the docs.

On the actual P1 — the explicit-signal InCore composite — the full ReduceOp family is supported. Re-verified on current main, since line numbers moved since August 11:

  • src/ir/op/distributed/allreduce.cpp:107DeduceTensorAllReduceType validates op_value >= kSum && op_value <= kProd. Not Sum-only, and not mode-dependent.
  • src/ir/transforms/lower_composite_ops_pass.cpp:881 (mesh) and :1171 (ring) — both assert the same [kSum, kProd] range, and both dispatch through the shared Reduce(op, ...) helper (line ~293) mapping Sum/Max/Min/Prod to tile.add / tile.maximum / tile.minimum / tile.mul.
  • tests/st/distributed/collectives/test_l3_tensor_allreduce_ring_intrinsic.py:228-238test_ring_allreduce_reduce_ops is parametrized over (Sum, Max, Min, Prod) and runs through the real ring pipeline with no op-level skip.

One thing worth flagging, because it is a genuine trap. There is a Sum-only assert in that lowering file:

// src/ir/transforms/lower_composite_ops_pass.cpp:1783
INTERNAL_CHECK_SPAN(op_value == static_cast<int>(ReduceOp::kSum), span)
    << "pld.tensor.reduce_scatter lowering supports ReduceOp::kSum only ...";

That one belongs to pld.tensor.reduce_scatter, a different collective — not allreduce. A grep for ReduceOp::kSum in this file surfaces it right next to the allreduce paths, and I suspect that is what the original review found. It does not constrain pld.tensor.allreduce in either mode.

Net: the tutorial text is accurate as it stands — steps 08-11 use the InCore composite, which takes the full family; the narrower Sum+FP32 contract belongs to the separate HOST builtin ring path, which these tutorials do not use, and which the docs already document correctly.

If you still see a specific path that restricts the family — a backend, an op, a shape — point me at it and I will document that case. Otherwise, could you re-review? This is currently the only review blocking the PR; CI is green across all 15 jobs including dist-system-tests and examples-tests.

Comment thread docs/en/user/distributed/16-allreduce_reveal.md Outdated
Comment thread docs/en/user/distributed/16-allreduce_reveal.md Outdated
Comment thread docs/en/user/distributed/15-allreduce_ring.md Outdated
Comment thread docs/en/user/distributed/15-allreduce_ring.md Outdated
@georgebisbas
georgebisbas force-pushed the feat/examples-distributed-allreduce branch from 659def1 to ed4fbef Compare August 11, 2026 11:40
@georgebisbas

Copy link
Copy Markdown
Contributor Author

@lyfne123 Thanks for the detailed review — round 2 is pushed.

  • Supported reduction operators: I verified the claim against the code — the deducer, unit tests, both mesh and ring lowerings, codegen, and the ring intrinsic ST all cover Sum/Max/Min/Prod and FP16 (evidence with file/line citations in the thread). The walkthrough keeps the full-family statement, and I removed the last unsupported carve-out (the "HOST builtin is Sum+FP32-only" line) since the host codegen path supports the same four variants. If there is a specific path that still restricts the family, point me at it and I'll document that case.
  • Builtin ring synchronization: the walkthrough now states that --mode ring lowers each round to a full-mesh barrier (EmitNotifyAll/EmitWaitAll), not step 10's neighbour-ready handshake, and frames that difference as the point of the IR diff.
  • Signal-shape asymmetry, scaling model, and the P=2 left-neighbour parenthetical: all corrected in both English and Chinese pages, plus the troubleshooting table and the reveal example docstring.

All five threads have replies; four are resolved (the reduction-operators thread is left open for your confirmation). CI is re-running on the amended commit.

@YunjiQin YunjiQin left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Three consistency notes on the example sources, left inline. None of them are correctness bugs — I walked the ring index arithmetic and the handshake chain and the algorithm is right — they are all about the ladder staying legible as a teaching sequence, where a style drift costs more than it would in ordinary code.

y: pl.Out[pl.Tensor[[1, SIZE], pl.FP32]],
scratch: pl.InOut[pld.DistributedTensor[[1, SIZE], pl.FP32]],
signal: pl.InOut[pld.DistributedTensor[[total_rounds, nr], pl.INT32]],
chunk_elems: pl.Scalar[pl.INDEX],

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

chunk_elems is redundant, and mixing it with chunk obscures the arithmetic this file exists to teach.

It is threaded through ring_step and per_rank as a pl.Scalar[pl.INDEX], but the host passes chunk_elems = SIZE // nr (line 200) — exactly the compile-time closure constant chunk already computed on line 55. The two are always equal, so the parameter buys nothing.

The cost is that offsets and shapes end up spelled differently on the same line:

recv = pld.tile.remote_load(scratch, peer=left,
                            offsets=[0, left_send_idx * chunk_elems],  # runtime scalar
                            shape=[1, chunk])                          # compile-time const

A reader working through recv_add_idx / left_send_idx on paper — which the walkthrough explicitly asks them to do — will reasonably assume the split is meaningful. Suggest dropping the parameter from both signatures and using chunk for offsets and shapes alike.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Agreed — removed in 6a918fa (the parameter, both signatures, the call site, and the host-side chunk_elems = SIZE // nr). All eight body uses now read chunk, so the index arithmetic is expressed in exactly one name.

You were right that it bought nothing: chunk is already the compile-time closure constant, and threading an always-equal pl.Scalar[pl.INDEX] alongside it just gave the reader two names for one quantity in the file whose whole subject is the index arithmetic.

Verified in the simulation image at P=2 and P=4.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Dropped — chunk_elems is gone from both ring_step and per_rank, and offsets and shapes now both use the closure constant chunk:

recv = pld.tile.remote_load(scratch, peer=left,
                            offsets=[0, left_send_idx * chunk],
                            shape=[1, chunk])

Your read of the cost was right: the split made the index arithmetic look like it carried a distinction it did not, on exactly the lines the walkthrough asks readers to work through on paper.

SIZE = 64


def build_mesh_allreduce(nr: int):

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The ladder switches program-definition idiom here, and only half the switch is explained.

Steps 01–07 use @pl.jit.host / @pl.jit.incore with program.compile(x, y, config=RunConfig(...)). Steps 08–11 use the @pl.program class form with ir.compile(program, platform=..., distributed_config=...) (line 172).

The docstring explains why a factory is needed (window shapes must be statically known and depend on nr) — that part is clear and correct. What it does not explain is why the factory also changes the decorator family and the compile entry point: a factory could equally return @pl.jit.host-decorated functions with nr closed over.

This lands immediately after step 07 taught "one source, any P" via pl.dynamic("NR") under @pl.jit, so a reader arriving at step 08 sees three things change at once with one of them motivated. A sentence or two in 13-allreduce_mesh.md §Walkthrough covering the decorator/compile switch (or, if the class form is not actually required, keeping @pl.jit inside the factory) would close the gap.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Addressed in 6a918fa, and your instinct to push on this was right — the explanation I had written was wrong in three separate ways.

I tried the conversion you suggested. It does not work, but not for the reason the docstring claimed. Converting 08 to the @pl.jit family fails at compile time with:

ValueError: @pl.jit: missing inferred tensor metadata for parameter 'signal'
  python/pypto/jit/specializer.py:1986 in _build_params

signal is a window shaped [pld.world_size(), 1], and @pl.jit must statically infer shape and dtype for every parameter it forwards to a dependency. It is not distributed tensors as such — steps 03, 05, 06 and 07 all pass pld.DistributedTensor through @pl.jit deps happily; every one of those windows is statically shaped. It is specifically a runtime-sized window dim. (There is a second, independent gap too: @pl.jit's constant folding reads only func.__globals__ (specializer.py:2101), never __closure__, so a factory closure constant referenced in a HOST body cannot resolve either. Both look worth their own issues; neither is a regression — the message dates to #915.)

But the factory was never needed here, which is what the old text got wrong. Step 08 has no tile shape depending on the rank count, so the barrier signal row count can simply stay dynamic: NR = pl.dynamic("NR") in the annotations, pld.world_size() in the host body. 08 is now a module-level @pl.program with no factory, matching tests/st/distributed/collectives/test_l3_allreduce.py — the system test for this same collective.

So the switch is still required, but the honest reason is the dynamic signal shape, not a compile-time one; and the compile entry point follows from it, since ir.compile(...) is simply the class form's entry point. The walkthrough now says exactly that, with the error string quoted.

Three corrections to the docs while I was in there, all of which were my errors:

  • It claimed "every rank-parametrized collective in tests/st/distributed/ uses this same class-form factory". False — three of five do not (test_l3_allreduce.py, test_l3_allgather.py, test_l3_broadcast.py are module-level with pl.dynamic).
  • It attributed the switch to @pl.program snapshotting the defining frame's locals. Real limitation, wrong conclusion for this file.
  • A troubleshooting row told readers a [nr, 1] shape must be a closure constant — which would have sent them to "fix" working code. Replaced with the actual error and its real remedy.

Steps 09/10 do keep the factory, and step 14 now states the sharp reason the docs previously lacked: their chunk size SIZE // nr is a tile shape, and tile shapes must be known when the kernel is compiled. A signal row count is not, which is why 08 does not need one. The idiom switch now marks a real constraint boundary rather than an arbitrary one.

Verified in the simulation image (pypto 10286517, pto-isa f51c92f6): all four examples pass at P=2 and P=4, plus 11 --mode ring at P=4.

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks — the investigation and the revised step 08 explanation make sense. There is one remaining consistency gap in the example docstrings:

  • Step 09 still says A rank-count factory (as in step 08) and attributes the factory to the nr-dependent window/signal shape. Step 08 no longer has a factory, and the actual compile-time constraint here is chunk = SIZE // nr being used as a tile shape.
  • Step 10 still says the signal shape and chunk size “both must be statically known” and refers to steps 08/09. That conflicts with the corrected explanation that a signal row count alone need not be static; it is the chunk tile shape that forces compile-time nr here.
  • Step 11 likewise says its factory is “as in steps 08-10” and attributes it to the signal shape. At minimum, the step 08 reference is stale; the rationale should distinguish mode specialization and its signal layout from the tile-shape constraint in steps 09/10.

Could you align these three docstrings with the corrected explanation before merge?

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Aligned in 9b37ba8 — all three, plus the step 11 walkthrough page (en + zh), which was the last place still describing the factory without the distinction.

Step 09 — the factory now has one reason, and it is not the signal:

A rank-count factory — the first step that needs one. Step 08 has none: its signal row count stays pl.dynamic. What forces a compile-time nr here is chunk = SIZE // nr, used as a tile shape (the [1, chunk] on every pl.load / remote_load below) [...] The [2, nr] signal then falls out of the same constant for free — but a signal shape on its own would not have required it.

Step 10 — same single reason as 09; the "both must be statically known" claim is gone, since it was the exact sentence that contradicted the correction.

Step 11 — you were right that this one needed its own rationale rather than a pointer at 08-10, and chasing it turned up something I could not settle by reading. The pld.tensor.allreduce docstring (tensor_ops.py:624-628) says the ring signal is [2*(NR-1), NR] "where NR must be a compile-time constant". But the lowering does not enforce that — lower_composite_ops_pass.cpp:1182-1194 only cross-checks shape[0] == 2*(shape[1]-1) when both dims are ConstInt, and explicitly skips when either is dynamic.

So rather than repeat last round's mistake and assert it, I tested it: a module-level @pl.program copy of step 11 with NR = pl.dynamic("NR"), a [2*(NR-1), NR] ring signal and pld.window(signal_buf, [2*(pld.world_size()-1), pld.world_size()], ...) in the host body compiles and passes its golden on a2a3sim at -d 0,1,2 and -d 0,1,2,3. A compile-time nr is not required in step 11 at all. The docstring now says so:

A factory over (nr, mode), and for a different reason than steps 09/10: the builtin owns the chunking, so no tile shape here depends on the rank count — nr need not be compile-time at all (the ring layout, the more constrained of the two, compiles and passes its golden with a pl.dynamic rank count). What must be fixed when the kernel is traced is mode: it picks both the lowering pld.tensor.allreduce emits and the signal layout the kernel is annotated with, and those are two different shapes, not two extents of one — mesh [nr, 1], ring [2*(nr-1), nr].

That makes the ladder's idiom boundary land where the real constraint is: 08 needs nothing, 09/10 need a compile-time nr for a tile shape, 11 needs a build-time mode and folds nr in beside it only so one source can spell both layouts.

The overstated line in the pld.tensor.allreduce docstring is a separate matter — tracked locally, not touched here, since it is API documentation rather than tutorial text.

Re-verified after the change: all four examples plus 11 --mode mesh / --mode ring print OK at P=4 on a2a3sim; pre-commit (incl. en-zh parity, markdownlint, ruff, pyright) and mkdocs build --strict clean.

# Each rank owns a dedicated row (offsets=[my_rank, 0]);
# AtomicAdd/Ge(1) means the wait only passes once every peer has
# staged its slice.
for peer in pl.range(nr):

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Four adjacent teaching files use four different rank-count spellings.

File kernel loops host orchestration loop
08 pl.range(nr) — closure constant pl.range(pld.world_size())
09 pl.range(nranks)pld.nranks(ctx) pl.range(pld.world_size())
10 pl.range(nranks) pl.range(nr)
11 pl.range(nr)

All four are functionally equivalent here (nr == world_size by construction), so nothing is broken. But steps 08–11 are meant to be read back-to-back and diffed against each other, and step 07 has just finished making the point that the rank count is a runtime property. Having the very next step drop nranks for a closure constant, and then having 10/11 drop pld.world_size() on the host side, sends the opposite signal.

Suggest picking one convention and applying it across all four: nranks = pld.nranks(ctx) inside kernels (carries step 07's lesson forward), pld.world_size() in host orchestration (matches step 07's source). nr then stays reserved for what genuinely needs to be compile-time — the window shapes — which is exactly the distinction the factory docstrings are trying to draw.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Fixed in 6a918fa. The four files now use one spelling each, by role:

File kernel loops host orchestration loop
08 pl.range(nranks)pld.nranks(ctx) pl.range(pld.world_size())
09 pl.range(nranks) pl.range(pld.world_size())
10 pl.range(nranks) pl.range(pld.world_size())
11 pl.range(pld.world_size())

The rule, applied uniformly: a kernel derives the rank count at runtime from its comm context (pld.nranks(ctx)), a host orchestrator uses pld.world_size(), and a bare nr is reserved for compile-time shapes only — never a loop bound.

Step 08 no longer has an nr at all (see the other thread), so the pl.range(nr) spelling is gone from the series entirely.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Converged on the convention you proposed — pld.nranks(ctx) inside kernels, pld.world_size() in host orchestration, and nr reserved for what genuinely has to be compile-time.

Current state across the four files:

File kernel loops host orchestration loop
08 pld.nranks(ctx) (:72) pl.range(pld.world_size()) (:130)
09 pld.nranks(ctx) (:74) pl.range(pld.world_size()) (:166)
10 pld.nranks(ctx) (:83) pl.range(pld.world_size()) (:201)
11 — (one builtin call, no rank loop) pl.range(pld.world_size()) (:112)

That also made the point you were making about step 07 land properly: nr now appears only where it is a real compile-time constraint, and the just-pushed 9b37ba8 makes each file say which constraint that is — a tile shape in 09/10, mode specialization in 11, and none at all in 08.

@georgebisbas
georgebisbas force-pushed the feat/examples-distributed-allreduce branch 2 times, most recently from fba63a1 to 6a918fa Compare August 19, 2026 08:15
@georgebisbas

Copy link
Copy Markdown
Contributor Author

@YunjiQin all three of your points are addressed and pushed — summary here since the branch has moved a fair bit since your review.

Your three items

Item Resolution
chunk_elems redundant with chunk Removed — parameter, both signatures, call site, and the host-side chunk_elems = SIZE // nr. All 8 body uses now read chunk.
Idiom switch at 08 half-explained 08 no longer has a factory at all, and the explanation was rewritten because it was wrong. Detail below.
Four rank-count spellings Unified by role: kernels use pld.nranks(ctx), host orchestrators use pld.world_size(), bare nr is reserved for compile-time shapes. pl.range(nr) is gone from the series.

On the idiom switch — you were right to push, and I was wrong twice

I tried the @pl.jit conversion you suggested. It fails:

ValueError: @pl.jit: missing inferred tensor metadata for parameter 'signal'
  python/pypto/jit/specializer.py, _build_params

@pl.jit must statically infer shape and dtype for every parameter it forwards to a dependency, and signal is a window shaped [pld.world_size(), 1]. Not distributed tensors in general — steps 03/05/06/07 pass them through @pl.jit fine; specifically a runtime-sized window dim. Filed as #2450, along with #2449 for a second, independent gap (@pl.jit constant folding reads only __globals__, never __closure__).

But investigating it showed the factory was never needed for step 08. Its signal row count is not a tile shape, so it can stay dynamic: NR = pl.dynamic("NR") plus pld.world_size(). Step 08 is now a module-level @pl.program with no factory, matching tests/st/distributed/collectives/test_l3_allreduce.py — the system test for this same collective.

The old walkthrough was wrong in three ways, all mine:

  • It claimed every rank-parametrized collective in tests/st/distributed/ uses a class-form factory. Three of five do not — test_l3_allreduce.py, test_l3_allgather.py, test_l3_broadcast.py are module-level with pl.dynamic.
  • It blamed the switch on @pl.program snapshotting the defining frame's locals. Real limitation, wrong conclusion for this file.
  • A troubleshooting row told readers a [nr, 1] shape must be a closure constant — which would have sent them to "fix" working code.

Steps 09/10 keep their factories, and step 14 now gives the reason the docs previously lacked: their chunk size SIZE // nr is a tile shape, and tile shapes must be known at compile time. So the idiom switch now marks a real constraint boundary instead of an arbitrary one.

Also fixed 01-collectives.md Ring Mode (77d278cf), which documented the @pl.jit-closure factory as the pattern on the grounds that "the specializer folds the closure constants" — it does not, per #2449.

Verification — simulation image, pypto 10286517 / pto-isa f51c92f6: all four examples pass at P=2 and P=4, plus 11 --mode ring at P=4. Local gates green: ruff check + format, EN↔ZH parity, docs nav, English-only, headers.

Re-review when you have a moment — happy to revisit any of it.

@georgebisbas
georgebisbas force-pushed the feat/examples-distributed-allreduce branch from 77d278c to 1e285c4 Compare August 21, 2026 06:13
…throughs +

CI

Four golden-validated programs build the all-reduce by hand three ways and
reveal the builtin:
- 08_allreduce_mesh.py: every rank reads every peer's slice and sums locally
- 09_allreduce_two_phase.py: reduce-scatter + all-gather (SIZE // P chunks)
- 10_allreduce_ring.py: chunked rotation around the ring, 2*(P-1) rounds,
  synchronized by neighbour-ready handshakes (notify the right neighbour
  after a store, wait on the left before a remote_load) instead of a full
  barrier per round
- 11_allreduce_reveal.py: pld.tensor.allreduce (mesh + ring modes); the
  builtin accepts the full ReduceOp family (Sum/Max/Min/Prod) and FP16/FP32
  in both modes

Each uses the class-form rank-count factory (build_*(nr) -> @pl.program) so
one source serves any P via -d. Ships walkthroughs 13-16 (en + zh), the
tutorial-index shipped marks, chapter cross-links, mkdocs nav entries, and
examples-tests P=4 CI legs (the comparisons are only observable at P>=4).

Walkthroughs document the builtin accurately: ring mode lowers each round to
a full-mesh barrier (EmitNotifyAll/EmitWaitAll), not the hand-written
neighbour-ready handshake; ring strictly validates the [2*(nr-1), nr] signal
while mesh validates only a static column count of 1 (row count unchecked);
the ring per-step N/P transfer is constant under weak scaling; the handshake
is O(P) signals per rank (O(P^2) system-wide); and at P=2 rank 0's left
neighbour is rank 1 (the (my_rank-1) % nranks hazard is truncating modulo,
not self-neighbouring). Docs gates green (nav/parity 133/133, pre-commit,
mkdocs --strict); ring and reveal sim-validated at P=2/P=4 on a2a3sim.
Review follow-ups on steps 08-11 (mesh / two-phase / ring / reveal).

- 10_allreduce_ring: drop the redundant `chunk_elems` parameter. The host
  always passed `SIZE // nr`, which is the factory's existing compile-time
  `chunk` constant, so offsets and shapes were spelled two different ways on
  the same call. Offsets now use `chunk` throughout.

- Rank-count spelling is now consistent with steps 01-07: kernel bodies derive
  the count at runtime via `pld.nranks(ctx)` (08 was using the closure `nr` as
  a loop bound), and host orchestration loops use `pld.world_size()` (10 and 11
  were using `nr`). The closure `nr` stays reserved for what genuinely must be
  a compile-time constant: window and signal shapes.

- Document why steps 08-11 use `@pl.program` while steps 01-07 use `@pl.jit`.
  The switch is required, not stylistic: `@pl.program` / `@pl.function`
  snapshot the defining frame's locals at decoration time, so a factory's `nr`
  resolves inside the HOST orchestrator body; `@pl.jit.host` re-specializes
  into `@pl.function` after that frame is gone, so a closure `nr` referenced
  in its body fails to resolve. Recorded in 08's module docstring, its factory
  docstring, and 13-allreduce_mesh.md (en + zh).

- 16-allreduce_reveal: name the InCore composite explicitly in the ReduceOp
  paragraph and point at the narrower HOST-builtin ring contract, so the two
  rails are not conflated.

Verified on the a2a3 simulator at P=2 and P=4 for all four examples, plus
`--mode ring` for step 11.
…l-reduce

Step 08 never needed a compile-time rank count. Its barrier signal row count
is not a tile shape, so it can stay dynamic: NR = pl.dynamic("NR") in the
annotations and pld.world_size() in the host body. The example is now a
module-level @pl.program with no factory, matching
tests/st/distributed/collectives/test_l3_allreduce.py, the system test for
this same collective.

Correct the walkthrough's explanation, which was wrong in three ways:

- It claimed every rank-parametrized collective in tests/st/distributed/ uses
  a class-form factory. Three of five do not (allreduce, allgather, broadcast);
  they use a module-level class with pl.dynamic.
- It attributed the @pl.jit -> @pl.program switch to closure frame capture.
  The switch is required, but because @pl.jit must statically infer shape and
  dtype for every parameter it forwards to a dependency and cannot type a
  window dim of pld.world_size(); it is fine with distributed tensors as such
  (steps 03, 05, 06, 07).
- Its troubleshooting row told readers a [nr, 1] shape must be a closure
  constant, which would send them to "fix" working code.

Step 14 now states why steps 09 and 10 do need a factory: their chunk size
SIZE // nr is a tile shape, and tile shapes must be known at compile time.

Verified in the simulation image (pypto 1028651, pto-isa f51c92f6): all four
examples pass at P=2 and P=4, plus 11 --mode ring at P=4.
… form

The Ring Mode section documented a rank-count factory that defines @pl.jit
functions in its own body, on the stated grounds that "the specializer folds
the closure constants into the generated program". It does not: @pl.jit's
constant folding reads only the function's module globals (__globals__) and
never __closure__ (python/pypto/jit/specializer.py), so a factory closure
constant referenced in a HOST orchestrator body fails to resolve with
Undefined variable.

The section already noted that the test it cites uses the @pl.program class
form instead, which was the tell. Document the class form as the pattern, with
the reason it works (@pl.program / @pl.function snapshot the defining frame's
locals at decoration time), and state explicitly that the @pl.jit family does
not work here so readers stop rediscovering this.
@georgebisbas
georgebisbas force-pushed the feat/examples-distributed-allreduce branch from 1e285c4 to 3c67b1e Compare August 24, 2026 11:30
… 08 correction

Step 08 no longer has a rank-count factory, and the corrected explanation on
pages 13/14 is that what forces a compile-time `nr` in steps 09/10 is
`chunk = SIZE // nr` being a tile shape -- a signal row count is not. Three
example docstrings still carried the old rationale: 09 and 11 cited step 08 as
a factory precedent, and 09/10/11 all attributed the factory to the signal
shape.

- 09: the factory exists for the chunk tile shape alone; the `[2, nr]` signal
  follows from the same constant but would not have required it.
- 10: same single reason as 09; drops the "both must be statically known"
  claim, which contradicted the correction.
- 11: a different reason entirely. The builtin owns the chunking, so no tile
  shape depends on the rank count; what must be fixed at trace time is `mode`,
  which picks both the lowering and the signal layout (mesh `[nr, 1]` vs ring
  `[2*(nr-1), nr]` are different shapes, not two extents of one).

Verified the step 11 claim rather than asserting it: a module-level variant
with `NR = pl.dynamic("NR")` and a `[2*(NR-1), NR]` ring signal compiles and
passes its golden on a2a3sim at P=3 and P=4, so `nr` genuinely need not be
compile-time there.

Also states the same distinction in the step 11 walkthrough (en + zh), the last
page that still described the factory without it.
georgebisbas added a commit to georgebisbas/pypto that referenced this pull request Aug 25, 2026
…unt spelling

Carries the two review outcomes from the all-reduce ladder (hw-native-sys#2330) forward to
the collective zoo, which had drifted the same two ways.

The factory rationale was wrong in all five files, in the same way step 08's
was before it was corrected: each claimed the factory exists because the
`[nr, 1]` signal and `[nr, SIZE]` windows "must be statically known". None of
those is a tile shape, and a window shape does not force a compile-time rank
count -- `pld.tensor.broadcast`'s own docstring says the InCore mesh signal's
rank count may be dynamic.

Verified rather than argued: each of the five was rebuilt with
`NR = pl.dynamic("NR")` plus `pld.nranks(ctx)` / `pld.world_size()` and run on
a2a3sim at P=4 in both modes. All five compile and pass their goldens, so a
compile-time `nr` is required by none of them. The real reason 12-15 need a
factory is `use_builtin`: `host_orch` branches on it to choose which
per-device orchestrator to dispatch, so it must be a Python constant when the
body is traced. Step 16 has no mode flag and therefore no requirement at all;
its docstring now says so and notes it could become a module-level program.

Rank-count spelling now follows the convention the all-reduce steps converged
on: `nranks = pld.nranks(ctx)` for kernel loop bounds, `pld.world_size()` in
host orchestration, and `nr` left only where a shape wants a literal. The
walkthrough snippets in 17-21 (en + zh) are updated to match the source they
quote.

Verified: all 18 CI-documented invocations of steps 12-16 pass on a2a3sim
(P=2 and P=4, hand and builtin); pre-commit and `mkdocs build --strict` clean.
@lyfne123
lyfne123 merged commit 0172485 into hw-native-sys:main Aug 26, 2026
17 checks passed
georgebisbas added a commit to georgebisbas/pypto that referenced this pull request Aug 26, 2026
…unt spelling

Carries the two review outcomes from the all-reduce ladder (hw-native-sys#2330) forward to
the collective zoo, which had drifted the same two ways.

The factory rationale was wrong in all five files, in the same way step 08's
was before it was corrected: each claimed the factory exists because the
`[nr, 1]` signal and `[nr, SIZE]` windows "must be statically known". None of
those is a tile shape, and a window shape does not force a compile-time rank
count -- `pld.tensor.broadcast`'s own docstring says the InCore mesh signal's
rank count may be dynamic.

Verified rather than argued: each of the five was rebuilt with
`NR = pl.dynamic("NR")` plus `pld.nranks(ctx)` / `pld.world_size()` and run on
a2a3sim at P=4 in both modes. All five compile and pass their goldens, so a
compile-time `nr` is required by none of them. The real reason 12-15 need a
factory is `use_builtin`: `host_orch` branches on it to choose which
per-device orchestrator to dispatch, so it must be a Python constant when the
body is traced. Step 16 has no mode flag and therefore no requirement at all;
its docstring now says so and notes it could become a module-level program.

Rank-count spelling now follows the convention the all-reduce steps converged
on: `nranks = pld.nranks(ctx)` for kernel loop bounds, `pld.world_size()` in
host orchestration, and `nr` left only where a shape wants a literal. The
walkthrough snippets in 17-21 (en + zh) are updated to match the source they
quote.

Verified: all 18 CI-documented invocations of steps 12-16 pass on a2a3sim
(P=2 and P=4, hand and builtin); pre-commit and `mkdocs build --strict` clean.
georgebisbas added a commit to georgebisbas/pypto that referenced this pull request Aug 27, 2026
…unt spelling

Carries the two review outcomes from the all-reduce ladder (hw-native-sys#2330) forward to
the collective zoo, which had drifted the same two ways.

The factory rationale was wrong in all five files, in the same way step 08's
was before it was corrected: each claimed the factory exists because the
`[nr, 1]` signal and `[nr, SIZE]` windows "must be statically known". None of
those is a tile shape, and a window shape does not force a compile-time rank
count -- `pld.tensor.broadcast`'s own docstring says the InCore mesh signal's
rank count may be dynamic.

Verified rather than argued: each of the five was rebuilt with
`NR = pl.dynamic("NR")` plus `pld.nranks(ctx)` / `pld.world_size()` and run on
a2a3sim at P=4 in both modes. All five compile and pass their goldens, so a
compile-time `nr` is required by none of them. The real reason 12-15 need a
factory is `use_builtin`: `host_orch` branches on it to choose which
per-device orchestrator to dispatch, so it must be a Python constant when the
body is traced. Step 16 has no mode flag and therefore no requirement at all;
its docstring now says so and notes it could become a module-level program.

Rank-count spelling now follows the convention the all-reduce steps converged
on: `nranks = pld.nranks(ctx)` for kernel loop bounds, `pld.world_size()` in
host orchestration, and `nr` left only where a shape wants a literal. The
walkthrough snippets in 17-21 (en + zh) are updated to match the source they
quote.

Verified: all 18 CI-documented invocations of steps 12-16 pass on a2a3sim
(P=2 and P=4, hand and builtin); pre-commit and `mkdocs build --strict` clean.
georgebisbas added a commit to georgebisbas/pypto that referenced this pull request Aug 28, 2026
…unt spelling

Carries the two review outcomes from the all-reduce ladder (hw-native-sys#2330) forward to
the collective zoo, which had drifted the same two ways.

The factory rationale was wrong in all five files, in the same way step 08's
was before it was corrected: each claimed the factory exists because the
`[nr, 1]` signal and `[nr, SIZE]` windows "must be statically known". None of
those is a tile shape, and a window shape does not force a compile-time rank
count -- `pld.tensor.broadcast`'s own docstring says the InCore mesh signal's
rank count may be dynamic.

Verified rather than argued: each of the five was rebuilt with
`NR = pl.dynamic("NR")` plus `pld.nranks(ctx)` / `pld.world_size()` and run on
a2a3sim at P=4 in both modes. All five compile and pass their goldens, so a
compile-time `nr` is required by none of them. The real reason 12-15 need a
factory is `use_builtin`: `host_orch` branches on it to choose which
per-device orchestrator to dispatch, so it must be a Python constant when the
body is traced. Step 16 has no mode flag and therefore no requirement at all;
its docstring now says so and notes it could become a module-level program.

Rank-count spelling now follows the convention the all-reduce steps converged
on: `nranks = pld.nranks(ctx)` for kernel loop bounds, `pld.world_size()` in
host orchestration, and `nr` left only where a shape wants a literal. The
walkthrough snippets in 17-21 (en + zh) are updated to match the source they
quote.

Verified: all 18 CI-documented invocations of steps 12-16 pass on a2a3sim
(P=2 and P=4, hand and builtin); pre-commit and `mkdocs build --strict` clean.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

Status: No status

Development

Successfully merging this pull request may close these issues.

3 participants