support high priority stream #1715
Conversation
There was a problem hiding this comment.
Pull Request Overview
This pull request adds support for high priority streams in the XCCL process group. Key changes include adding a new Options struct with high priority and group name parameters, introducing a new groupRanks() accessor, and updating constructor and logging logic to reflect high priority stream usage.
Reviewed Changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 1 comment.
| File | Description |
|---|---|
| src/xccl/ProcessGroupXCCL.hpp | Added a high priority stream option and new Options struct for configuration. |
| src/xccl/ProcessGroupXCCL.cpp | Updated constructor initialization, logging, and introduced groupRanks(). |
Comments suppressed due to low confidence (1)
src/xccl/ProcessGroupXCCL.hpp:25
- The constant TORCH_XCCL_HIGH_PRIORITY is defined as a non-const vector. Consider renaming and declaring it as a const container (or using constexpr) to clearly indicate its immutability.
static std::vector<std::string> TORCH_XCCL_HIGH_PRIORITY = {
| @@ -290,14 +290,26 @@ const std::string& ProcessGroupXCCL::logPrefix() const { | |||
| return logPrefix_; | |||
| } | |||
|
|
|||
| const std::vector<uint64_t>& ProcessGroupXCCL::groupRanks() const { | |||
There was a problem hiding this comment.
Returning a reference to a static vector that is only populated when options_->global_ranks_in_group is empty and local_id_ == 0 may lead to inconsistent behavior and potential thread-safety issues. Consider moving the default global ranks logic into the Options struct or ensuring proper synchronization.
Copilot uses AI. Check for mistakes.
No description provided.