distributed debug handlers #126601

d4l3k · 2024-05-18T00:55:00Z

This adds debug handlers as described in:

https://gist.github.com/d4l3k/828b7be585c7615e85b2c448b308d925 (public copy)
https://docs.google.com/document/d/1la68szcS6wUYElUUX-P6zXgkPA8lnfzpagMTPys3aQ8/edit (internal copy)

This is only adding the C++ pieces that will be used from the main process. The Python and torchrun pieces will be added in a follow up PR.

This adds 2 handlers out of the box:

/handler/ping for testing purposes
/handler/dump_nccl_trace_pickle as a POC integration with Flight Recorder

Test plan:

python test/distributed/elastic/test_control_plane.py

cc @mrshenli @pritamdamania87 @zhaojuanmao @satgera @gqchen @aazzolini @osalpekar @jiayisuse @H-Huang @kwen2501 @awgu @penguinwu @fegin @XilunWu @wanchaol @fduwjj @wz337 @tianyu-l @wconstab @yf225 @chauhang

pytorch-bot · 2024-05-18T00:55:03Z

🔗 Helpful Links

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

📄 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.

❗ 1 Active SEVs

There are 1 currently active SEVs. If your PR is affected, please view them below:

Upgrade MacOS runner to 14

✅ You can merge normally! (1 Unrelated Failure)

As of commit 469b55d with merge base 38a33c3 ():

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

pull / linux-focal-cuda12.4-py3.10-gcc9-sm86 / test (default, 5, 5, linux.g5.4xlarge.nvidia.gpu, unstable) (gh) ()
inductor/test_efficient_conv_bn_eval.py::EfficientConvBNEvalCudaTests::test_basic_cuda

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

test/distributed/elastic/test_control_plane.py

kurman · 2024-05-21T18:35:59Z

test/distributed/elastic/test_control_plane.py

can we have a more descriptive name, given that is from another module?

I was thinking about following the hydra pattern here: https://hydra.cc/docs/intro/

In practice this should be something like @worker_server.main.

kurman · 2024-05-21T18:39:13Z

test/distributed/elastic/test_control_plane.py

UX:

Do we need to register the handler first?

If not for some cases, how would users know what is enabled by default?

I wonder if we should adopt some RESTful API approach?

handlers do need to be registered before the post request -- we're currently statically registering all handlers so they'll always be present by this call

RESTful is a pretty overloaded term could you clarify what you mean? I'm not really sure how to express "dump_nccl_trace_pickle" in the rest model since it doesn't really map to a data object with CRUD

i guess naively, GET "/worker/handler/nccl_trace" seems 'restful' as you're reading the state of an object. I'm not sure if the RESTful model accounts for an object that is constantly changing, but otherwise that seems ok to me?

or is the difference that we are requesting a dump to disk in this call vs a return of the current value?

This returns the data directly. For this one I think it's more or less fine to do get, but for some other potential handlers it's weirder. "Start tracing for 10 seconds" is not a very restful behavior.

I think the prior art for that is a CREATE/POST call to create the profiling and then other requests to get the data. i.e.

CREATE /profile {"time": 10} -> {"id": 15} GET /profile/15 -> {...}

vs

POST /handler/profile {"time": 10} -> {...}

We'd have to overhaul the simpler handler design to support that I think -- so in someways would just prefer keeping it all POSTs even if it isn't pure "rest"

RESTful is a pretty overloaded term could you clarify what you mean?

Worth exploring HATEOAS that could support various cases given pluggable nature of handlers.

If we were going that route I think something like openapi or Swagger might be best. Could potentially generate thrift defs off of Swagger as well

I do like keeping things simple though initially until we flesh out what these interfaces should be

kurman · 2024-05-21T18:46:56Z

torch/csrc/distributed/c10d/control_plane/Handlers.hpp

Is it a standard pattern to update shared state in a constructor?

Yes, this is a static object guard -- we use the same thing for torch custom op registration. The shared state is in a meyers singleton in getHandlersRegistry so it neatly avoids any static initialization ordering issues.

Ex:

pytorch/torch/csrc/jit/runtime/custom_operator.h

Line 21 in ed32787

