Repository navigation
Allow any site to be the root of a hierarchical communicator - #7829
Conversation
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info
📝 Summary
Merge Risk: ⚪ Minimal · up to
|
| Check name | Status | Explanation | Resolution |
|---|---|---|---|
| Docstring Coverage |  |
Docstring coverage is 30.61% which is insufficient. The required threshold is 80.00%. Docstring coverage is scoped to functions touched by this diff. Analyzed 98 functions across 29 files. | Write docstrings for the functions missing them to satisfy the coverage threshold. |
-
Check name Status Explanation Title check The title "Allow any site to be the root of a hierarchical communicator" matches the main change. The raw summary and PR description both show that hierarchical communicators now accept any site as ro… Description check The description states that the hierarchical communicator tree was always rooted at site 0 and that root_site now works for any site. It explains the leader selection rule for ranges, the root/ com… Linked Issues check Check skipped because no linked issues were found for this pull request. Out of Scope Changes check Check skipped because no linked issues were found for this pull request. Full details: Title check
-
Explanation
The title "Allow any site to be the root of a hierarchical communicator" matches the main change. The raw summary and PR description both show that hierarchical communicators now accept any site as root_site, where previously the tree was always rooted at site 0. The title is specific, concise, and free of file lists or vague wording.
Full details: Description check
-
Explanation
The description states that the hierarchical communicator tree was always rooted at site 0 and that root_site now works for any site. It explains the leader selection rule for ranges, the root<N>/ communicator name prefix for non-zero roots, the call-site restrictions for root-side operations, and the expanded test coverage. This matches the changeset in the summaries.
-
✨ Finishing Touches
-
🧪 Generate unit tests (beta)
-
- Create a new PR
-
- Autofix · Keep fixing CodeRabbit findings and required CI, and resolving merge conflicts
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 @coderabbitai help to get the list of available commands.
|
Can one of the admins verify this patch? |
Up to standards ✅🟢 Issues
|
hkaiser
left a comment
hkaiser
left a comment
There was a problem hiding this comment.
Thanks, this is a nice generalization. I went through the tree construction and the walks: since leaders nest, the root leads every range containing it, each sub-communicator is created with the leader's rank as its root on all sites, and communicators.site(i) is exactly that rank wherever the code used to pass this_site_arg(0). Gather, scatter and the scans keep the site order. I built the PR locally (Windows, Release, TCP parcelport) and all 17 affected collectives tests pass.
The DCO check fails: the commit has no Signed-off-by line. Please amend it with git commit --amend -s and force-push.
Two small things about the documentation of root_site, see the inline comments.
The tree of a hierarchical communicator was always rooted at site 0. Make root_site work for any site: the leader of a range of sites is the root if the range contains it and its leftmost site otherwise. Leaders nest, so the root takes part in every level of the tree while the ranges stay contiguous in site order. Gather, scatter and the scans keep their site order without any rotation, and reductions need no commutativity. The communicator names of a non-zero root get a root<N>/ prefix, so different roots can share a base name; the names of root 0 are unchanged. The root-side operations (broadcast_to, gather_here, reduce_here, scatter_to) must be called from the root site, and the composite operations accept only the default or the root of the communicator as root_site. Every hierarchical collective test now also runs with other roots, including across localities, and hierarchical_helpers checks which sites lead which ranges for every root. Signed-off-by: constracktor <[email protected]>
a8ef320 to
49aef0f
Compare
|
The remaining CI problem is unrelated. |
Describe the PRs merged since release-2.0.X was last rebuilt on master (#7769, #7784, #7787, #7829, #7832, #7833, #7835, #7836, #7838, #7839, #7845, #7855, #7858, #7860, #7864, #7865, #7867), and state in the C++20 modules section and under Breaking changes that applications must include only the public headers and the generated hpx/modules/<module>.hpp headers, not the headers inside an HPX module. Regenerate the closed issue and pull request lists for release-2.0.X at b059b41 against the v1.11.0 tag (795 PRs, 178 issues). Signed-off-by: Hartmut Kaiser <[email protected]>
Describe the PRs merged since release-2.0.X was last rebuilt on master (#7769, #7784, #7787, #7829, #7832, #7833, #7835, #7836, #7838, #7839, #7845, #7855, #7858, #7860, #7864, #7865, #7867), and state in the C++20 modules section and under Breaking changes that applications must include only the public headers and the generated hpx/modules/<module>.hpp headers, not the headers inside an HPX module. Regenerate the closed issue and pull request lists for release-2.0.X at b059b41 against the v1.11.0 tag (795 PRs, 178 issues). Signed-off-by: Hartmut Kaiser <[email protected]>
The tree of a hierarchical communicator was always rooted at site 0. Make root_site work for any site: the leader of a range of sites is the root if the range contains it and its leftmost site otherwise. Leaders nest, so the root takes part in every level of the tree while the ranges stay contiguous in site order. Gather, scatter and the scans keep their site order without any rotation, and reductions need no commutativity.
The communicator names of a non-zero root get a root/ prefix, so different roots can share a base name; the names of root 0 are unchanged. The root-side operations (broadcast_to, gather_here, reduce_here, scatter_to) must be called from the root site, and the composite operations accept only the default or the root of the communicator as root_site.
Every hierarchical collective test now also runs with other roots, including across localities, and hierarchical_helpers checks which sites lead which ranges for every root.