Skip to content

chore: Fix assertion in ReduceScatter::eval_cpu - #4557

Merged
zcbenz merged 1 commit into
ml-explore:mainfrom
PicoMLX:sum-scatter-assert
Sep 28, 2026
Merged

zcbenz merged 1 commit into
ml-explore:mainfrom
PicoMLX:sum-scatter-assert

Conversation

@ronaldmannak

@ronaldmannak ronaldmannak commented Sep 24, 2026 •

Copy link
Copy Markdown
Contributor

ReduceScatter::eval_cpu is asserting inputs.size() == 0 while reading the first element, thus aborting valid calls when assertions are enabled.
This PR asserts inputs.size() == 1.

  • ☑️ I understand it is strictly prohibited to use AI to write PR description
  • AI usage disclosure: Opus 5.5 found this the issue while working on JACCL

@ronaldmannak

Copy link
Copy Markdown
Contributor Author

This is ready for review, but it appears there's a limit how many PRs a person can open

@ronaldmannak ronaldmannak changed the title Assert inputs.size = 1 ReduceScatter::eval_cpu: Assert inputs.size = 1 Sep 24, 2026
@zcbenz

zcbenz commented Sep 25, 2026

Copy link
Copy Markdown
Member

The limitation is set because we are unable to spend too much time reviewing contributor PRs at the moment, please do not open new PRs before we review your current open ones.

@ronaldmannak

Copy link
Copy Markdown
Contributor Author

@zcbenz Got it! I split changes into small PRs thinking it'd make reviewing easier for you :) I have a few more lined up, so I'll wait. BTW, does the one-PR-at-a-time rule also apply to mlx-swift?

@zcbenz

zcbenz commented Sep 25, 2026

Copy link
Copy Markdown
Member

mlx-swift receives much less PRs so it does not have the restriction, but I think it is generally good practice not to open too much PRs at the same time.

@zcbenz
zcbenz marked this pull request as ready for review September 25, 2026 01:10
@zcbenz zcbenz changed the title ReduceScatter::eval_cpu: Assert inputs.size = 1 chore: Fix assertion in ReduceScatter::eval_cpu Sep 28, 2026
@zcbenz
zcbenz merged commit 42987b6 into ml-explore:main Sep 28, 2026
29 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants