docs: add architecture + get_m2m_map diagrams and design walkthrough - #110
Conversation
📝 WalkthroughWalkthroughAdds documentation for the DTensor M-to-N redistribution mapping: a README architecture image and core subsection introducing get_m2m_map, plus a new design document detailing the LCM-based alignment, stamping/distribution, decode/redistribute, and all-gather pipeline; index updated. ChangesM-to-N DTensor redistribution documentation
🎯 1 (Trivial) | ⏱️ ~3 minutes
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
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 |
|
Failed to generate code suggestions for PR |
![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 `@README.md`:
- Line 20: The heading "The core: the M-to-N communication map" is currently an
h3 and skips h2; change its markdown level from ### to ## so it becomes a proper
h2 and restores the document heading hierarchy.
🪄 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: defaults
Review profile: CHILL
Plan: Pro
Run ID: e312538c-bd99-4b15-a93d-f456a92f10ef
⛔ Files ignored due to path filters (2)
docs/design/etha_arch.pngis excluded by!**/*.pngdocs/design/etha_m2m_map.pngis excluded by!**/*.png
📒 Files selected for processing (3)
README.mddocs/design/get-m2m-map.mddocs/design/index.md
b768fb7 to
0f334d7
Compare
There was a problem hiding this comment.
♻️ Duplicate comments (1)
README.md (1)
20-20:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winFix heading level to follow markdown hierarchy.
The heading jumps from h1 (line 1) to h3 without an intervening h2. Change to h2 (##) to maintain proper document structure.
📐 Proposed fix
-### The core: the M-to-N communication map +## The core: the M-to-N communication map🤖 Prompt for 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. In `@README.md` at line 20, The heading "The core: the M-to-N communication map" is currently an h3 (###) and skips an h2 level; update that heading to h2 (##) in README.md so it follows the document hierarchy and sits under the top-level title, i.e., replace the "### The core: the M-to-N communication map" line with "## The core: the M-to-N communication map".
🤖 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.
Duplicate comments:
In `@README.md`:
- Line 20: The heading "The core: the M-to-N communication map" is currently an
h3 (###) and skips an h2 level; update that heading to h2 (##) in README.md so
it follows the document hierarchy and sits under the top-level title, i.e.,
replace the "### The core: the M-to-N communication map" line with "## The core:
the M-to-N communication map".
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: a4d68b2b-3b8b-4d6e-8e57-9d7fdea521ea
⛔ Files ignored due to path filters (2)
docs/design/etha_arch.pngis excluded by!**/*.pngdocs/design/etha_m2m_map.pngis excluded by!**/*.png
📒 Files selected for processing (3)
README.mddocs/design/get-m2m-map.mddocs/design/index.md
✅ Files skipped from review due to trivial changes (1)
- docs/design/index.md
274a332 to
fa38df9
Compare
Add an M-to-N redistribution overview banner to the README header and a get_m2m_map algorithm diagram with a step-by-step design walkthrough under docs/design/. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
fa38df9 to
502e1c2
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
README.md (1)
47-49:⚠️ Potential issue | 🟠 Major | ⚡ Quick winCorrect the source
Partialbehavior description to match the real contract.The current note says source
Partialis collapsed via a pre-send all-reduce, but the implementation contract is more specific: source-sidePartialis traced asReplicateand then reduce-only shadow entries are added; target-sidePartialis rejected. Please align this wording to avoid misleading users about when/where reduction happens.Based on learnings: “
Partialin placements can corrupt map tracing viafull_tensor(); it does not silently fall back toReplicatebehavior.”🤖 Prompt for 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. In `@README.md` around lines 47 - 49, Update the README description under "Placements" to accurately describe source-side Partial semantics: state that source `Partial` is traced as `Replicate` for map tracing and additionally has reduce-only shadow entries added on the source (not an all-reduce collapse before send), while target `Partial` remains rejected; mention that using `Partial` can affect map tracing via `full_tensor()` rather than silently falling back to `Replicate`. Reference the placement names `Shard`, `Replicate`, `Partial` and the `full_tensor()` behavior so readers see the exact contract.
🧹 Nitpick comments (1)
docs/design/get-m2m-map.md (1)
15-15: ⚡ Quick winAdd fenced code block languages to satisfy markdown lint.
Please annotate these fences (for example with
text) so MD040 does not flag the document.Also applies to: 43-43, 58-58, 74-74, 97-97, 115-115
🤖 Prompt for 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. In `@docs/design/get-m2m-map.md` at line 15, The markdown file has several fenced code blocks without language annotations causing MD040; update each backtick fence (the code blocks at the locations mentioned) to include a language token (e.g., add "text" after the opening ```). Locate the empty fences in get-m2m-map.md (the code block markers around the sample snippets) and change them from ``` to ```text for each occurrence so the linter stops flagging MD040.
🤖 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/design/get-m2m-map.md`:
- Around line 59-60: The sentence "each rank's row-shard is gathered back into
the whole 4×2 tensor, then sent" is misleading; update the wording to state that
each source rank performs a per-source send (fan-out) to its assigned target
rank(s) rather than implying a single global send. Specifically locate that
exact phrase and change it to something like "each source rank gathers its
row-shard into the 4×2 tensor and then sends that shard to one or more assigned
target ranks (per-source fan-out)," and make the same edit for the identical
occurrence at the other location.
---
Outside diff comments:
In `@README.md`:
- Around line 47-49: Update the README description under "Placements" to
accurately describe source-side Partial semantics: state that source `Partial`
is traced as `Replicate` for map tracing and additionally has reduce-only shadow
entries added on the source (not an all-reduce collapse before send), while
target `Partial` remains rejected; mention that using `Partial` can affect map
tracing via `full_tensor()` rather than silently falling back to `Replicate`.
Reference the placement names `Shard`, `Replicate`, `Partial` and the
`full_tensor()` behavior so readers see the exact contract.
---
Nitpick comments:
In `@docs/design/get-m2m-map.md`:
- Line 15: The markdown file has several fenced code blocks without language
annotations causing MD040; update each backtick fence (the code blocks at the
locations mentioned) to include a language token (e.g., add "text" after the
opening ```). Locate the empty fences in get-m2m-map.md (the code block markers
around the sample snippets) and change them from ``` to ```text for each
occurrence so the linter stops flagging MD040.
🪄 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: defaults
Review profile: CHILL
Plan: Pro
Run ID: 90c47bca-d26d-4039-9dd6-6e96ef7dfe40
⛔ Files ignored due to path filters (2)
docs/design/etha_arch.pngis excluded by!**/*.pngdocs/design/etha_m2m_map.pngis excluded by!**/*.png
📒 Files selected for processing (3)
README.mddocs/design/get-m2m-map.mddocs/design/index.md
✅ Files skipped from review due to trivial changes (1)
- docs/design/index.md
| each rank's row-shard is gathered back into the whole 4×2 tensor, then sent | ||
|
|
There was a problem hiding this comment.
Clarify “sent once” to avoid an incorrect transfer model.
This wording implies a single global send, but the implementation sends from each source rank to one or more assigned target ranks. Please rephrase to reflect per-source-rank send fan-out where applicable.
Also applies to: 123-123
🤖 Prompt for 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.
In `@docs/design/get-m2m-map.md` around lines 59 - 60, The sentence "each rank's
row-shard is gathered back into the whole 4×2 tensor, then sent" is misleading;
update the wording to state that each source rank performs a per-source send
(fan-out) to its assigned target rank(s) rather than implying a single global
send. Specifically locate that exact phrase and change it to something like
"each source rank gathers its row-shard into the 4×2 tensor and then sends that
shard to one or more assigned target ranks (per-source fan-out)," and make the
same edit for the identical occurrence at the other location.
2 participants
Adds an M-to-N redistribution overview banner to the README header, plus a
get_m2m_mapalgorithm diagram and a step-by-step design walkthrough underdocs/design/.docs/design/etha_arch.png— overview banner (README header)docs/design/etha_m2m_map.png— get_m2m_map algorithm diagramdocs/design/get-m2m-map.md— step-by-step walkthrough (linked from README), added to the design toctreeHeads-up: the
etha_m2m_map.pngrender still carries two known errors (Stage 2formula is missing the
c0*5term; Stage 6 row3·(0,1)should map to3·(1,0)).To be replaced with a corrected render before/after merge.
🤖 Generated with Claude Code
Summary by CodeRabbit