-
Notifications
You must be signed in to change notification settings - Fork 47
[Feature] Relax the divisibility constraint in P2pNcclAFDConnector #309
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Merged
specture724
merged 3 commits into
vllm-project:main
from
swjeong9:feature/arbitrary-m2n
Sep 11, 2026
Merged
Changes from all commits
Commits
Show all changes
3 commits
Select commit
Hold shift + click to select a range
File filter
Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
There are no files selected for viewing
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,112 @@ | ||
| # SPDX-License-Identifier: Apache-2.0 | ||
| # SPDX-FileCopyrightText: Copyright contributors to the AFD plugin project | ||
| """Subgroup partition tests for the P2P rank mapping. | ||
|
|
||
| ``build_rank_mapping`` spreads the Attention ranks over the FFN ranks in | ||
| contiguous blocks that differ in size by at most one (attention ``a`` joins | ||
| the subgroup of FFN rank ``a * F // A``). When ``F`` divides ``A`` this is the | ||
| grouping the connector has always built, which | ||
| ``test_divisible_layouts_keep_the_historical_grouping`` pins; the other tests | ||
| cover every ``A >= F`` pair, including the layouts that grouping could not | ||
| express. | ||
| """ | ||
|
|
||
| from __future__ import annotations | ||
|
|
||
| import pytest | ||
|
|
||
| from afd_plugin.config import AFDConfig | ||
| from afd_plugin.distributed.topology import build_rank_mapping | ||
|
|
||
| # Every A >= F pair with A, F in 1..8, divisible and not. | ||
| _GRID = [(a, f) for a in range(1, 9) for f in range(1, a + 1)] | ||
|
|
||
|
|
||
| def _config(role: str, attention: int, ffn: int) -> AFDConfig: | ||
| return AFDConfig( | ||
| role=role, | ||
| connector="P2pNcclAFDConnector", | ||
| num_attention_ranks=attention, | ||
| num_ffn_ranks=ffn, | ||
| ) | ||
|
|
||
|
|
||
| def _mapping(role: str, role_rank: int, attention: int, ffn: int): | ||
| return build_rank_mapping(_config(role, attention, ffn), role_rank) | ||
|
|
||
|
|
||
| @pytest.mark.parametrize(("attention", "ffn"), _GRID) | ||
| def test_every_attention_rank_belongs_to_exactly_one_subgroup(attention, ffn): | ||
| subgroups = { | ||
| ffn_rank: _mapping("ffn", ffn_rank, attention, ffn).subgroup_ranks | ||
| for ffn_rank in range(ffn) | ||
| } | ||
|
|
||
| # The FFN rank leads its own subgroup, and the Attention members partition | ||
| # the Attention world in world order. | ||
| assert [ranks[0] for ranks in subgroups.values()] == list(range(ffn)) | ||
| peers = [rank for ranks in subgroups.values() for rank in ranks[1:]] | ||
| assert peers == list(range(ffn, ffn + attention)) | ||
|
|
||
| # Block sizes differ by at most one, and none is empty. | ||
| sizes = [len(ranks) - 1 for ranks in subgroups.values()] | ||
| assert min(sizes) >= 1 | ||
| assert max(sizes) - min(sizes) <= 1 | ||
|
|
||
|
|
||
| @pytest.mark.parametrize(("attention", "ffn"), _GRID) | ||
| def test_both_roles_agree_on_the_subgroup_they_share(attention, ffn): | ||
| for attention_rank in range(attention): | ||
| mapping = _mapping("attention", attention_rank, attention, ffn) | ||
| owner = _mapping("ffn", mapping.subgroup_index, attention, ffn) | ||
| assert mapping.subgroup_ranks == owner.subgroup_ranks | ||
| assert mapping.subgroup_ranks[mapping.rank_in_subgroup] == mapping.world_rank | ||
|
|
||
|
|
||
| @pytest.mark.parametrize(("attention", "ffn"), _GRID) | ||
| def test_world_and_p2p_ranks_follow_the_role_layout(attention, ffn): | ||
| min_size = min(attention, ffn) | ||
| dp_destinations: list[int] = [] | ||
| for role, size in (("ffn", ffn), ("attention", attention)): | ||
| for role_rank in range(size): | ||
| mapping = _mapping(role, role_rank, attention, ffn) | ||
| assert mapping.min_size == min_size | ||
| if role == "ffn": | ||
| assert mapping.world_rank == role_rank | ||
| assert mapping.p2p_rank == role_rank | ||
| else: | ||
| assert mapping.world_rank == ffn + role_rank | ||
| assert mapping.p2p_rank == role_rank + min_size | ||
| dp_destinations.extend(mapping.dp_metadata_destinations) | ||
|
|
||
| # Only Attention ranks send DP metadata, and each FFN rank receives it | ||
| # from exactly one of them. | ||
| assert sorted(dp_destinations) == list(range(ffn)) | ||
|
|
||
|
|
||
| @pytest.mark.parametrize( | ||
| ("attention", "ffn", "expected"), | ||
| [ | ||
| (3, 2, [(0, 2, 3), (1, 4)]), | ||
| (5, 2, [(0, 2, 3, 4), (1, 5, 6)]), | ||
| (5, 3, [(0, 3, 4), (1, 5, 6), (2, 7)]), | ||
| (6, 4, [(0, 4, 5), (1, 6), (2, 7, 8), (3, 9)]), | ||
| ], | ||
| ) | ||
| def test_partition_literal_examples(attention, ffn, expected): | ||
| assert [ | ||
| _mapping("ffn", ffn_rank, attention, ffn).subgroup_ranks | ||
| for ffn_rank in range(ffn) | ||
| ] == expected | ||
|
|
||
|
|
||
| @pytest.mark.parametrize(("attention", "ffn"), [(a, f) for a, f in _GRID if a % f == 0]) | ||
| def test_divisible_layouts_keep_the_historical_grouping(attention, ffn): | ||
| ratio = attention // ffn | ||
| for ffn_rank in range(ffn): | ||
| mapping = _mapping("ffn", ffn_rank, attention, ffn) | ||
| assert len(mapping.subgroup_ranks) - 1 == ratio | ||
| assert mapping.subgroup_ranks == ( | ||
| ffn_rank, | ||
| *(ffn + ffn_rank * ratio + offset for offset in range(ratio)), | ||
| ) |
Oops, something went wrong.
Oops, something went wrong.
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
IMHO, is this rank division logic the same as
ffn_metadata.py: L42-53? If so, make it a helper:There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thank you for the review. I agree that consolidating the two expressions into a shared helper makes the code clearer. Applied as suggested.