Skip to content

Fix crash when reporting partial rings in mlx.distributed_config - #4026

Merged
zcbenz merged 1 commit into
ml-explore:mainfrom
ayaangazali:fix-ring-diagnostic-crash
Aug 6, 2026
Merged

Fix crash when reporting partial rings in mlx.distributed_config#4026
zcbenz merged 1 commit into
ml-explore:mainfrom
ayaangazali:fix-ring-diagnostic-crash

Conversation

@ayaangazali

@ayaangazali ayaangazali commented Aug 6, 2026

Copy link
Copy Markdown
Contributor

Proposed changes

When mlx.distributed_config cannot find a full ring it tries to print the partial rings it did find, but that diagnostic path raises TypeError instead:

TypeError: list indices must be integers or slices, not list

extract_rings returns (path, count) pairs:

rings.append((r, cnt))
...
return sorted(rings, key=lambda x: -len(x[0]))

check_valid_ring already unpacks correctly when testing for a full ring (len(rings[0][0]) == len(hosts)), but the error branch iterates the pairs as if each element were a bare path and indexes hosts with it:

for r in rings:
    log_error(f" - {','.join(hosts[i].ssh_hostname for i in r)}")

so i is the path list itself rather than a node index.

This means the failure is worst exactly when the diagnostic matters most: a user whose fabric is miswired gets a traceback instead of the list of rings that were found, and loses the hint to run --dot.

Repro

The branch is on the error path whenever a partial ring exists and no full ring does:

class H:
    def __init__(self, n): self.ssh_hostname = n

hosts = [H("h1"), H("h2"), H("h3")]
rings = [([0, 1], 2)]                     # partial ring, shape as returned by extract_rings

for r in rings:                           # current code
    print(" - " + ",".join(hosts[i].ssh_hostname for i in r))
# TypeError: list indices must be integers or slices, not list

After the change the same input prints what was intended:

 - h1,h2

Checklist

  • I have read the CONTRIBUTING document
  • I have run pre-commit run --all-files to format my code / installed pre-commit prior to committing changes
  • I have added tests that prove my fix is effective or that my feature works
  • I have updated the necessary documentation (if needed)

No test added: this is a one-line fix in an error-reporting branch that needs a miswired multi-host Thunderbolt fabric to reach naturally, and this repo has trimmed tests from small fixes of this size before (#3914). Happy to add one if you would prefer.

extract_rings returns (path, count) pairs, but the diagnostic in
check_valid_ring iterated the list as if each element were a bare path
and indexed hosts with it, raising TypeError instead of printing the
rings it found.
@zcbenz
zcbenz merged commit 32df751 into ml-explore:main Aug 6, 2026
28 checks passed
krystophny pushed a commit to krystophny/mlx-jaccl-fix-small-recv that referenced this pull request Aug 7, 2026
krystophny pushed a commit to krystophny/mlx-jaccl-fix-small-recv that referenced this pull request Aug 7, 2026
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants