Add warning on ProcessGroup and ProcessGroup::Work APIs #46220

agolynski · 2020-10-12T22:15:27Z

Stack from ghstack:

Add warning on ProcessGroup and ProcessGroup::Work APIs #46220 Add warning on ProcessGroup and ProcessGroup::Work APIs

Differential Revision: D24294437

[ghstack-poisoned]

dr-ci · 2020-10-12T22:26:28Z

💊 CI failures summary and remediations

As of commit 1a36843 (more details on the Dr. CI page):

1/1 failures possibly* introduced in this PR
- 1/1 non-CircleCI failure(s)

codecov.io: 1 failed

Failed: codecov/patch

This comment was automatically generated by Dr. CI (expand for details).

Follow this link to opt-out of these comments for your Pull Requests.

Please report bugs/suggestions on the GitHub issue tracker or post in the (internal) Dr. CI Users group.

See how this bot performed.

This comment has been revised 32 times.

[ghstack-poisoned]

ghstack-source-id: 70fae3b9745be76053f12a847873e8f6a9e1900b Pull Request resolved: #46220

mrshenli

Hey @agolynski, I might not convey this clearly. The goal is that, whenever users call certain APIs on Work, we print the warning message. We will need to add things like TORCH_WARN_ONCE to impacted APIs. The impacted ones are those that won't be available in the new Future API. At least including the following ones.

pytorch/torch/csrc/distributed/c10d/init.cpp

Lines 979 to 981 in b98e359

    
           .def("is_success", &::c10d::ProcessGroup::Work::isSuccess) 
        
           .def("exception", &::c10d::ProcessGroup::Work::exception) 
        
           .def("source_rank", &::c10d::ProcessGroup::Work::sourceRank)

pytorch/torch/csrc/distributed/c10d/init.cpp

Line 987 in b98e359

.def("synchronize", &::c10d::ProcessGroup::Work::synchronize)

@wanchaol @gmagogsfm please comment if there are more impacted APIs.

codecov · 2020-10-13T02:39:59Z

Codecov Report

Merging #46220 into gh/agolynski/10/base will not change coverage.
The diff coverage is n/a.

@@                  Coverage Diff                  @@
##           gh/agolynski/10/base   #46220   +/-   ##
=====================================================
  Coverage                 68.33%   68.33%           
=====================================================
  Files                       410      410           
  Lines                     53795    53795           
=====================================================
  Hits                      36760    36760           
  Misses                    17035    17035

Impacted Files	Coverage Δ
torch/distributed/distributed_c10d.py	`27.29% <ø> (ø)`

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update f2e5ae4...1a36843. Read the comment docs.

mrshenli · 2020-10-13T20:24:08Z

@agolynski

Raised the concern that APIs like source_rank is also used by other features like torch.distributed.recv, so adding a warning message will impact those as well. To avoid this, can we do sth like (this can also apply to other APIs):

provide both source_rank and _source_rank APIs.
change our code to use _source_rank
let source_rank print warning

[ghstack-poisoned]

wanchaol

looks good to me, minor comments inline. Thanks for doing this!

wanchaol · 2020-10-13T23:18:25Z

torch/csrc/distributed/c10d/init.cpp

@@ -974,15 +974,48 @@ that adds a prefix to each key inserted to the store.

  shared_ptr_class_<::c10d::ProcessGroup::Work>(module, "Work")
      .def("is_completed", &::c10d::ProcessGroup::Work::isCompleted)


shall we deprecate this as well? This is called done in future, see

pytorch/torch/csrc/jit/python/init.cpp

Line 1094 in a3caa71

"done",

This is risky, as we did expose this API in our API doc. If you wanna change this, please mention it where it is defined in https://github.com/pytorch/pytorch/blob/master/docs/source/distributed.rst as well.

wanchaol · 2020-10-13T23:19:39Z

torch/lib/c10d/ProcessGroup.hpp

@@ -157,6 +160,12 @@ class ProcessGroup {
    return size_;
  }

+  // *************************************************************************


shall we add this comment in the beginning of ProcessGroup class instead?

[ghstack-poisoned]

agolynski · 2020-10-14T00:19:08Z

Hey @agolynski, I might not convey this clearly. The goal is that, whenever users call certain APIs on Work, we print the warning message. We will need to add things like TORCH_WARN_ONCE to impacted APIs. The impacted ones are those that won't be available in the new Future API. At least including the following ones.

pytorch/torch/csrc/distributed/c10d/init.cpp

Lines 979 to 981 in b98e359

.def("is_success", &::c10d::ProcessGroup::Work::isSuccess)

.def("exception", &::c10d::ProcessGroup::Work::exception)

.def("source_rank", &::c10d::ProcessGroup::Work::sourceRank)

pytorch/torch/csrc/distributed/c10d/init.cpp

Line 987 in b98e359

.def("synchronize", &::c10d::ProcessGroup::Work::synchronize)

@wanchaol @gmagogsfm please comment if there are more impacted APIs.

done

agolynski · 2020-10-14T00:19:33Z

@agolynski

Raised the concern that APIs like source_rank is also used by other features like torch.distributed.recv, so adding a warning message will impact those as well. To avoid this, can we do sth like (this can also apply to other APIs):

provide both source_rank and _source_rank APIs.

change our code to use _source_rank

let source_rank print warning

done

mrshenli

Stamp to unblock, but please fix the error I mentioned inline.

mrshenli · 2020-10-14T00:50:23Z

torch/csrc/distributed/c10d/init.cpp

+                "deprecated, please ping "
+                "https://github.com/pytorch/pytorch/issues/46291 "
+                "if you see this warning");
+            return work.isSuccess();


shouldn't this be isCompleted?

mrshenli · 2020-10-14T01:08:34Z

torch/csrc/distributed/c10d/init.cpp

+            TORCH_WARN_ONCE("ProcessGroup::Work::is_completed API is being "
+                "deprecated, please ping "
+                "https://github.com/pytorch/pytorch/issues/46291 "
+                "if you see this warning");


Is this what clang-format gives you? The devserver clang-format gives me this:

TORCH_WARN_ONCE( "ProcessGroup::Work::is_completed API is being " "deprecated, please ping " "https://github.com/pytorch/pytorch/issues/46291 " "if you see this warning");

And ditto for other APIs as well.

reformatted via vscode clang-format

malfet · 2020-10-14T01:14:22Z

torch/csrc/distributed/c10d/init.cpp

+      .def(
+          "is_completed",
+          [](::c10d::ProcessGroup::Work& work) -> bool {
+            TORCH_WARN_ONCE("ProcessGroup::Work::is_completed API is being "
+                "deprecated, please ping "
+                "https://github.com/pytorch/pytorch/issues/46291 "
+                "if you see this warning");
+            return work.isSuccess();
+          })
+      .def(
+          "is_success",
+          [](::c10d::ProcessGroup::Work& work) -> bool {
+            TORCH_WARN_ONCE("ProcessGroup::Work::is_success API is being "
+                "deprecated, please ping "
+                "https://github.com/pytorch/pytorch/issues/46291 "
+                "if you see this warning");
+            return work.isSuccess();
+          })
+      .def(
+          "exception",
+          [](::c10d::ProcessGroup::Work& work) -> std::exception_ptr {
+            TORCH_WARN_ONCE("ProcessGroup::Work::exception API is being "
+                "deprecated, please ping "
+                "https://github.com/pytorch/pytorch/issues/46291 "
+                "if you see this warning");
+            return work.exception();
+          })
+      .def(
+          "source_rank",
+          [](::c10d::ProcessGroup::Work& work) -> int {
+            TORCH_WARN_ONCE("ProcessGroup::Work::source_rank API is being "
+                "deprecated, please ping "
+                "https://github.com/pytorch/pytorch/issues/46291 "
+                "if you see this warning");
+            return work.sourceRank();


I don't like preprocessor macro, but it seem to be ideal to avoid string literal duplication, i.e. use something like

#define _DEPRECATED_DEF(name, method) \ def( name, [](::c10d::ProcessGroup::Work& work) -> int { \ TORCH_WARN_ONCE("ProcessGroup::Work::" name " API is being " \ "deprecated, please ping " \ "https://github.com/pytorch/pytorch/issues/46291 " \ "if you see this warning"); \ return work.method(); \ })

Suggested change

.def(

"is_completed",

[](::c10d::ProcessGroup::Work& work) -> bool {

TORCH_WARN_ONCE("ProcessGroup::Work::is_completed API is being "

"deprecated, please ping "

"https://github.com/pytorch/pytorch/issues/46291 "

"if you see this warning");

return work.isSuccess();

})

.def(

"is_success",

[](::c10d::ProcessGroup::Work& work) -> bool {

TORCH_WARN_ONCE("ProcessGroup::Work::is_success API is being "

"deprecated, please ping "

"https://github.com/pytorch/pytorch/issues/46291 "

"if you see this warning");

return work.isSuccess();

})

.def(

"exception",

[](::c10d::ProcessGroup::Work& work) -> std::exception_ptr {

TORCH_WARN_ONCE("ProcessGroup::Work::exception API is being "

"deprecated, please ping "

"https://github.com/pytorch/pytorch/issues/46291 "

"if you see this warning");

return work.exception();

})

.def(

"source_rank",

[](::c10d::ProcessGroup::Work& work) -> int {

TORCH_WARN_ONCE("ProcessGroup::Work::source_rank API is being "

"deprecated, please ping "

"https://github.com/pytorch/pytorch/issues/46291 "

"if you see this warning");

return work.sourceRank();

._DEPRECATED_DEF("is_completed", isCompleted)

._DEPRECATED_DEF("is_success", isSuccess)

._DEPRECATED_DEF("exception", exception)

._DEPRECATED_DEF("source_rank", sourceRank)

Another option is constexpr + fmt::format

added a macro

... and switched to fmt::format since clang-tidy doesn't like macros

Differential Revision: [D24294437](https://our.internmc.facebook.com/intern/diff/D24294437) [ghstack-poisoned]

torch/csrc/distributed/c10d/init.cpp

Differential Revision: [D24294437](https://our.internmc.facebook.com/intern/diff/D24294437) [ghstack-poisoned]

mrshenli · 2020-10-14T01:54:55Z

torch/csrc/distributed/c10d/init.cpp

@@ -972,17 +972,44 @@ that adds a prefix to each key inserted to the store.
    py::call_guard<py::gil_scoped_release>());
 #endif

+#define PROCESS_GROUP_DEPRECATION_WARNING(api_method)                \
+  TORCH_WARN_ONCE(#api_method                                        \
+                  "API is being deprecated, please ping "            \


Curious, do you need a space before "API"?

Differential Revision: [D24294437](https://our.internmc.facebook.com/intern/diff/D24294437) [ghstack-poisoned]

ghstack-source-id: 278069176ca85555d2d3bc458ca399bee5721a54 Pull Request resolved: #46220 Add warnings on deprecated ProcesssGroup::Work functionality ghstack-source-id: 278069176ca85555d2d3bc458ca399bee5721a54 Pull Request resolved: #46294 fix formatting ghstack-source-id: 278069176ca85555d2d3bc458ca399bee5721a54 Pull Request resolved: #46295

Differential Revision: [D24294437](https://our.internmc.facebook.com/intern/diff/D24294437) [ghstack-poisoned]

ghstack-source-id: ea13ae5b2b37f929408cff5ffb18995649342ef0 Pull Request resolved: #46220 Add warnings on deprecated ProcesssGroup::Work functionality ghstack-source-id: ea13ae5b2b37f929408cff5ffb18995649342ef0 Pull Request resolved: #46294 fix formatting ghstack-source-id: ea13ae5b2b37f929408cff5ffb18995649342ef0 Pull Request resolved: #46295

agolynski · 2020-10-14T13:24:24Z

Clang-tidy is broken now, fix is on the way:
#46315

mrshenli · 2020-10-14T14:24:59Z

Clang-tidy is broken now, fix is on the way: #46315

Isn't the clang-tidy failure caused by this PR?

[
  {
    path: 'torch/csrc/distributed/c10d/init.cpp',
    start_line: 975,
    end_line: 975,
    start_column: 9,
    end_column: 9,
    annotation_level: 'failure',
    message: "[cppcoreguidelines-macro-usage] warning: function-like macro 'PROCESS_GROUP_DEPRECATION_WARNING' used; consider a 'constexpr' template function"
  }
]

agolynski · 2020-10-14T14:37:14Z

Clang-tidy is broken now, fix is on the way: #46315

Isn't the clang-tidy failure caused by this PR?

[
  {
    path: 'torch/csrc/distributed/c10d/init.cpp',
    start_line: 975,
    end_line: 975,
    start_column: 9,
    end_column: 9,
    annotation_level: 'failure',
    message: "[cppcoreguidelines-macro-usage] warning: function-like macro 'PROCESS_GROUP_DEPRECATION_WARNING' used; consider a 'constexpr' template function"
  }
]

it should be fixed now (before tidy run)

Differential Revision: [D24294437](https://our.internmc.facebook.com/intern/diff/D24294437) [ghstack-poisoned]

ghstack-source-id: f5427d315d18dc2585d68a394f36409602bbc505 Pull Request resolved: #46220

facebook-github-bot · 2020-10-15T00:18:54Z

@gmagogsfm merged this pull request in e7e919f.

ghstack-source-id: f5427d315d18dc2585d68a394f36409602bbc505 Pull Request resolved: #46220

warning

d3814c5

[ghstack-poisoned]

agolynski requested review from mingzhe09088, mrshenli, pietern, pritamdamania87, rohan-varma and zhaojuanmao as code owners October 12, 2020 22:15

facebook-github-bot added the oncall: distributed Add this issue/PR to distributed oncall triage queue label Oct 12, 2020

agolynski changed the title ~~warning~~ Add warning on ProcessGroup and ProcessGroup::Work APIs Oct 12, 2020

Update on "Add warning on ProcessGroup and ProcessGroup::Work APIs"

24e1fe0

[ghstack-poisoned]

agolynski added a commit that referenced this pull request Oct 12, 2020

Add warning on ProcessGroup and ProcessGroup::Work APIs

6e91ead

ghstack-source-id: 70fae3b9745be76053f12a847873e8f6a9e1900b Pull Request resolved: #46220

mrshenli requested changes Oct 12, 2020

View reviewed changes

This was referenced Oct 13, 2020

Add warnings on deprecated ProcesssGroup::Work functionality #46294

Closed

fix formatting #46295

Closed

Update on "Add warning on ProcessGroup and ProcessGroup::Work APIs"

4c8bbf8

[ghstack-poisoned]

agolynski requested a review from apaszke as a code owner October 13, 2020 23:05

wanchaol approved these changes Oct 13, 2020

View reviewed changes

Update on "Add warning on ProcessGroup and ProcessGroup::Work APIs"

2a3ed36

[ghstack-poisoned]

Update on "Add warning on ProcessGroup and ProcessGroup::Work APIs"

63a740f

[ghstack-poisoned]

mrshenli approved these changes Oct 14, 2020

View reviewed changes

mrshenli reviewed Oct 14, 2020

View reviewed changes

malfet reviewed Oct 14, 2020

View reviewed changes

Update on "Add warning on ProcessGroup and ProcessGroup::Work APIs"

e670b1b

Differential Revision: [D24294437](https://our.internmc.facebook.com/intern/diff/D24294437) [ghstack-poisoned]

mrshenli reviewed Oct 14, 2020

View reviewed changes

torch/csrc/distributed/c10d/init.cpp Outdated Show resolved Hide resolved

Update on "Add warning on ProcessGroup and ProcessGroup::Work APIs"

34b2a76

Differential Revision: [D24294437](https://our.internmc.facebook.com/intern/diff/D24294437) [ghstack-poisoned]

mrshenli reviewed Oct 14, 2020

View reviewed changes

Update on "Add warning on ProcessGroup and ProcessGroup::Work APIs"

506d8cf

Differential Revision: [D24294437](https://our.internmc.facebook.com/intern/diff/D24294437) [ghstack-poisoned]

Update on "Add warning on ProcessGroup and ProcessGroup::Work APIs"

0901732

Differential Revision: [D24294437](https://our.internmc.facebook.com/intern/diff/D24294437) [ghstack-poisoned]

Update on "Add warning on ProcessGroup and ProcessGroup::Work APIs"

846135c

Differential Revision: [D24294437](https://our.internmc.facebook.com/intern/diff/D24294437) [ghstack-poisoned]

Update on "Add warning on ProcessGroup and ProcessGroup::Work APIs"

6f507ea

Differential Revision: [D24294437](https://our.internmc.facebook.com/intern/diff/D24294437) [ghstack-poisoned]

Update on "Add warning on ProcessGroup and ProcessGroup::Work APIs"

1a36843

Differential Revision: [D24294437](https://our.internmc.facebook.com/intern/diff/D24294437) [ghstack-poisoned]

agolynski added a commit that referenced this pull request Oct 14, 2020

Add warning on ProcessGroup and ProcessGroup::Work APIs

604601b

ghstack-source-id: f5427d315d18dc2585d68a394f36409602bbc505 Pull Request resolved: #46220

facebook-github-bot closed this in e7e919f Oct 14, 2020

agolynski added a commit that referenced this pull request Oct 14, 2020

Add warning on ProcessGroup and ProcessGroup::Work APIs

0fdbabc

ghstack-source-id: f5427d315d18dc2585d68a394f36409602bbc505 Pull Request resolved: #46220

This was referenced Oct 14, 2020

[v1.7.0] Release Tracker #45592

Closed

[v1.7 patch] Add warning on ProcessGroup and ProcessGroup::Work APIs #46366

Merged

facebook-github-bot added the Merged label Oct 15, 2020

malfet pushed a commit that referenced this pull request Oct 15, 2020

Add warning on ProcessGroup and ProcessGroup::Work APIs (#46366)

286647b

ghstack-source-id: f5427d315d18dc2585d68a394f36409602bbc505 Pull Request resolved: #46220

facebook-github-bot deleted the gh/agolynski/10/head branch October 18, 2020 14:18

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add warning on ProcessGroup and ProcessGroup::Work APIs #46220

Add warning on ProcessGroup and ProcessGroup::Work APIs #46220

agolynski commented Oct 12, 2020 •

edited

dr-ci bot commented Oct 12, 2020 •

edited

mrshenli left a comment

codecov bot commented Oct 13, 2020 •

edited

mrshenli commented Oct 13, 2020

wanchaol left a comment

wanchaol Oct 13, 2020

agolynski Oct 14, 2020

mrshenli Oct 14, 2020

agolynski Oct 14, 2020

wanchaol Oct 13, 2020

agolynski Oct 13, 2020

agolynski commented Oct 14, 2020

agolynski commented Oct 14, 2020

mrshenli left a comment

mrshenli Oct 14, 2020

agolynski Oct 14, 2020

mrshenli Oct 14, 2020

mrshenli Oct 14, 2020

agolynski Oct 14, 2020

malfet Oct 14, 2020

mrshenli Oct 14, 2020

agolynski Oct 14, 2020

agolynski Oct 14, 2020

mrshenli Oct 14, 2020

agolynski Oct 14, 2020

agolynski commented Oct 14, 2020

mrshenli commented Oct 14, 2020

agolynski commented Oct 14, 2020

facebook-github-bot commented Oct 15, 2020

	.def("is_success", &::c10d::ProcessGroup::Work::isSuccess)
	.def("exception", &::c10d::ProcessGroup::Work::exception)
	.def("source_rank", &::c10d::ProcessGroup::Work::sourceRank)

		@@ -974,15 +974,48 @@ that adds a prefix to each key inserted to the store.

		shared_ptr_class_<::c10d::ProcessGroup::Work>(module, "Work")
		.def("is_completed", &::c10d::ProcessGroup::Work::isCompleted)

Add warning on ProcessGroup and ProcessGroup::Work APIs #46220

Add warning on ProcessGroup and ProcessGroup::Work APIs #46220

Conversation

agolynski commented Oct 12, 2020 • edited

dr-ci bot commented Oct 12, 2020 • edited

💊 CI failures summary and remediations

codecov.io: 1 failed

mrshenli left a comment

Choose a reason for hiding this comment

codecov bot commented Oct 13, 2020 • edited

Codecov Report

mrshenli commented Oct 13, 2020

wanchaol left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

agolynski commented Oct 14, 2020

agolynski commented Oct 14, 2020

mrshenli left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

agolynski commented Oct 14, 2020

mrshenli commented Oct 14, 2020

agolynski commented Oct 14, 2020

facebook-github-bot commented Oct 15, 2020

agolynski commented Oct 12, 2020 •

edited

dr-ci bot commented Oct 12, 2020 •

edited

codecov bot commented Oct 13, 2020 •

edited