Repository navigation
Fix lost rank output in the distributed launcher - #4615
Merged
Merged
Conversation
zcbenz
requested changes
Oct 6, 2026
zcbenz
left a comment
zcbenz
left a comment
Member
There was a problem hiding this comment.
Instead of duplicating the read code, I think we should just keep reading until error happens, there is no need to check p.poll().
zcbenz
added
the
await response
Contributor
Author
|
Updated to use a single read loop. Kept Local ring tests pass. I also tested SSH manually between two Macs using bash ranks. With delayed reads, main loses output, but not with this patch. Could a maintainer approve the CI run? |
zcbenz
removed
the
await response
Contributor
Author
|
Fixed the CI test failures. Windows now skips the launcher tests, and the two timing-sensitive tests use file synchronization instead of sleeps. Ruff passes. Tested 10/10 on macOS and 8/8 on Linux aarch64. |
The reader thread stops as soon as poll() reports the process has exited. Output the process wrote after the last select() is still in the pipe and was dropped, so a rank that prints its result and exits right away could lose it, more often on a loaded machine. Read the pipes to EOF after the loop.
The launcher's reads are slowed so each rank prints its last line and exits while the reader thread is still busy; the test fails on the previous launcher and passes with the drain.
zcbenz
force-pushed
the
fix/launch-drain-rank-output
branch
from
October 10, 2026 06:02
d514775 to
32bf7aa
Compare
zcbenz
approved these changes
Oct 10, 2026
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
2 participants
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.
The reader thread in _launch_with_io exits as soon as p.poll() reports the rank is done, which can drop output still buffered in the pipe. This caused intermittent missing output in oMLX CI.
The fix drains both pipes to EOF after the loop. Reads remain non-blocking, so it won't hang if a pipe is still open.
The test delays launcher reads so ranks can write their final line and exit first. It fails on main and passes 10/10 with the fix. Tested on macOS with the ring backend on 127.0.0.1; ssh was not tested.