update for mlx v0.32.2 - #450
Conversation
|
There is an incoming 0.32.0 mlx-c in ml-explore/mlx-c#123. #445 should take care of the JIT issue. |
a0fff05 to
d876219
Compare
|
|
||
| - name: Patch mlx submodule | ||
| shell: sh | ||
| run: ./tools/patch-mlx.sh |
There was a problem hiding this comment.
We should not need this -- we should rely on mlx and mlx-c tags. Anything that needs special handling should be done statically via tools at the time when we make the new release.
In particular SwiftPM can't apply patches (or make any source mods) so this won't really work.
There was a problem hiding this comment.
You're right, and the SwiftPM point settles it — removed in 2d27240. I'd wired the patch into the places this repo controls (update-mlx.sh, both workflows, the CMake PATCH_COMMAND, a README step), but a package consumer gets the submodule pristine and never runs any of it, so they'd build with the bug regardless. That's the path that actually matters and I hadn't covered it. There's no static option either: the patch touches headers inside the submodule, so there's nothing checked in for tools/ to bake in at release time. update-mlx.sh, README.md and MAINTENANCE.md are byte-identical to main again.
What that leaves: ml-explore/mlx#4086 (the ordering fix, from #4083) is on main but unreleased — v0.32.0 is still the newest tag and git tag --contains for it is empty. So on the Xcode 27 SDK, libc++ validates the comparator and eval() aborts out of std::set<Stream> in eval_impl. It isn't confined to a couple of tests; it fires wherever cpu and gpu streams are both open, so the run aborts rather than reporting failures. Builds are all fine — swift build, iOS, and the xcodeproj all succeed.
So this needs an mlx release containing #4086 before it can go green. Happy to hold it until then and re-pin, or leave it open as-is if you'd rather have the diff visible in the meantime.
Separately, I rebased onto #445 and dropped my hand-added jit-source entries — I checked the derived sets match what the hand bump produced (48 Metal targets, 22 CUDA sources), so the committed mlx-generated output is unchanged. One leftover I'd flag: CMakeLists.txt still declares mlx ahead of mlx-c to pin v0.32.0, because mlx-c currently pins v0.31.2 and the two build paths would otherwise disagree. That comes out once mlx-c pins 0.32.0 itself, e.g. via #123.
|
|
||
| public static let gpu = Stream(mlx_default_gpu_stream_new()) | ||
| public static let cpu = Stream(mlx_default_cpu_stream_new()) | ||
| /// Make a stream on `device` that can be used from any thread. |
There was a problem hiding this comment.
We have been pretty loose about streams until now. I think we may need to:
- manage a per task/thread stream, e.g. if you create a new task you get an implicit new stream unless you use some kind of
withNewStreamthe of API - because of [BUG] crash while using new_stream mlx#2118 I think we may need to manage a pool of streams -- we can't create and destroy them at will
There was a problem hiding this comment.
Understood — reading this as direction rather than something to solve here. Leaving it out of this PR.
Recording where I got to, in case it's useful later. The minimal step looks like it needs no new API: mlx_default_gpu_stream_new() already calls default_stream(DeviceType::gpu), and default_stream_storage is static thread_local as of 0.31.2, so each call already returns the calling thread's stream. The only reason it breaks is that Stream.gpu/.cpu are static let, so the first thread to touch them wins. Making those (and StreamOrDevice.cpu/.gpu, and Device.defaultStream, which is a stored property) resolve per thread would fix the three failing tests without a shim or an mlx-c binding.
It wouldn't be a complete answer, which I think is your point: withNewDefaultStream installs one new_stream as a @TaskLocal, so a task that hops threads across an await still breaks, and it does nothing for #2118 — Stream(_ device:) still creates a stream per call. Pooling plus per-task streams is the real fix.
So for this PR the three thread tests fail (MLXRandomTests/testRandomThreadsDifferent, testRandomThreadsSame, TransformTests/testVmapThreadSafety — all pass on 0.31.1). I've left them failing rather than skipping them, so the behaviour change stays visible; happy to add XCTSkips pointing at this thread if you'd rather.
One correction to what I wrote earlier: I said this had to land with or before the bump. That was wrong — the suite can't be green anyway until mlx releases the operator< fix, so the two are independent.
568e965 to
2d27240
Compare
|
Sharing notes from a working bump of the vendored core to v0.32.1 on a fork, in case they save this PR a cycle when it moves forward (or forward to 0.32.1):
Everything above is public at |
5d6775f to
20b1dfa
Compare
mlx v0.32.2 - carry `Source/Cmlx/mlx.patch` for mlx's `Device::operator<` / `Stream::operator<`, which are not strict weak orderings and now run on every `eval()` via `std::set<Stream>`. libc++ aborts on them under the Xcode 27 SDK. Still broken upstream in v0.32.0 and on main, no issue filed. Applied by `tools/patch-mlx.sh` from `update-mlx.sh` and CI, since `git submodule update` restores a pristine checkout. - streams became thread affine in v0.31.2, which broke the multi-threaded tests. Adopt mlx's `new_thread_unsafe_stream` (added upstream for language bindings that lock themselves -- mlx-swift serializes on `evalLock`) via a small C shim, as mlx-c does not bind it. - FFT ops gained a `norm` parameter: add `FFTNorm` and thread it through, plus `fftfreq`/`rfftfreq`. - wrap the new reducing slice-updates in `ArrayAtIndices` and add `exportToDot`/`graphDescription`/`NodeNamer`. - fix `SEEK_END` in the in-memory IO stream -- it computed from the read position rather than the end of the buffer, which v0.32.0's `load_safetensors` size check exposed. - route `cpu/jit_compiler.cpp` through `mlx-conditional`: it shells out via `std::system()`, unavailable on iOS. - `gemv` is JIT compiled as of v0.32.0, so drop `gemv.metal` from the metallib in `fix-metal-includes.sh`. The jit source lists themselves no longer need touching -- ml-explore#445 derives them from the vendored tree, and the derived sets match what a hand-bump would have produced here (48 Metal targets, 22 CUDA sources).
20b1dfa to
709dcd9
Compare
|
cmake builds will fail until mlx-c gets a tag |
|
OK, not making it today. Next step, debug CUDA build: claude says:
This is running |
|
Status: waiting for feedback from mlx folks. It may be a lifecycle issue: let deviceCtx = mlx_device_new_type(MLX_GPU, 0)
let streamCtx = mlx_stream_new_thread_unsafe(deviceCtx)
mlx_stream_free(streamCtx)
mlx_device_free(deviceCtx)is a minimal repro. The program crashes when the |
Proposed changes
Updates the vendored
mlxto v0.32.0 andmlx-cto fba4470 (its latest; those bindings were generated against 0.31.2, so 0.32.0's new ops aren't exposed yet).MLX_VERSIONis bumped inPackage.swiftandxcode/xcconfig/Cmlx.xcconfig.Adaptations the bump requires:
FFT gained a
normparameter. Everymlx_fft_*signature changed, so this adds a publicFFTNormenum (.backwarddefault, preserving current behaviour) and threads it throughSource/MLX/FFT.swift. Also wraps the newfftfreq/rfftfreq.SEEK_ENDin the in-memory IO stream was wrong — it computed from the read position rather than the end of the buffer. Latent until v0.32.0'sload_safetensorsadded a size check viaseek(0, end)+tell(), which madesaveToData/loadArraysround-trips fail.SaveTestscovers it.cpu/jit_compiler.cppnow shells out viastd::system(), which is unavailable on iOS. Routed throughmlx-conditionalthe same waycompiled.cppalready is, and excluded from the direct build so it isn't compiled twice.gemvis JIT compiled as of v0.32.0 (jit_kernels.cppgainedget_gemv_kernelusingmetal::gemv()), sogemv.metalno longer needs compiling into the metallib and comes out offix-metal-includes.sh. The jit source lists themselves need no change — Derive the jit-source lists in update-mlx.sh instead of hand-listing them #445 derives them, and the derived sets match what a hand bump would produce (48 Metal targets, 22 CUDA sources).Package.swiftexcludes and xcodeproj target membership updated for added/removed upstream files (jaccl moved underlib/,nccl_stubgone, newcuda/dirs.cppandqmmsources, newbackend/common/metal_kernel.{h,cpp}), plus regeneratedinclude/,include-framework/andmlx-generated/.New API now reachable from mlx-c fba4470:
ArrayAtIndicesso pure-sliceat[]updates usemlx_slice_update_add/prod/min/maxinstead of a scatter (mirroring upstream'smlx_add_item), falling back to scatter otherwiseexportToDot/graphDescription/NodeNamerin a newSource/MLX/GraphUtils.swiftCMakeLists.txtdeclaresmlxahead ofmlx-cso both build paths use the same mlx — mlx-c currently pins v0.31.2. That can be dropped once mlx-c pins 0.32.0 itself, e.g. via ml-explore/mlx-c#123.Per review, the
mlx.patchscaffolding and the localnew_thread_unsafe_streamshim were both removed in 2d27240 — see the two threads below.tools/update-mlx.sh,README.mdandMAINTENANCE.mdare byte-identical tomainagain.Known blockers
Two failures remain, both from upstream changes rather than from this change. Neither is fixed here.
The ordering abort.
Device::operator<andStream::operator<are not strict weak orderings, and 0.31.2 started running them on everyeval()viastd::set<Stream>ineval_impl. libc++ validates the comparator under the Xcode 27 SDK and aborts wherever cpu and gpu streams are both open, so the test run aborts rather than reporting failures. Reported as ml-explore/mlx#4083 and fixed by #4086, but that is onmainand unreleased — v0.32.0 is still the newest tag. This needs an mlx release containing #4086.Thread-affine streams. Streams became thread affine in 0.31.2 (#3348, #3281) and upstream treats it as intended (#3727 was closed pointing at a consumer-side fix in mlx-lm). Because
Stream.gpu/.cpuare cached process-wide,MLXRandomTests/testRandomThreadsDifferent,testRandomThreadsSameandTransformTests/testVmapThreadSafetyfail withThere is no Stream(gpu, 0) in current thread.They pass on 0.31.1. Making mlx-swift's default streams resolve per thread would fix them and needs no new API, but the stream model wants its own design rather than being settled inside a version bump, so it's deferred to follow-up work — see theStream.swiftthread. It doesn't block this bump.Verification
On macOS 27.0 (Apple Silicon), Xcode 27.0:
swift buildxcodebuild -scheme mlx-swift-Package -destination 'generic/platform=iOS'xcode/MLX.xcodeproj(-scheme MLX, macOS)cmake .. -G Ninja && ninja./tutorialrunspre-commit run --all-filesswift testscripts/verify-docs.shWith the patch still applied locally, the full suite was 554 tests / 0 failures, so the two blockers above are the only outstanding failures.
verify-docs.shfails inExtractAPI, which can't find any C++ stdlib header (algorithm,cmath,iostream) while parsing theCmlxtarget. This is not from this change: it fails identically with my new headers removed, and it failed the same way before the bump on unmodifiedmetal-cpp. It looks like Xcode 27 parsing C++ headers in C mode. I couldn't get a clean baseline run to prove it — SwiftPM rejected the manifest in a baseline worktree under the docc plugin — so worth confirming on CI's Xcode.Checklist
Put an
xin the boxes that apply.pre-commit run --all-filesto format my code / installed pre-commit prior to committing changesOn tests:
FFTTests(norm scaling per mode, round-trips,fftfreq/rfftfreq),GraphUtilsTests, slice-update cases inArrayAtTestsincluding a slice-vs-scatter agreement check, and aSaveTestsregression for theSEEK_ENDfix. On docs: the new public API carries DocC comments; no article changes are needed now that the stream work is out.