Skip to content

Conversation

@frost-intel
Copy link
Contributor

@frost-intel frost-intel commented Oct 21, 2025

Group UID needs to be set before creating the LogPrefix, and the initialization list order is fixed now.

Upstream PR will cause options to be properly set.

@Copilot Copilot AI review requested due to automatic review settings October 21, 2025 19:30
Copy link
Contributor

Copilot AI left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull Request Overview

This PR fixes the initialization order of ProcessGroupXCCL options to ensure the group UID is set before creating the log prefix. The key changes include reordering the member initialization list and moving the setGroupUid call before createLogPrefix().

  • Moved options_ initialization after local_id_ in the member initialization list
  • Relocated setGroupUid call to execute before createLogPrefix()

Tip: Customize your code reviews with copilot-instructions.md. Create the file or learn how to get started.

@frost-intel frost-intel requested a review from Chao1Han October 21, 2025 19:46
Copy link
Contributor

@dvrogozh dvrogozh left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Upstream PR will cause options to be properly set.

@frost-intel, can you, please, clarify how your PR relates with the upstream PR mentioned above? Is upstream PR a prerequisite to this one or vice versa? i.e. which one should be merged first or they are independent?

@frost-intel
Copy link
Contributor Author

@dvrogozh Both PRs are necessary to fix a specific bug, where the ProcessGroupXCCL::Options was not being set properly when passed as an argument. However, the two PRs are not dependent on each other. There are two different bugs which have the same effect. The upstream PR fixes the issue of Options not being writable, and this PR fixes a problem with the initialization list order and the proper value not being set before it was used by createLogPrefix.

Copy link
Contributor

@dvrogozh dvrogozh left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants