Repository navigation
Conversation
|
Thanks @jinyouzhi, this is a nice speedup for the check I added in #8922, and falling back to the pairwise walk keeps the error messages exactly as they were. I checked it against master (b9100c5) by running both on the same 20,000 random single-rank maps. Each map tiles a 1-D to 3-D shard grid with random cuts, then sometimes changes one piece: shifting it, swapping two pieces' destinations, moving a destination by 1–3 elements, starting it at a random address so it wraps past a row or leaves the shard, or giving a size-1 axis an arbitrary stride. One small suggestion on the new test, not blocking: its second assertion monkeypatches LGTM. |
|
Thanks @Achyuthan-S for the differential run against master. Good point about the test: 06edb58 drops the |
06edb58 to
ac3a329
Compare
_validate_holders compared every piece with every other piece, and _sources compared every source region with every other region, so a map with 1024 pieces already exceeded the default max_pair_checks and planning time grew quadratically. When no rank repeats a box and distinct boxes do not overlap, a rank covers a box exactly when it holds that very box. The holders of each box can then be read from a table. Add boxes_disjoint to affine_geometry: it sweeps along the most varied axis and compares a box only with the boxes still open where it starts. Maps that fail the fast checks take the original pairwise walk, which names the offending piece, so error messages are unchanged. A randomized test checks that plans and errors match the pairwise walk, including replicas split at different points, overlapping pieces and wrong holder lists. Signed-off-by: Jin, Youzhi <youzhi.jin@intel.com>
A rank whose pieces sweep as disjoint in its shard and fill it passes _shards_tile with boxes_disjoint; any other rank takes the pairwise walk, so error messages are unchanged. Maps that pass no longer hit the per-rank len(held)**2 budget. Suggested-by: 0z5a Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> Signed-off-by: Jin, Youzhi <youzhi.jin@intel.com>
A disjoint layout can keep every box open for the whole sweep, so boxes_disjoint was still quadratic without a budget of its own. Give it an optional max_checks: once exceeded it reports the boxes as not shown disjoint, and since the sweep compares at most n*(n-1)/2 pairs, the caller's pairwise fallback under the same budget refuses at once. The planner passes max_pair_checks at every call. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> Signed-off-by: Jin, Youzhi <youzhi.jin@intel.com>
…pairwise walk Signed-off-by: Jin, Youzhi <youzhi.jin@intel.com>
78708fb to
b2ddefe
Compare
Codex Review Summary: ✅ CompletedThis comment shows the latest Codex review activity on this pull request.
ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
Codex reacts with 👀 while any review is running, comments if it has suggestions, and reacts with 👍 once all reviews finish with no findings. |
ParamAffineMap._validate_destinations compared every box-shaped piece of a rank with every other one, and validate() runs whenever a ZeRO, AutoEP or block-gather map is built. Sweep the boxes with boxes_disjoint first; only a rank that fails the sweep takes the pairwise walk, which names the shared address, so error messages are unchanged. Validating 2048 interleaved column pieces per rank drops from 6.7 s to 68 ms. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> Signed-off-by: Jin, Youzhi <youzhi.jin@intel.com>
_dest_box() now returns a LogicalBox built with coordinate(), so _validate_destinations() hands boxes straight to boxes_disjoint() and names a shared address from box.intersection() and dot(). The hand-written _first_shared_address() and divmod loop are gone. address_range() and overlaps_itself() move to affine_geometry. The transfer executor reuses address_range() for its buffer-reach check and dot() for chunk offsets. Error messages and behavior are unchanged. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> Signed-off-by: Jin, Youzhi <youzhi.jin@intel.com>
The randomized destination test monkeypatched affine.boxes_disjoint to compare the sweep with the pairwise walk, which ties it to that helper. Check the contract instead: a refusal names an address the map writes twice. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> Signed-off-by: Jin, Youzhi <youzhi.jin@intel.com>
gpu-test-labeler
Bot
added
the
skip-gpu-test
Stacked on #9011 (only the last three commits are new here; #9010 has merged, and I will rebase after #9011 merges).
ParamAffineMap._validate_destinations(added in #8922) checks the box-shaped pieces of a column split by comparing every box of a rank with every other one.validate()runs whenever a ZeRO, AutoEP or block-gather map is built and before every conversion, and this pass has no budget, so validation grows quadratically with the number of strided pieces per rank.This PR sweeps the boxes with
boxes_disjointfrom #9011 first. Only a rank that fails the sweep takes the pairwise walk, which names the shared shard address, so error messages are unchanged.The second commit moves the remaining hand-written geometry onto
affine_geometry:_dest_boxreturns aLogicalBoxbuilt withcoordinate(), the shared address comes fromintersection()anddot(), andaddress_range()/overlaps_itself()move there fromaffine.py. The transfer executor reusesaddress_range()for its buffer-reach check anddot()for chunk offsets. No behavior or message changes.validate()of 8 ranks with interleaved columns (64 rows; ms, single CPU process):Tests:
validate()refuses exactly the maps whose pieces write some shard address twice (enumerated independently in the test), and every refusal names an address that is written twice. The test checks this contract only, so it does not depend on which helper does the sweep.tests/unit/checkpoint/test_*affine*files: 839 passed, 1 skipped on 4× NVIDIA L20 at f6e1f6c. At ac3a329 on CPU (DS_ACCELERATOR=cpu, 4 ranks): 842 passed, 5 skipped.AI assistance
Implementation, tests and this description were drafted with GitHub Copilot CLI and reviewed/validated by the author.