[c10d] Socket, TCPStore: add better logging #128673
Conversation
🔗 Helpful Links🧪 See artifacts and rendered test results at hud.pytorch.org/pr/128673
Note: Links to docs will display an error until the docs builds have been completed. ✅ No FailuresAs of commit c3fc85f with merge base 5a80d2d ( This comment was automatically generated by Dr. CI and updates every 15 minutes. |
pytorch-bot
bot
added
oncall: distributed
d4l3k
force-pushed
the
d4l3k/socket_logging
branch
from
520eb58 to
c3fc85f
Compare
|
@pytorchbot merge |
Merge startedYour 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 |
| C10_THROW_ERROR( | ||
| DistNetworkError, | ||
| fmt::format( | ||
| "failed to format addr, unknown family={}", addr.ai_family)); | ||
| } |
There was a problem hiding this comment.
We're seeing
failed to format addr, unknown family=0
in a job which would otherwise run fine if we disable logging by TORCH_CPP_LOG_LEVEL=WARNING.
This is probably a bug - we shall not assume we're formatting a VALID addrinfo. When connection is being established there might be invalid addrinfo to be logged.
There was a problem hiding this comment.
We have not identified exactly what causes the invalid addrinfo to appear, but I'm sending https://github.com/pytorch/pytorch/pull/137745/files and I believe it is a good fix regardless. In general, formatter should avoid crashing as much as possible.
Formatter shall avoid throwing exceptions as much as possible. Fixes #128673 (comment) Pull Request resolved: #137745 Approved by: https://github.com/d4l3k, https://github.com/Skylion007
This adds better logging of errors to the socket and TCPStore classes.
All socket operations should now include the local and remote addresses and we actually log errors from the TCPStoreBackend::run as well as TCPStoreBackendUV which were previously INFO messages and not actually logged.
It also overhauls test_wait in test_store.py as it had a race condition causing it to be flaky.
Test plan:
cc @mrshenli @pritamdamania87 @zhaojuanmao @satgera @gqchen @aazzolini @osalpekar @jiayisuse @H-Huang @kwen2501 @awgu @penguinwu @fegin @XilunWu @wanchaol @fduwjj @wz337 @tianyu-l @wconstab @yf225 @chauhang