[c10d][libuv] add partial read test for libuv backend and fix an error which only happens when partially reading a buffer #116141

XilunWu · 2023-12-19T23:20:08Z

Stack from ghstack (oldest at bottom):

[c10d][libuv] refactor TCPStore to allow reuse of SendBuffer #116180
-> [c10d][libuv] add partial read test for libuv backend and fix an error which only happens when partially reading a buffer #116141

Test Plan

build pytorch
execute TORCH_CPP_LOG_LEVEL=INFO build/bin/TCPStoreTest --gtest_filter=TCPStoreTest.testLibUVPartialRead from the pytorch root directory.

without the change:

with the change:

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

…r which only happens when partially reading a buffer [ghstack-poisoned]

pytorch-bot · 2023-12-19T23:20:13Z

🔗 Helpful Links

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

📄 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! (2 Unrelated Failures)

As of commit 2730aea with merge base 5ba87a3 ():

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

pull / linux-focal-py3.11-clang10 / test (dynamo, 1, 7, linux.2xlarge, unstable) (gh)
test_unary_ufuncs.py::TestUnaryUfuncsCPU::test_reference_numerics_large__refs_special_multigammaln_mvlgamma_p_5_cpu_int16
pull / linux-focal-py3.8-clang10 / test (dynamo, 1, 7, linux.2xlarge, unstable) (gh)
test_unary_ufuncs.py::TestUnaryUfuncsCPU::test_reference_numerics_large__refs_special_multigammaln_mvlgamma_p_5_cpu_int16

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

…r which only happens when partially reading a buffer ghstack-source-id: 75197e797234bad767eb53290ff2b598d8840fc1 Pull Request resolved: #116141

…fix an error which only happens when partially reading a buffer" cc mrshenli pritamdamania87 zhaojuanmao satgera rohan-varma gqchen aazzolini osalpekar jiayisuse H-Huang kwen2501 awgu penguinwu fegin wanchaol fduwjj wz337 tianyu-l wconstab yf225 [ghstack-poisoned]

…r which only happens when partially reading a buffer ghstack-source-id: 861efed0a47ca8e8a6dd6a10b9c82009ccd057a5 Pull Request resolved: #116141

…fix an error which only happens when partially reading a buffer" **Test Plan** 1. build pytorch 2. execute `TORCH_CPP_LOG_LEVEL=INFO build/bin/TCPStoreTest --gtest_filter=TCPStoreTest.testLibUVPartialRead` from the pytorch root directory. without the change: <img width="761" alt="image" src="https://github.com/pytorch/pytorch/assets/12968408/1942e3c2-a9c1-4fe4-87e8-7e21f4d8f9aa"> with the change: <img width="747" alt="image" src="https://github.com/pytorch/pytorch/assets/12968408/f3e96a5b-0ed1-49bd-9184-bb8a5ebebc33"> cc mrshenli pritamdamania87 zhaojuanmao satgera rohan-varma gqchen aazzolini osalpekar jiayisuse H-Huang kwen2501 awgu penguinwu fegin wanchaol fduwjj wz337 tianyu-l wconstab yf225 [ghstack-poisoned]

…r which only happens when partially reading a buffer ghstack-source-id: c88cb2e6fc431a1f00e1b826614eafd7fc0564c9 Pull Request resolved: #116141

fduwjj · 2023-12-20T00:52:52Z

torch/csrc/distributed/c10d/TCPStore.cpp

@@ -396,6 +396,18 @@ void TCPStore::validate(void) {
  buffer.flush();
 }

+void TCPStore::_splitSet(


Can we actually make this into test by inherit TCPStore and add it to the child class instead of adding it to TCPStore itself?

IMO that would require a refactor of TCPStore because the TCPClient object is a Private member of TCPStore and can only be accessed from the TCPStore object.

i think @fduwjj means, can you make a TestTCPStore class in TcpStoreTest.cpp, and just implement the extra method on that?

(Agree its better not to put this code into the real TCPStore file)

I have tried that attempt, but the thing is, the subclass cannot access the private member of the superclass, so the _splitSet function in the subclass won't be able to use the buffer

ok. could probably change the base class member to protected, add your test class as a friend class, or just copy some code into the test class.

Or, maybe you don't even need a tcpstore at all- just create a raw tcpclient obj in your test case, and then write the 'split_set' as a standalone helper function that uses tcpclient?

ok. i was thinking about saying that you should just reverse the order, land the next refactor PR first, then land this one. But i think its worth landing the fix now and its not that bad to add the _split_set, as long as you promise to clean it up soon.

@wconstab @fduwjj for the follow-up there're several options I can come up with, let me know your opinions:

in test's TCP store client thread, do not use any TCPStore, SendBuffer, or TCPClient because they either cannot be used outside of TCPStore.cpp or encapsulate private members from users. Instead, use the Socket to perform the exact same thing as in those classes. The downside is making the test quite verbose (but we can hide them in test util files).

change some members to Protected and make the test class a friend class. The downside is I don't know the consequence of making the underlying networking components Protected. Can there be any security issue? Who would be the expert we can ask?

Refactor the structure of TCPStore.hpp and TCPStoreBackend.hpp. Currently, TCPClient and SendBuffer can only have incomplete declaration in TCPStore.hpp because QueryType is defined in TCPStoreBackend.hpp and TCPStoreBackend.hpp include TCPStore.hpp. This forbids them from being exposed outside of TCPStore.cpp. The question is, do we want to refactor them? Any benefit? A single test may not be a good justification.

I think #2 should be fine? What security issue can you think of?

If another class A inherits TCPStore, the class A or an instance of A can access a TCPStore's buffer and read/write on it. I assume the original design decision (making them private) is to avoid this.

Well, what's the problem for that? If it's a child of TCPStore then it's ok to access the buffer?

test/cpp/c10d/TCPStoreTest.cpp

fduwjj · 2023-12-20T01:16:50Z

test/cpp/c10d/TCPStoreTest.cpp

+      c10d::test::check(*clientTCPStore, key, "v");
+    }
+  });
+


Do you also need serverTCPStore = nullptr; here?

That will cause the issue. If we have it here, the server could be destructed before the client completes its execution.

Then maybe add after clientThread Join?

