fix(server): loopback-bind the TP rendezvous store, add --dist-port - #461
Open
shikhakath wants to merge 1 commit into
Open
shikhakath wants to merge 1 commit into
shikhakath wants to merge 1 commit into
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Fixes #301.
The TP rendezvous port was hardcoded to
server_port + 1, so two instances on adjacent ports fight over it. There's also a second issue the thread surfaced: even though the init URL saystcp://127.0.0.1:PORT, torch's TCPStore ignores the host you give it and binds every interface -- so today it's an unauthenticated store reachable off-box on any bare-metal or--network hostsetup.This keeps TCP (per the thread's discussion, since a future multi-node rendezvous still needs it) but builds the store explicitly instead of trusting
init_method: rank 0 pre-binds a socket to127.0.0.1and hands the fd toTCPStoreviamaster_listen_fd. Other ranks connect as normal clients.--dist-portmakes the port configurable, defaulting to--port + 1so nothing changes for existing users.Testing
I don't have a GPU, and couldn't even install the package to run it locally (
flashlibhas no macOS wheel) -- so no checkpoint, noft serve, no driver to report.What I could check: I reproduced the exact logic standalone -- pre-bind a loopback socket, pass it to TCPStore via master_listen_fd, spawn two processes and run init_process_group + all_reduce over gloo. The socket only binds to 127.0.0.1, and the rendezvous still works. I've added that as two real tests
(
tests/engine/test_distributed_store.py,tests/server/test_dist_port_arg.py),but haven't run them against this actual package -- someone with an install should confirm they pass before merge.
Completely untested: the NCCL/TP>=2 path in
_init_communication. Same gap nobody in the original thread could close either. Happy to test on a multi-GPU box if someone points me at one.