Skip to content

Conversation

wconstab
Copy link
Contributor

@wconstab wconstab commented Nov 3, 2023

Stack from ghstack (oldest at bottom):

After adding support for labeler, we don't need CODEOWNERS.

This change will cause the distributed team members previously listed in
CODEOWNERS to stop being auto-added as reviewers on PRs touching these
files. The preceding PR adds labeler support for these same sets of
files, and contains instructions for adding yourself to be cc'd for that
label.

It is preferable to be auto-cc'd rather than auto-tagged as reviewer, so
that there is more signal in the reviewers list (either someone opted in
which shows the PR author someone is likely looking at it, or the PR
author added someone specifically which is a stronger notification to
the tagged reviewer than the blanket CODEOWNERS behavior.

cc @mrshenli @pritamdamania87 @zhaojuanmao @satgera @rohan-varma @gqchen @aazzolini @osalpekar @jiayisuse @H-Huang @kwen2501 @awgu @penguinwu @fegin @XilunWu @wanchaol @fduwjj @wz337 @tianyu-l @yf225 @kiukchung @d4l3k @LucasLLC

After adding support for labeler, we don't need CODEOWNERS.

This change will cause the distributed team members previously listed in
CODEOWNERS to stop being auto-added as reviewers on PRs touching these
files.  The preceding PR adds labeler support for these same sets of
files, and contains instructions for adding yourself to be cc'd for that
label.

It is preferable to be auto-cc'd rather than auto-tagged as reviewer, so
that there is more signal in the reviewers list (either someone opted in
which shows the PR author someone is likely looking at it, or the PR
author added someone specifically which is a stronger notification to
the tagged reviewer than the blanket CODEOWNERS behavior.

[ghstack-poisoned]
Copy link

pytorch-bot bot commented Nov 3, 2023

🔗 Helpful Links

🧪 See artifacts and rendered test results at hud.pytorch.org/pr/112813

Note: Links to docs will display an error until the docs builds have been completed.

✅ No Failures

As of commit 0d2b91d with merge base 75adb9f (image):
💚 Looks good so far! There are no failures yet. 💚

This comment was automatically generated by Dr. CI and updates every 15 minutes.

Copy link
Collaborator

@wanchaol wanchaol left a comment

Choose a reason for hiding this comment

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

lgtm

After adding support for labeler, we don't need CODEOWNERS.

This change will cause the distributed team members previously listed in
CODEOWNERS to stop being auto-added as reviewers on PRs touching these
files.  The preceding PR adds labeler support for these same sets of
files, and contains instructions for adding yourself to be cc'd for that
label.

It is preferable to be auto-cc'd rather than auto-tagged as reviewer, so
that there is more signal in the reviewers list (either someone opted in
which shows the PR author someone is likely looking at it, or the PR
author added someone specifically which is a stronger notification to
the tagged reviewer than the blanket CODEOWNERS behavior.

[ghstack-poisoned]
wconstab added a commit that referenced this pull request Nov 3, 2023
After adding support for labeler, we don't need CODEOWNERS.

This change will cause the distributed team members previously listed in
CODEOWNERS to stop being auto-added as reviewers on PRs touching these
files.  The preceding PR adds labeler support for these same sets of
files, and contains instructions for adding yourself to be cc'd for that
label.

It is preferable to be auto-cc'd rather than auto-tagged as reviewer, so
that there is more signal in the reviewers list (either someone opted in
which shows the PR author someone is likely looking at it, or the PR
author added someone specifically which is a stronger notification to
the tagged reviewer than the blanket CODEOWNERS behavior.

ghstack-source-id: bb877ef
Pull Request resolved: #112813
@fegin
Copy link
Contributor

fegin commented Nov 3, 2023

Should we just let users to remove themselves from the CODEOWNERS? Some people may still want to get the notifications under distributed?

@wconstab
Copy link
Contributor Author

wconstab commented Nov 3, 2023

Should we just let users to remove themselves from the CODEOWNERS? Some people may still want to get the notifications under distributed?

well, generally we should let people decide for their own settings.

But also i think its helpful to totally clear out CODEOWNERS. This is the only way to solve the problem that 'reviewers' are auto-populated, which makes looking at a PR to see who is actually doing a review much harder. So i prefer to just remove everyone from there.

However, everyone added to the label issue will still receive notifications. That's bc the bot will @cc them on the issue. So it should be a fairly smooth transition.

@wconstab wconstab closed this Nov 3, 2023
@wconstab wconstab reopened this Nov 3, 2023
@wconstab
Copy link
Contributor Author

wconstab commented Nov 3, 2023

@mrshenli @zhaojuanmao @rohan-varma @kiukchung @d4l3k I hesitated about whether to migrate you from CODEOWNERS over to labeler. In the end i did add all of you to labeler, so you'll be CC'd on all the same PRs you used to be tagged on as reviewer. If you are no longer interested in these notifications you can just remove yourself by editing the summary of #24422 directly, no need for a stamp or approval.

@wconstab
Copy link
Contributor Author

wconstab commented Nov 3, 2023

@pytorchbot merge

@pytorch-bot pytorch-bot bot added the ciflow/trunk Trigger trunk jobs on your pull request label Nov 3, 2023
@pytorchmergebot
Copy link
Collaborator

Merge started

Your change will be merged once all checks pass (ETA 0-4 Hours).

Learn more about merging in the wiki.

Questions? Feedback? Please reach out to the PyTorch DevX Team

Advanced Debugging
Check the merge workflow status
here

@wconstab
Copy link
Contributor Author

wconstab commented Nov 3, 2023

@pytorchbot merge -ic

Copy link

pytorch-bot bot commented Nov 3, 2023

-ic flag is deprecated, please use -i instead for the same effect.

@wconstab
Copy link
Contributor Author

wconstab commented Nov 3, 2023

@pytorchbot merge -i

@pytorchmergebot
Copy link
Collaborator

Merge started

Your change will be merged while ignoring the following 17 checks: pull / linux-focal-py3.11-clang10 / test (default, 1, 3, linux.2xlarge), pull / linux-focal-py3.11-clang10 / test (crossref, 1, 2, linux.2xlarge), pull / linux-focal-py3.11-clang10 / test (dynamo, 1, 2, linux.2xlarge), pull / linux-focal-py3.8-clang10 / test (default, 1, 3, linux.2xlarge), pull / linux-focal-py3.8-clang10 / test (crossref, 1, 2, linux.2xlarge), pull / linux-focal-py3.8-clang10 / test (dynamo, 1, 2, linux.2xlarge), pull / linux-jammy-py3.8-gcc11 / test (default, 1, 3, linux.2xlarge), pull / linux-jammy-py3.10-clang15-asan / test (default, 1, 6, linux.4xlarge), pull / linux-focal-cuda12.1-py3.10-gcc9 / test (default, 1, 5, linux.4xlarge.nvidia.gpu), pull / linux-focal-cuda12.1-py3.10-gcc9-sm86 / test (default, 1, 5, linux.g5.4xlarge.nvidia.gpu), trunk / macos-12-py3-arm64 / test (default, 1, 3, macos-m1-12, unstable), trunk / macos-12-py3-arm64 / test (default, 2, 3, macos-m1-12, unstable), trunk / macos-12-py3-arm64 / test (default, 3, 3, macos-m1-12, unstable), trunk / macos-12-py3-arm64-mps / test (mps, 1, 1, macos-m1-12, unstable), trunk / win-vs2019-cpu-py3 / test (default, 1, 3, windows.4xlarge.nonephemeral), trunk / linux-focal-cuda12.1-py3.10-gcc9 / test (nogpu_AVX512, 1, 1, linux.2xlarge), trunk / linux-focal-cuda12.1-py3.10-gcc9 / test (nogpu_NO_AVX2, 1, 1, linux.2xlarge)

Learn more about merging in the wiki.

Questions? Feedback? Please reach out to the PyTorch DevX Team

Advanced Debugging
Check the merge workflow status
here

@wconstab
Copy link
Contributor Author

wconstab commented Nov 3, 2023

@pytorchbot merge -i

@pytorchmergebot
Copy link
Collaborator

Merge started

Your change will be merged while ignoring the following 17 checks: pull / linux-focal-py3.11-clang10 / test (default, 1, 3, linux.2xlarge), pull / linux-focal-py3.11-clang10 / test (crossref, 1, 2, linux.2xlarge), pull / linux-focal-py3.11-clang10 / test (dynamo, 1, 2, linux.2xlarge), pull / linux-focal-py3.8-clang10 / test (default, 1, 3, linux.2xlarge), pull / linux-focal-py3.8-clang10 / test (crossref, 1, 2, linux.2xlarge), pull / linux-focal-py3.8-clang10 / test (dynamo, 1, 2, linux.2xlarge), pull / linux-jammy-py3.8-gcc11 / test (default, 1, 3, linux.2xlarge), pull / linux-jammy-py3.10-clang15-asan / test (default, 1, 6, linux.4xlarge), pull / linux-focal-cuda12.1-py3.10-gcc9 / test (default, 1, 5, linux.4xlarge.nvidia.gpu), pull / linux-focal-cuda12.1-py3.10-gcc9-sm86 / test (default, 1, 5, linux.g5.4xlarge.nvidia.gpu), trunk / macos-12-py3-arm64 / test (default, 1, 3, macos-m1-12, unstable), trunk / macos-12-py3-arm64 / test (default, 2, 3, macos-m1-12, unstable), trunk / macos-12-py3-arm64 / test (default, 3, 3, macos-m1-12, unstable), trunk / macos-12-py3-arm64-mps / test (mps, 1, 1, macos-m1-12, unstable), trunk / win-vs2019-cpu-py3 / test (default, 1, 3, windows.4xlarge.nonephemeral), trunk / linux-focal-cuda12.1-py3.10-gcc9 / test (nogpu_AVX512, 1, 1, linux.2xlarge), trunk / linux-focal-cuda12.1-py3.10-gcc9 / test (nogpu_NO_AVX2, 1, 1, linux.2xlarge)

Learn more about merging in the wiki.

Questions? Feedback? Please reach out to the PyTorch DevX Team

Advanced Debugging
Check the merge workflow status
here

@wconstab
Copy link
Contributor Author

wconstab commented Nov 3, 2023

@pytorchbot merge -f"unrelated failures. and also -i wasn't working"

@pytorchmergebot
Copy link
Collaborator

Merge started

Your change will be merged immediately since you used the force (-f) flag, bypassing any CI checks (ETA: 1-5 minutes). Please use -f as last resort and instead consider -i/--ignore-current to continue the merge ignoring current failures. This will allow currently pending tests to finish and report signal before the merge.

Learn more about merging in the wiki.

Questions? Feedback? Please reach out to the PyTorch DevX Team

Advanced Debugging
Check the merge workflow status
here

@pytorchmergebot
Copy link
Collaborator

Merge failed

Reason: Command git -C /home/runner/work/pytorch/pytorch cherry-pick -x 2e894d758135443c1052328c2e6df8daad7d6335 returned non-zero exit code 1

Auto-merging CODEOWNERS
CONFLICT (content): Merge conflict in CODEOWNERS
error: could not apply 2e894d75813... Remove torch distributed from CODEOWNERS
hint: After resolving the conflicts, mark them with
hint: "git add/rm <pathspec>", then run
hint: "git cherry-pick --continue".
hint: You can instead skip this commit with "git cherry-pick --skip".
hint: To abort and get back to the state before "git cherry-pick",
hint: run "git cherry-pick --abort".
Details for Dev Infra team Raised by workflow job

After adding support for labeler, we don't need CODEOWNERS.

This change will cause the distributed team members previously listed in
CODEOWNERS to stop being auto-added as reviewers on PRs touching these
files.  The preceding PR adds labeler support for these same sets of
files, and contains instructions for adding yourself to be cc'd for that
label.

It is preferable to be auto-cc'd rather than auto-tagged as reviewer, so
that there is more signal in the reviewers list (either someone opted in
which shows the PR author someone is likely looking at it, or the PR
author added someone specifically which is a stronger notification to
the tagged reviewer than the blanket CODEOWNERS behavior.

cc H-Huang awgu kwen2501 wanchaol fegin fduwjj wz337 mrshenli zhaojuanmao rohan-varma kiukchung d4l3k

[ghstack-poisoned]
wconstab added a commit that referenced this pull request Nov 4, 2023
After adding support for labeler, we don't need CODEOWNERS.

This change will cause the distributed team members previously listed in
CODEOWNERS to stop being auto-added as reviewers on PRs touching these
files.  The preceding PR adds labeler support for these same sets of
files, and contains instructions for adding yourself to be cc'd for that
label.

It is preferable to be auto-cc'd rather than auto-tagged as reviewer, so
that there is more signal in the reviewers list (either someone opted in
which shows the PR author someone is likely looking at it, or the PR
author added someone specifically which is a stronger notification to
the tagged reviewer than the blanket CODEOWNERS behavior.

ghstack-source-id: 6c62081
Pull Request resolved: #112813
After adding support for labeler, we don't need CODEOWNERS.

This change will cause the distributed team members previously listed in
CODEOWNERS to stop being auto-added as reviewers on PRs touching these
files.  The preceding PR adds labeler support for these same sets of
files, and contains instructions for adding yourself to be cc'd for that
label.

It is preferable to be auto-cc'd rather than auto-tagged as reviewer, so
that there is more signal in the reviewers list (either someone opted in
which shows the PR author someone is likely looking at it, or the PR
author added someone specifically which is a stronger notification to
the tagged reviewer than the blanket CODEOWNERS behavior.

cc H-Huang awgu kwen2501 wanchaol fegin fduwjj wz337 mrshenli zhaojuanmao rohan-varma kiukchung d4l3k

[ghstack-poisoned]
wconstab added a commit that referenced this pull request Nov 6, 2023
After adding support for labeler, we don't need CODEOWNERS.

This change will cause the distributed team members previously listed in
CODEOWNERS to stop being auto-added as reviewers on PRs touching these
files.  The preceding PR adds labeler support for these same sets of
files, and contains instructions for adding yourself to be cc'd for that
label.

It is preferable to be auto-cc'd rather than auto-tagged as reviewer, so
that there is more signal in the reviewers list (either someone opted in
which shows the PR author someone is likely looking at it, or the PR
author added someone specifically which is a stronger notification to
the tagged reviewer than the blanket CODEOWNERS behavior.

ghstack-source-id: 69f50e3
Pull Request resolved: #112813
@wconstab
Copy link
Contributor Author

wconstab commented Nov 7, 2023

@pytorchbot merge

@pytorchmergebot
Copy link
Collaborator

Merge started

Your change will be merged once all checks pass (ETA 0-4 Hours).

Learn more about merging in the wiki.

Questions? Feedback? Please reach out to the PyTorch DevX Team

Advanced Debugging
Check the merge workflow status
here

xuhancn pushed a commit to xuhancn/pytorch that referenced this pull request Nov 7, 2023
After adding support for labeler, we don't need CODEOWNERS.

This change will cause the distributed team members previously listed in
CODEOWNERS to stop being auto-added as reviewers on PRs touching these
files.  The preceding PR adds labeler support for these same sets of
files, and contains instructions for adding yourself to be cc'd for that
label.

It is preferable to be auto-cc'd rather than auto-tagged as reviewer, so
that there is more signal in the reviewers list (either someone opted in
which shows the PR author someone is likely looking at it, or the PR
author added someone specifically which is a stronger notification to
the tagged reviewer than the blanket CODEOWNERS behavior.

Pull Request resolved: pytorch#112813
Approved by: https://github.com/wanchaol, https://github.com/fduwjj
@facebook-github-bot facebook-github-bot deleted the gh/wconstab/212/head branch November 10, 2023 15:24
Skylion007 pushed a commit to Skylion007/pytorch that referenced this pull request Nov 14, 2023
After adding support for labeler, we don't need CODEOWNERS.

This change will cause the distributed team members previously listed in
CODEOWNERS to stop being auto-added as reviewers on PRs touching these
files.  The preceding PR adds labeler support for these same sets of
files, and contains instructions for adding yourself to be cc'd for that
label.

It is preferable to be auto-cc'd rather than auto-tagged as reviewer, so
that there is more signal in the reviewers list (either someone opted in
which shows the PR author someone is likely looking at it, or the PR
author added someone specifically which is a stronger notification to
the tagged reviewer than the blanket CODEOWNERS behavior.

Pull Request resolved: pytorch#112813
Approved by: https://github.com/wanchaol, https://github.com/fduwjj
@albanD albanD added oncall: distributed Add this issue/PR to distributed oncall triage queue and removed module: distributed labels Dec 8, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

ciflow/trunk Trigger trunk jobs on your pull request Merged oncall: distributed Add this issue/PR to distributed oncall triage queue topic: not user facing topic category

Projects

None yet

Development

Successfully merging this pull request may close these issues.

6 participants