When the unique_ptr object serverTCPStore goes out of the scope (in this case it's the end of the test), the object will be destroyed (same as serverTCPStore = nullptr) and the TCPStore will be freed. So adding serverTCPStore = nullptr will just be the same as the current code.

I guess my point is to set to null after join, by the time of join, all clients should have finished? But since this is a unique pointer, it should be fine.

torch/csrc/distributed/c10d/TCPStore.cpp

…fix an error which only happens when partially reading a buffer" **Test Plan** 1. build pytorch 2. execute `TORCH_CPP_LOG_LEVEL=INFO build/bin/TCPStoreTest --gtest_filter=TCPStoreTest.testLibUVPartialRead` from the pytorch root directory. without the change: <img width="761" alt="image" src="https://github.com/pytorch/pytorch/assets/12968408/1942e3c2-a9c1-4fe4-87e8-7e21f4d8f9aa"> with the change: <img width="747" alt="image" src="https://github.com/pytorch/pytorch/assets/12968408/f3e96a5b-0ed1-49bd-9184-bb8a5ebebc33"> cc mrshenli pritamdamania87 zhaojuanmao satgera rohan-varma gqchen aazzolini osalpekar jiayisuse H-Huang kwen2501 awgu penguinwu fegin wanchaol fduwjj wz337 tianyu-l wconstab yf225 [ghstack-poisoned]

…r which only happens when partially reading a buffer ghstack-source-id: 6067f62bfdc8d94411f42758102a612d97dd4994 Pull Request resolved: #116141

XilunWu · 2023-12-20T18:34:15Z

@pytorchbot merge

pytorchmergebot · 2023-12-20T18:37:33Z

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

…r which only happens when partially reading a buffer (pytorch#116141) **Test Plan** 1. build pytorch 2. execute `TORCH_CPP_LOG_LEVEL=INFO build/bin/TCPStoreTest --gtest_filter=TCPStoreTest.testLibUVPartialRead` from the pytorch root directory. without the change: <img width="761" alt="image" src="https://github.com/pytorch/pytorch/assets/12968408/1942e3c2-a9c1-4fe4-87e8-7e21f4d8f9aa"> with the change: <img width="747" alt="image" src="https://github.com/pytorch/pytorch/assets/12968408/f3e96a5b-0ed1-49bd-9184-bb8a5ebebc33"> Pull Request resolved: pytorch#116141 Approved by: https://github.com/wconstab

@XilunWu

We've been facing issues where TCPStore can successfully connect but then fail in the validate() function due to resets from listen backlog queue overflow when combined with reset enabled as well as long init times. This PR does a few things: * Retry that connect and validate up to the specified timeout. * Use exponential backoff for the retry logic with jitter instead of a fixed 1s sleep. * Eliminate the `sleep(std::chrono::milliseconds(numWorkers))` on init which can add significant delays to startup. This is no longer necessary per @XilunWu #116141 Test plan: ``` python test/distributed/test_store.py -v ./build/bin/BackoffTest ``` Will do internal testing with some large scale jobs to ensure TCPStore works correctly. At 4k scale: 4x improvement ``` tristanr@devvm4382 ~/pt_tests [SIGABRT]> time TORCH_SHOW_CPP_STACKTRACES=1 python tcpstore_large_test.py (pytorch-3.10) started 0 init 0 set 0 joined all ________________________________________________________ Executed in 1.98 secs fish external usr time 0.93 secs 91.00 micros 0.93 secs sys time 1.98 secs 954.00 micros 1.97 secs tristanr@devvm4382 ~/pt_tests> conda activate torchdrive-3.10 (pytorch-3.10) tristanr@devvm4382 ~/pt_tests> time TORCH_SHOW_CPP_STACKTRACES=1 python tcpstore_large_test.py (torchdrive-3.10) started 0 init 0 set 0 joined all ________________________________________________________ Executed in 8.20 secs fish external usr time 2.15 secs 0.00 micros 2.15 secs sys time 2.76 secs 843.00 micros 2.76 secs ``` ```py import time import os import threading from multiprocessing import Pool WORLD_SIZE = 10000 import torch.distributed as dist def run(rank): should_log = rank % (WORLD_SIZE // 10) == 0 if should_log: print(f"started {rank}") store = dist.TCPStore( host_name="devvm4382.nao0.facebook.com", port=29500, world_size=WORLD_SIZE, is_master=rank == 0, use_libuv=True, ) if should_log: print(f"init {rank}") store.set(f"key{rank}", "1234") if should_log: print(f"set {rank}") del store def noop(rank): pass print("starting pool") with Pool(WORLD_SIZE) as pool: pool.map(noop, range(WORLD_SIZE), 1) print("pool hot") start = time.time() pool.map(run, range(WORLD_SIZE), 1) print("run finished", time.time()-start) ``` ``` tristanr@devvm4382 ~/pt_tests> python tcpstore_large_test.py (pytorch-3.10) starting pool pool hot started 0 [W624 16:58:09.086081750 TCPStore.cpp:343] [c10d] Starting store with 10000 workers but somaxconn is 4096.This might cause instability during bootstrap, consider increasing it. started 1000 init 1000 set 1000 started 2000 init 2000 set 2000 started 3000 init 3000 set 3000 started 4000 init 4000 set 4000 started 5000 init 5000 set 5000 started 6000 init 6000 set 6000 started 7000 init 7000 set 7000 started 8000 init 8000 set 8000 started 9000 init 9000 set 9000 init 0 set 0 run finished 0.705092191696167 ``` Pull Request resolved: #129261 Approved by: https://github.com/rsdcastro, https://github.com/wconstab, https://github.com/kurman, https://github.com/XilunWu, https://github.com/c-p-i-o

[c10d][libuv] add partial read test for libuv backend and fix an erro…

9685b82

…r which only happens when partially reading a buffer [ghstack-poisoned]

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

github-actions bot added the oncall: distributed Add this issue/PR to distributed oncall triage queue label Dec 19, 2023

XilunWu requested review from H-Huang, wconstab, fduwjj, shuqiangzhang and cbalioglu December 19, 2023 23:25

XilunWu added ciflow/periodic Trigger jobs ran periodically on master (periodic.yml) on the PR ciflow/trunk Trigger trunk jobs on your pull request labels Dec 20, 2023

fduwjj reviewed Dec 20, 2023

View reviewed changes

test/cpp/c10d/TCPStoreTest.cpp Outdated Show resolved Hide resolved

fduwjj reviewed Dec 20, 2023

View reviewed changes

XilunWu commented Dec 20, 2023

View reviewed changes

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

XilunWu mentioned this pull request Dec 20, 2023

[c10d][libuv] refactor TCPStore to allow reuse of SendBuffer #116180

Closed

wconstab approved these changes Dec 20, 2023

View reviewed changes

pytorchmergebot added the merging label Dec 20, 2023

pytorchmergebot added the Merged label Dec 20, 2023

pytorchmergebot closed this in 0b0b9b3 Dec 20, 2023

pytorchmergebot removed the merging label Dec 20, 2023

facebook-github-bot deleted the gh/XilunWu/54/head branch December 24, 2023 15:23

XilunWu mentioned this pull request Dec 28, 2023

PyTorch Distributed Elastic Launch Segmentation Fault with Python 3.12 #116423

Open

d4l3k mentioned this pull request Jun 24, 2024

TCPStore: improve connect and retry logic #129261

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[c10d][libuv] add partial read test for libuv backend and fix an error which only happens when partially reading a buffer #116141

[c10d][libuv] add partial read test for libuv backend and fix an error which only happens when partially reading a buffer #116141

XilunWu commented Dec 19, 2023 •

edited

Loading

pytorch-bot bot commented Dec 19, 2023 •

edited

Loading

fduwjj Dec 20, 2023

XilunWu Dec 20, 2023

wconstab Dec 20, 2023 •

edited

Loading

XilunWu Dec 20, 2023

wconstab Dec 20, 2023

wconstab Dec 20, 2023

XilunWu Dec 20, 2023

fduwjj Dec 20, 2023

XilunWu Dec 20, 2023

fduwjj Dec 26, 2023

fduwjj Dec 20, 2023

XilunWu Dec 20, 2023

fduwjj Dec 20, 2023

XilunWu Dec 20, 2023

fduwjj Dec 26, 2023

XilunWu commented Dec 20, 2023

pytorchmergebot commented Dec 20, 2023

[c10d][libuv] add partial read test for libuv backend and fix an error which only happens when partially reading a buffer #116141

[c10d][libuv] add partial read test for libuv backend and fix an error which only happens when partially reading a buffer #116141

Conversation

XilunWu commented Dec 19, 2023 • edited Loading

pytorch-bot bot commented Dec 19, 2023 • edited Loading

🔗 Helpful Links

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

✅ You can merge normally! (2 Unrelated Failures)

Choose a reason for hiding this comment

Choose a reason for hiding this comment

wconstab Dec 20, 2023 • edited Loading

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

Choose a reason for hiding this comment

XilunWu commented Dec 20, 2023

pytorchmergebot commented Dec 20, 2023

Merge started

XilunWu commented Dec 19, 2023 •

edited

Loading

pytorch-bot bot commented Dec 19, 2023 •

edited

Loading

wconstab Dec 20, 2023 •

edited

Loading