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

+        os.environ[TORCH_WORKER_SERVER_SOCKET] = socket_path
+
+        print("starting")
+        with main():


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

+    @requires_cuda
+    def test_dump_nccl_trace_pickle(self) -> None:
+        with local_worker_server() as session:
+            resp = session.post("http+unix://worker/handler/dump_nccl_trace_pickle")


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

+class RegisterHandler {
+ public:
+  RegisterHandler(const std::string& name, HandlerFunc f) {
+    registerHandler(name, f);


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

d4l3k · 2024-05-24T20:58:15Z

@pytorchbot merge

pytorchmergebot · 2024-05-24T21:00:07Z

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 · 2024-05-24T21:37:10Z

Merge failed

Reason: 11 mandatory check(s) failed. The first few are:

pull / linux-focal-cuda12.4-py3.10-gcc9 / test (default, 1, 5, linux.4xlarge.nvidia.gpu, unstable)
pull / linux-focal-cuda12.4-py3.10-gcc9 / test (default, 2, 5, linux.4xlarge.nvidia.gpu, unstable)
pull / linux-focal-cuda12.4-py3.10-gcc9 / test (default, 3, 5, linux.4xlarge.nvidia.gpu, unstable)
pull / linux-focal-cuda12.4-py3.10-gcc9 / test (default, 4, 5, linux.4xlarge.nvidia.gpu, unstable)
pull / linux-focal-cuda12.4-py3.10-gcc9 / test (default, 5, 5, linux.4xlarge.nvidia.gpu, unstable)

Dig deeper by viewing the failures on hud

Details for Dev Infra team

Raised by workflow job

Failing merge rule: Core Maintainers

d4l3k · 2024-05-29T21:36:16Z

@pytorchbot merge

pytorchmergebot · 2024-05-29T21:38:12Z

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

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)))

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 suo, shuqiangzhang, c-p-i-o, kiukchung, kurman, 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

d4l3k force-pushed the d4l3k/control_plane branch from 041cfe5 to 6f41b08 Compare May 24, 2024 20:54

pytorch-bot bot added the ciflow/trunk Trigger trunk jobs on your pull request label May 24, 2024

pytorchmergebot added the merging label May 24, 2024

pytorchmergebot removed the merging label May 24, 2024

distributed/control_plane

469b55d

d4l3k force-pushed the d4l3k/control_plane branch from 6f41b08 to 469b55d Compare May 29, 2024 21:36

pytorchmergebot added the merging label May 29, 2024

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

View reviewed changes

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

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

distributed debug handlers #126601

distributed debug handlers #126601

d4l3k commented May 18, 2024 •

edited

pytorch-bot bot commented May 18, 2024 •

edited

kurman May 21, 2024

d4l3k May 21, 2024

kurman May 21, 2024

kurman May 21, 2024

d4l3k May 21, 2024

d4l3k May 21, 2024

wconstab May 21, 2024

d4l3k May 21, 2024 •

edited

kurman May 22, 2024

d4l3k May 23, 2024 •

edited

kurman May 21, 2024

d4l3k May 21, 2024

wconstab commented May 21, 2024

kurman left a comment

c-p-i-o May 23, 2024

c-p-i-o May 23, 2024

d4l3k May 24, 2024

c-p-i-o left a comment

d4l3k commented May 24, 2024

pytorchmergebot commented May 24, 2024

pytorchmergebot commented May 24, 2024

d4l3k commented May 29, 2024

pytorchmergebot commented May 29, 2024

PaliC commented May 31, 2024

pytorchmergebot commented May 31, 2024

pytorchmergebot commented May 31, 2024

distributed debug handlers #126601

distributed debug handlers #126601

Conversation

d4l3k commented May 18, 2024 • edited

pytorch-bot bot commented May 18, 2024 • edited

🔗 Helpful Links

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

❗ 1 Active SEVs

✅ You can merge normally! (1 Unrelated Failure)

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

d4l3k May 21, 2024 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

d4l3k May 23, 2024 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

wconstab commented May 21, 2024

kurman 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

c-p-i-o left a comment

Choose a reason for hiding this comment

d4l3k commented May 24, 2024

pytorchmergebot commented May 24, 2024

Merge started

pytorchmergebot commented May 24, 2024

Merge failed

d4l3k commented May 29, 2024

pytorchmergebot commented May 29, 2024

Merge started

PaliC commented May 31, 2024

pytorchmergebot commented May 31, 2024

pytorchmergebot commented May 31, 2024

d4l3k commented May 18, 2024 •

edited

pytorch-bot bot commented May 18, 2024 •

edited

d4l3k May 21, 2024 •

edited

d4l3k May 23, 2024 •

edited