PRs stuck for 2 months, for MPS perf

1

Two MPS perf PRs have been done and green for a few weeks but I can’t get eyes on them:

  • PR #184780 (unique): native Metal kernel replacing the MPSGraph scatter path. ~12,000x perf improvement, and closes issues: #97310 and #111173. Comments addressed, no formal blocks, so I think it just needs one approval to self-merge.
  • PR #184779 (bincount): same idea, ~9,600x perf improvement. All comments addressed, but stuck behind a CHANGES_REQUESTED from May that predates the fixes (reviewer’s been unreachable).

Who’s the right person to review these or clear the stale block? I’ve been trying to reach the reviewers but haven’t heard back for 8 weeks.

2

Hey!

I’m afraid we get a lot more PRs than we have people that can review them. So there is some delays :confused:

The right person is already on the PR: malfet.

Especially for the MPS backend as it is only volunteers working on it at the moment as no company is really supporting that effort.

Cheers,

Alban

3

It’s very easy to write a kernel(especially with the help of AI) that achieves amazing speed up for a particular shape / normalized input, but it is much harder to ensure a correctness/do not cause regressions across plethora of shapes.

If you noticed, both your PRs received high number of comments(20+) from reviewers highlighting one or another shortcoming of proposes implementation, and because benchmarking is not something current harness automates it requires manual verification.

Last but not least, due to the volume of PRs and scarcity of reviewers, we can try to guarantee timely reviews only on PRs that address concerns raised in the issues marked as actionable (see link below), and neither of them has been marked as such

4

Thanks @malfet, and thanks for landing unique – and fair enough, this stuff is genuinely hard to verify. For what it’s worth, both ops run the OpInfo sweep (cross-shape correctness vs CPU), and each PR ships a reproducer plus an n=200 A/B benchmark to keep the manual perf check cheap.

Bincount (#184779) is down to one open design call now; weighted-output dtype, keep the MPS contract vs match CPU/CUDA!

And thanks again for sticking with me to the end here - I love the work you’ve done to make MPS great, and I’m happy to just be able to contribute to it :slight_smile: