Make init_process_group timeout kwarg override pg_options #112611

wconstab · 2023-11-01T16:56:56Z

Stack from ghstack (oldest at bottom):

This used to be ambiguous but the pg_options._timeout value, if passed
in, is being ignored. Make it sane and warn if 2 values are provided.

[ghstack-poisoned]

pytorch-bot · 2023-11-01T16:57:01Z

🔗 Helpful Links

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

📄 Preview Python docs built from this PR
📄 Preview C++ docs built from this PR
❓ Need help or want to give feedback on the CI? Visit the bot commands wiki or our office hours

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

✅ You can merge normally! (3 Unrelated Failures)

As of commit e52c613 with merge base d084a02 ():

UNSTABLE - The following jobs failed but were likely due to flakiness present on trunk and has been marked as unstable:

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

ghstack-source-id: 5ab421c Pull Request resolved: #112611

voznesenskym · 2023-11-01T17:01:47Z

torch/distributed/distributed_c10d.py

+                if pg_options._timeout:
+                    raise ValueError(
+                        "pg_options._timeout was specified, "
+                        "but timeout kwarg has a default value that will always override it. "
+                    )


Why does this option only matter under NCCL? how does this interact w/ base_pg_options._timeout?

well, gloo/mpi paths actually use this other object called 'base_pg_options' which has its OWN timeout field. That's a whole 'nother problem, which i kinda want to try fixing too.

it looks like if someone passes in a 'pg_options' object, it only gets used for nccl. and if you use the gloo/mpi backends, the 'base_pg_options' thing gets used. pretty sketchy.

note: if we do fix this, we might be blocked on the fact that ProcessGroupNCCL::Options is bound to python differently than ProcessGroup::Options. that is also something to fix.

One reason for the weirdness is that it always used to be init_pg -> creates 1 backend. Now init_pg -> can create multiple backends. However, pg_options in init_pg is only for NCCL and no other backends use it.

Hence all the other backends are using base_pg_options._timeout and this is not exposed to the user. We are just using the options as a property bag for the timeout value and backend name.

fduwjj · 2023-11-01T22:34:24Z

torch/distributed/distributed_c10d.py

                    pg_options, ProcessGroupNCCL.Options
                ), "Expected pg_options argument to be of type ProcessGroupNCCL.Options"
+                if pg_options._timeout:
+                    raise ValueError(


do you want to just throw a warning instead of Error?

yea, good point. If this discrepancy hasn't been bothering people, then they can take their time to fix their usage. No point introducing a bunch of breakages for people.

I dont think many people are using timeout in pg_options, so warning sounds good. If we really wanted to we could just save the timeout and then set it again after if it has a value.

H-Huang

Okay with throwing a warning as Junjie proposed. Stamping to unblock

H-Huang · 2023-11-02T13:19:59Z

torch/distributed/distributed_c10d.py

                    pg_options, ProcessGroupNCCL.Options
                ), "Expected pg_options argument to be of type ProcessGroupNCCL.Options"
+                if pg_options._timeout:
+                    raise ValueError(


I dont think many people are using timeout in pg_options, so warning sounds good. If we really wanted to we could just save the timeout and then set it again after if it has a value.

H-Huang · 2023-11-02T13:33:49Z

torch/distributed/distributed_c10d.py

+                if pg_options._timeout:
+                    raise ValueError(
+                        "pg_options._timeout was specified, "
+                        "but timeout kwarg has a default value that will always override it. "
+                    )


One reason for the weirdness is that it always used to be init_pg -> creates 1 backend. Now init_pg -> can create multiple backends. However, pg_options in init_pg is only for NCCL and no other backends use it.

Hence all the other backends are using base_pg_options._timeout and this is not exposed to the user. We are just using the options as a property bag for the timeout value and backend name.

[ghstack-poisoned]

This used to be ambiguous but the pg_options._timeout value, if passed in, is being ignored. Make it sane and warn if 2 values are provided. [ghstack-poisoned]

H-Huang · 2023-11-02T16:27:25Z

torch/distributed/distributed_c10d.py

                assert isinstance(
                    pg_options, ProcessGroupNCCL.Options
                ), "Expected pg_options argument to be of type ProcessGroupNCCL.Options"
+                if pg_options._timeout:


do we need to check if pg_options._timeout != default timeout instead?

This used to be ambiguous but the pg_options._timeout value, if passed in, is being ignored. Make it sane and warn if 2 values are provided. [ghstack-poisoned]

This used to be ambiguous but the pg_options._timeout value, if passed in, is being ignored. Make it sane and warn if 2 values are provided. ghstack-source-id: 0a5abbb Pull Request resolved: #112611

wconstab · 2023-11-02T22:33:02Z

@pytorchbot merge

pytorchmergebot · 2023-11-02T22:34:44Z

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

pytorchmergebot · 2023-11-02T23:11:31Z

Merge failed

Reason: 11 jobs have failed, first few of them are: pull / linux-focal-py3.8-clang10 / test (default, 2, 3, linux.2xlarge), pull / linux-focal-py3.8-clang10 / test (crossref, 2, 2, linux.2xlarge), pull / linux-focal-py3.8-clang10 / test (dynamo, 2, 2, linux.2xlarge), pull / linux-focal-py3.11-clang10 / test (default, 2, 3, linux.2xlarge), pull / linux-focal-py3.11-clang10 / test (crossref, 1, 2, linux.2xlarge)

Details for Dev Infra team

Raised by workflow job

wconstab · 2023-11-03T03:15:15Z

@pytorchbot merge

pytorchmergebot · 2023-11-03T03:17:18Z

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

pytorchmergebot · 2023-11-03T03:17:30Z

Merge failed

Reason: 14 jobs have failed, first few of them are: pull / linux-focal-py3.8-clang10 / test (default, 2, 3, linux.2xlarge), pull / linux-focal-py3.8-clang10 / test (crossref, 2, 2, linux.2xlarge), pull / linux-focal-py3.8-clang10 / test (dynamo, 2, 2, linux.2xlarge), pull / linux-focal-py3.11-clang10 / test (default, 2, 3, linux.2xlarge), pull / linux-focal-py3.11-clang10 / test (crossref, 1, 2, linux.2xlarge)

Details for Dev Infra team

Raised by workflow job

This used to be ambiguous but the pg_options._timeout value, if passed in, is being ignored. Make it sane and warn if 2 values are provided. [ghstack-poisoned]

wconstab · 2023-11-03T23:10:43Z

@pytorchbot merge

pytorchmergebot · 2023-11-03T23:12:39Z

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

Pull Request resolved: #112803 Approved by: https://github.com/H-Huang ghstack dependencies: #112611

…#112893) Previous PRs changed the c++ default timeout for PGNccl, but this path was only hit in some cases, and the python defaults took over in other cases. This PR ensures that NCCL pg always default to the changed NCCL-specific timeout value. Pull Request resolved: #112893 Approved by: https://github.com/xw285cornell, https://github.com/kwen2501, https://github.com/XilunWu ghstack dependencies: #112611, #112803

…2611) This used to be ambiguous but the pg_options._timeout value, if passed in, is being ignored. Make it sane and warn if 2 values are provided. Pull Request resolved: pytorch#112611 Approved by: https://github.com/H-Huang

Pull Request resolved: pytorch#112803 Approved by: https://github.com/H-Huang ghstack dependencies: pytorch#112611

…pytorch#112893) Previous PRs changed the c++ default timeout for PGNccl, but this path was only hit in some cases, and the python defaults took over in other cases. This PR ensures that NCCL pg always default to the changed NCCL-specific timeout value. Pull Request resolved: pytorch#112893 Approved by: https://github.com/xw285cornell, https://github.com/kwen2501, https://github.com/XilunWu ghstack dependencies: pytorch#112611, pytorch#112803

…2611) This used to be ambiguous but the pg_options._timeout value, if passed in, is being ignored. Make it sane and warn if 2 values are provided. Pull Request resolved: pytorch#112611 Approved by: https://github.com/H-Huang

Pull Request resolved: pytorch#112803 Approved by: https://github.com/H-Huang ghstack dependencies: pytorch#112611

…pytorch#112893) Previous PRs changed the c++ default timeout for PGNccl, but this path was only hit in some cases, and the python defaults took over in other cases. This PR ensures that NCCL pg always default to the changed NCCL-specific timeout value. Pull Request resolved: pytorch#112893 Approved by: https://github.com/xw285cornell, https://github.com/kwen2501, https://github.com/XilunWu ghstack dependencies: pytorch#112611, pytorch#112803

WIP make timeout kwarg work consistently over pg_options

53d9d3f

[ghstack-poisoned]

wconstab requested review from H-Huang, awgu, d4l3k, fduwjj, fegin, kiukchung, kwen2501, mrshenli, rohan-varma, wanchaol, wz337 and zhaojuanmao as code owners November 1, 2023 16:56

wconstab mentioned this pull request Nov 1, 2023

Remove default timeout from PGNCCL::Options ctor #112555

Closed

pytorch-bot bot added the release notes: distributed (c10d) release notes category label Nov 1, 2023

wconstab added a commit that referenced this pull request Nov 1, 2023

WIP make timeout kwarg work consistently over pg_options

fe693d5

ghstack-source-id: 5ab421c Pull Request resolved: #112611

voznesenskym reviewed Nov 1, 2023

View reviewed changes

fduwjj reviewed Nov 1, 2023

View reviewed changes

H-Huang approved these changes Nov 2, 2023

View reviewed changes

Update on "WIP make timeout kwarg work consistently over pg_options"

6f78f99

[ghstack-poisoned]

Update on "Make init_process_group timeout kwarg override pg_options"

dd1172e

This used to be ambiguous but the pg_options._timeout value, if passed in, is being ignored. Make it sane and warn if 2 values are provided. [ghstack-poisoned]

wconstab changed the title ~~WIP make timeout kwarg work consistently over pg_options~~ Make init_process_group timeout kwarg override pg_options Nov 2, 2023

H-Huang reviewed Nov 2, 2023

View reviewed changes

Update on "Make init_process_group timeout kwarg override pg_options"

27851da

This used to be ambiguous but the pg_options._timeout value, if passed in, is being ignored. Make it sane and warn if 2 values are provided. [ghstack-poisoned]

wconstab mentioned this pull request Nov 2, 2023

Add test for init_process_group timeout #112803

Closed

pytorch-bot bot added the ciflow/trunk Trigger trunk jobs on your pull request label Nov 2, 2023

pytorchmergebot added the merging label Nov 2, 2023

pytorchmergebot removed the merging label Nov 2, 2023

pytorchmergebot added the merging label Nov 3, 2023

pytorchmergebot removed the merging label Nov 3, 2023

Update on "Make init_process_group timeout kwarg override pg_options"

e52c613

This used to be ambiguous but the pg_options._timeout value, if passed in, is being ignored. Make it sane and warn if 2 values are provided. [ghstack-poisoned]

wconstab mentioned this pull request Nov 3, 2023

Fix default timeouts for python entrypoints (e.g. init_process_group) #112893

Closed

wconstab requested a review from LucasLLC as a code owner November 3, 2023 19:32

pytorchmergebot added the merging label Nov 3, 2023

pytorchmergebot added the Merged label Nov 3, 2023

pytorchmergebot removed the merging label Nov 3, 2023

pytorchmergebot closed this in 65b74c9 Nov 3, 2023

pytorchmergebot pushed a commit that referenced this pull request Nov 4, 2023

Add test for init_process_group timeout (#112803)

854882b

Pull Request resolved: #112803 Approved by: https://github.com/H-Huang ghstack dependencies: #112611

wconstab mentioned this pull request Nov 6, 2023

Remove duplicated sentence from docs #113044

Closed

xuhancn pushed a commit to xuhancn/pytorch that referenced this pull request Nov 7, 2023

Add test for init_process_group timeout (pytorch#112803)

c17559c

Pull Request resolved: pytorch#112803 Approved by: https://github.com/H-Huang ghstack dependencies: pytorch#112611

facebook-github-bot deleted the gh/wconstab/209/head branch November 7, 2023 15:24

Skylion007 pushed a commit to Skylion007/pytorch that referenced this pull request Nov 14, 2023

Add test for init_process_group timeout (pytorch#112803)

f9b52dd

Pull Request resolved: pytorch#112803 Approved by: https://github.com/H-Huang ghstack dependencies: pytorch#112611

Make init_process_group timeout kwarg override pg_options #112611

Make init_process_group timeout kwarg override pg_options #112611

Uh oh!

Conversation

wconstab commented Nov 1, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

pytorch-bot bot commented Nov 1, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

🔗 Helpful Links

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

✅ You can merge normally! (3 Unrelated Failures)

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

fduwjj Nov 1, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

H-Huang left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

H-Huang Nov 2, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

wconstab commented Nov 2, 2023

Uh oh!

pytorchmergebot commented Nov 2, 2023

Merge started

Uh oh!

pytorchmergebot commented Nov 2, 2023

Merge failed

Uh oh!

wconstab commented Nov 3, 2023

Uh oh!

pytorchmergebot commented Nov 3, 2023

Merge started

Uh oh!

pytorchmergebot commented Nov 3, 2023

Merge failed

Uh oh!

wconstab commented Nov 3, 2023

Uh oh!

pytorchmergebot commented Nov 3, 2023

Merge started

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

wconstab commented Nov 1, 2023 •

edited

Loading

pytorch-bot bot commented Nov 1, 2023 •

edited

Loading

fduwjj Nov 1, 2023 •

edited

Loading

H-Huang Nov 2, 2023 •

edited

Loading