Repository navigation
Fix C++ op implementations missing stream arg - #4746
Conversation
c76208a to
6f4741f
Compare
mlx-swift main now carries ml-explore/mlx#4746, which records the rfft padding on the caller's stream. Drop the explicit `padded` workaround and call `rfft(frames, n:)` again. Keep the cross-thread tests, which pass without it, and point their comment at the core fix.
88310d6 to
c549d6c
Compare
…g the stream, in that case op is running on the default stream. This commit is fixing found sites, adds regression tests which were failing before and passsing after. Found when working on my own app and got crashes with: ``` MLX/ErrorHandler.swift:345: Fatal error: There is no Stream(gpu, 6) in current thread. ```
The ops stream test used detail::InTracing and built a distributed::AllGather node directly. Neither is exported from the library, so the test executable failed to link in the Linux and Windows CI jobs, which build mlx as a shared library with hidden visibility: undefined references to InTracing::trace_stack(), InTracing::grad_counter and the AllGather vtable. The dynamic-shape case now goes through a shapeless compile, the only public path that traces dynamic shapes. The all-gather vjp subcase is dropped: a singleton group never builds the node, so there is no public way to reach it from a unit test.
16efa63 to
ff95829
Compare
ff95829 to
ba4d7b8
Compare
59e481e to
c1e9fdd
Compare
|
Thanks for this PR, glad I found it! While running Qwen3-TTS voice cloning in Swift (mlx-audio-swift), I hit a 2x slowdown compared to the Python version of the same model, and it turns out the When So on top of the crash, this PR also fixes that slowdown:
Mac mini M2 Pro. Swift: Python reproimport time, mlx.core as mx
mx.random.seed(0)
w, s, b = mx.quantize((mx.random.normal((2048, 1024)) * 0.02).astype(mx.bfloat16), group_size=64, bits=8)
mx.eval(w, s, b)
def bench(x, stream):
eps = [mx.array(i * 1e-3, x.dtype) for i in range(75)]; mx.eval(eps); x = mx.array(x); mx.eval(x)
f = lambda: mx.eval([mx.quantized_matmul(mx.add(x, e, stream=stream), w, s, b, transpose=True, group_size=64, bits=8, stream=stream) for e in eps])
for _ in range(10): f()
t0 = time.perf_counter()
for _ in range(50): f()
return (time.perf_counter() - t0) / 50 * 1e3
print("mlx", mx.__version__)
other = mx.new_stream(mx.gpu)
for name, st in [("default", mx.default_stream(mx.gpu)), ("new_stream", other)]:
for dt in [mx.bfloat16, mx.float32]:
x = mx.random.normal((1, 1, 1024)).astype(dt); mx.eval(x)
print(f"stream={name} x={'f32' if dt == mx.float32 else 'bf16'} scales=bf16 ms_per_75={bench(x, st):.2f}")Investigated with AI: it traced the slowdown to this hunk, wrote the same fix with a test, and ran the benchmarks above. I then used it to search the open and closed PRs for an existing fix, which is how I found this one. Leaving this here so the speed improvement is on record for anyone who runs into the same slowdown. |
Fixes missing stream argument in some ops. Ops call helper op skipping the stream argument, in that case op is running on the default stream, causing a crash.
This PR is fixing found sites, adds regression tests which were failing before and passing after, adds a guard and commit-hook to check stream args passing.
Found when working on my own app and got crashes with:
AI usage disclosure: Fix written with Claude Code Opus 5.5, verified and adjusted by Fable 5.1 and Astra 6