explicit RegisterOperators(std::vector<std::optional<Operator>> operators) {

torch/csrc/distributed/c10d/control_plane/WorkerServer.cpp

wconstab · 2024-05-21T20:08:04Z

didnt do a careful review yet but it looks great to me overall!

kurman

Overall looks good, approving to unblock

Would be good to get additional reviews on C++ code structure
Address failing tests

c-p-i-o · 2024-05-23T23:49:10Z

test/distributed/elastic/test_control_plane.py

+    @requires_cuda
+    def test_dump_nccl_trace_pickle(self) -> None:
+        with local_worker_server() as pool:
+            resp = pool.request("POST", "/handler/dump_nccl_trace_pickle")


c-p-i-o · 2024-05-23T23:56:12Z

torch/csrc/distributed/c10d/control_plane/Handlers.cpp

+
+    auto it = handlers_.find(name);
+    if (it == handlers_.end()) {
+      throw std::runtime_error(fmt::format("Failed to find handler {}", name));


Can this cause a crash at runtime if I call with a BAD request?

Or will it be caught somewhere?
Maybe it is better to just return some HTTP friendly code?

I verified that it doesn't cause a crash and just returns a 500 erorr. I added a custom error handling though to report a sane error instead.

c-p-i-o

LGTM - aside from the concern of crash when calling
curl http://host/NON_EXISTENT_PATH

This adds debug handlers as described in: * https://gist.github.com/d4l3k/828b7be585c7615e85b2c448b308d925 (public copy) * https://docs.google.com/document/d/1la68szcS6wUYElUUX-P6zXgkPA8lnfzpagMTPys3aQ8/edit (internal copy) This is only adding the C++ pieces that will be used from the main process. The Python and torchrun pieces will be added in a follow up PR. This adds 2 handlers out of the box: * `/handler/ping` for testing purposes * `/handler/dump_nccl_trace_pickle` as a POC integration with Flight Recorder Test plan: ``` python test/distributed/elastic/test_control_plane.py ``` Pull Request resolved: #126601 Approved by: https://github.com/kurman, https://github.com/c-p-i-o

PaliC · 2024-05-31T01:19:47Z

@pytorchbot revert -m "breaking internal typechecking tests" -c "ghfirst"

pytorchmergebot · 2024-05-31T01:21:18Z

@pytorchbot successfully started a revert job. Check the current status here.
Questions? Feedback? Please reach out to the PyTorch DevX Team

pytorchmergebot · 2024-05-31T01:21:27Z

@d4l3k your PR has been successfully reverted.

This reverts commit 3d54183. Reverted #126601 on behalf of https://github.com/PaliC due to breaking internal typechecking tests ([comment](#126601 (comment)))

This reverts commit 7646825.

This reverts commit 7646825. Pull Request resolved: #127805 Approved by: https://github.com/PaliC

This adds debug handlers as described in: * https://gist.github.com/d4l3k/828b7be585c7615e85b2c448b308d925 (public copy) * https://docs.google.com/document/d/1la68szcS6wUYElUUX-P6zXgkPA8lnfzpagMTPys3aQ8/edit (internal copy) This is only adding the C++ pieces that will be used from the main process. The Python and torchrun pieces will be added in a follow up PR. This adds 2 handlers out of the box: * `/handler/ping` for testing purposes * `/handler/dump_nccl_trace_pickle` as a POC integration with Flight Recorder Test plan: ``` python test/distributed/elastic/test_control_plane.py ``` Pull Request resolved: #126601 Approved by: https://github.com/kurman, https://github.com/c-p-i-o (cherry picked from commit 3d54183)

This reverts commit 3d54183. Reverted #126601 on behalf of https://github.com/PaliC due to breaking internal typechecking tests ([comment](#126601 (comment))) (cherry picked from commit 7646825)

This reverts commit 7646825. Pull Request resolved: #127805 Approved by: https://github.com/PaliC (cherry picked from commit 597922b)

This reverts commit 3d54183. Reverted pytorch#126601 on behalf of https://github.com/PaliC due to breaking internal typechecking tests ([comment](pytorch#126601 (comment)))

#126601 (internally [D58103182](https://www.internalfb.com/diff/D58103182)) was exported missing one class definition. This PR brings github repo in sync with fbcode. Pull Request resolved: #130296 Approved by: https://github.com/kit1980, https://github.com/seemethere, https://github.com/malfet

pytorch#126601 (internally [D58103182](https://www.internalfb.com/diff/D58103182)) was exported missing one class definition. This PR brings github repo in sync with fbcode. Pull Request resolved: pytorch#130296 Approved by: https://github.com/kit1980, https://github.com/seemethere, https://github.com/malfet

Because right now it leads to symbol conflict from binary builds. Use of `std::filesystem::file_exists` was introduced by #126601 and in this PR it is replaced with a very straightforward implementation that calls `stat` on the given path, which is a classic C-way of checking for the file existence. This PR should be reverted once one figures out how to keep `std::filesystem` methods linked into the binary private Fixes symptoms of #133437 Pull Request resolved: #134494 Approved by: https://github.com/atalman, https://github.com/d4l3k

Because right now it leads to symbol conflict from binary builds. Use of `std::filesystem::file_exists` was introduced by pytorch#126601 and in this PR it is replaced with a very straightforward implementation that calls `stat` on the given path, which is a classic C-way of checking for the file existence. This PR should be reverted once one figures out how to keep `std::filesystem` methods linked into the binary private Fixes symptoms of pytorch#133437 Pull Request resolved: pytorch#134494 Approved by: https://github.com/atalman, https://github.com/d4l3k

Because right now it leads to symbol conflict from binary builds. Use of `std::filesystem::file_exists` was introduced by #126601 and in this PR it is replaced with a very straightforward implementation that calls `stat` on the given path, which is a classic C-way of checking for the file existence. This PR should be reverted once one figures out how to keep `std::filesystem` methods linked into the binary private Fixes symptoms of #133437 Pull Request resolved: #134494 Approved by: https://github.com/atalman, https://github.com/d4l3k Co-authored-by: Nikita Shulga <nshulga@meta.com>

Because right now it leads to symbol conflict from binary builds. Use of `std::filesystem::file_exists` was introduced by pytorch#126601 and in this PR it is replaced with a very straightforward implementation that calls `stat` on the given path, which is a classic C-way of checking for the file existence. This PR should be reverted once one figures out how to keep `std::filesystem` methods linked into the binary private Fixes symptoms of pytorch#133437 Pull Request resolved: pytorch#134494 Approved by: https://github.com/atalman, https://github.com/d4l3k

pytorch-bot bot added oncall: distributed Add this issue/PR to distributed oncall triage queue release notes: distributed (torchelastic) labels May 18, 2024

d4l3k force-pushed the d4l3k/control_plane branch 4 times, most recently from 26c5779 to 7b71448 Compare May 21, 2024 18:06

d4l3k requested review from c-p-i-o, kiukchung, kurman, shuqiangzhang, suo, wconstab and zdevito May 21, 2024 18:09

d4l3k changed the title ~~distributed/control_plane~~ distributed debug handlers May 21, 2024

d4l3k marked this pull request as ready for review May 21, 2024 18:11

kurman reviewed May 21, 2024

View reviewed changes

d4l3k force-pushed the d4l3k/control_plane branch from 7b71448 to 1e2c7fc Compare May 21, 2024 19:28

d4l3k force-pushed the d4l3k/control_plane branch 2 times, most recently from 6cfac6d to 9dbfd35 Compare May 23, 2024 18:52

kurman approved these changes May 23, 2024

View reviewed changes

d4l3k force-pushed the d4l3k/control_plane branch 4 times, most recently from 256640a to f9ed48c Compare May 23, 2024 22:46

c-p-i-o reviewed May 23, 2024

View reviewed changes

c-p-i-o approved these changes May 23, 2024

View reviewed changes

d4l3k force-pushed the d4l3k/control_plane branch from f9ed48c to 041cfe5 Compare May 24, 2024 20:52

pytorchmergebot added the Merged label May 30, 2024

pytorchmergebot closed this in 3d54183 May 30, 2024

pytorchmergebot removed the merging label May 30, 2024

d4l3k deleted the d4l3k/control_plane branch May 30, 2024 15:59

pytorchmergebot added a commit that referenced this pull request May 31, 2024

Revert "distributed debug handlers (#126601)"

7646825

This reverts commit 3d54183. Reverted #126601 on behalf of https://github.com/PaliC due to breaking internal typechecking tests ([comment](#126601 (comment)))

pytorchmergebot added the Reverted label May 31, 2024

d4l3k added a commit that referenced this pull request Jun 3, 2024

Reapply "distributed debug handlers (#126601)"

f78cf1e

This reverts commit 7646825.

pytorchmergebot pushed a commit that referenced this pull request Jun 4, 2024

Reapply "distributed debug handlers (#126601)" (#127805)

597922b

This reverts commit 7646825. Pull Request resolved: #127805 Approved by: https://github.com/PaliC

bigfootjon pushed a commit that referenced this pull request Jun 5, 2024

Reapply "distributed debug handlers (#126601)" (#127805)

86d53d5

This reverts commit 7646825. Pull Request resolved: #127805 Approved by: https://github.com/PaliC (cherry picked from commit 597922b)

izaitsevfb added a commit that referenced this pull request Jul 8, 2024

fix discrepancy from the export of #126601

bdea161

izaitsevfb mentioned this pull request Jul 8, 2024

fix discrepancy from the export of #126601 #130296

Closed

pytorchmergebot pushed a commit that referenced this pull request Jul 9, 2024

fix discrepancy from the export of #126601

e43bb32

This was referenced Aug 26, 2024

PyTorch 2.8.0 exposes statically linked libstdc++ CXX11 ABI symbols. #133437

Closed

Do not use <filesystem> on Linux #134494

Closed

atalman mentioned this pull request Aug 27, 2024

Do not use <filesystem> on Linux (#134494) #134604

Merged

distributed debug handlers #126601

distributed debug handlers #126601

Uh oh!

Conversation

d4l3k commented May 18, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

pytorch-bot bot commented May 18, 2024 • 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/126601

❗ 1 Active SEVs

✅ You can merge normally! (1 Unrelated Failure)

Uh oh!

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!

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!

d4l3k May 21, 2024 • 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!

d4l3k May 23, 2024 • 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!

Uh oh!

wconstab commented May 21, 2024

Uh oh!

kurman 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!

Choose a reason for hiding this comment

Uh oh!

c-p-i-o left a comment

Choose a reason for hiding this comment

Uh oh!

PaliC commented May 31, 2024

Uh oh!

pytorchmergebot commented May 31, 2024

Uh oh!

pytorchmergebot commented May 31, 2024

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

6 participants

d4l3k commented May 18, 2024 •

edited

Loading

pytorch-bot bot commented May 18, 2024 •

edited

Loading

d4l3k May 21, 2024 •

edited

Loading

d4l3k May 23, 2024 •

edited

Loading