Skip to content

Commit 0cef6de

Browse files
SuryanshSS1011claudemetascroy
authored
[MLX] Isolate submodule build with ExternalProject (#20585)
### Summary Fixes #20556. Right now MLX gets pulled in with `add_subdirectory`, so MLX's entire CMake project lands in ExecuTorch's target/option namespace. This is a problem because MLX fetches `nlohmann_json`, which collides with the copy ExecuTorch already provides. The current workaround patches MLX's `CMakeLists.txt` at configure time to guard the fetch. That patch is brittle as it's pinned to specific lines, meaning an MLX bump that touches them makes it silently stop applying, and the collision comes back with no obvious signal. It also leaves the submodule dirty and lets MLX's `MLX_BUILD_*` options bleed into ExecuTorch's cache. This builds MLX as an `ExternalProject` in its own CMake scope and consumes it through an imported `mlx` target, so MLX runs its `FetchContent` in its own namespace and the patch is no longer needed. - The imported `mlx` target re-adds the Metal/Foundation/QuartzCore frameworks, since a static `libmlx.a` doesn't carry them. This mirrors the imported `mlx` target already in `tools/cmake/executorch-config.cmake`. `mlxdelegate` depends on `mlx_external` explicitly, because `add_dependencies` on an imported target doesn't order the build on its own. - `install(TARGETS mlx)` is changed to `install(FILES ${_mlx_static_lib})`; an imported target cannot be installed with `install(TARGETS)`. - The metallib install and `MLX_METALLIB_PATH` now point at the ExternalProject output. - Removed `backends/mlx/patches/mlx_json.patch` and the patch loop. The `BINARY_DIR` is kept where `add_subdirectory` put it, so `libmlx.a` and `mlx.metallib` land at the same paths as before. The package config, the metallib copy helper in `Utils.cmake`, and the wheel path in `setup.py` are unchanged. ### Test plan Verified on Apple Silicon (macOS, Metal toolchain installed): `cmake --preset mlx-release` configures, builds, and installs; `libmlx.a`, `libmlxdelegate.a`, and `mlx.metallib` are installed to `cmake-out/lib/`, and the MLX submodule stays clean (`git -C backends/mlx/third-party/mlx status`). With `-DEXECUTORCH_BUILD_TESTS=ON`, `op_test_runner`, `multi_thread_test_runner`, and `mlx_mutable_state_test` build and link `mlx`; `mlx_mutable_state_test` passes. I don't have the model checkpoints to run the qwen3_5_moe / gemma4_31b MLX runners or the wheel end-to-end, so I'm relying on CI for those. Since the artifact paths are unchanged, I'd expect them to behave the same as before. ### AI assistance disclosure I used an AI coding assistant for parts of this change, including drafting the CMake and investigating the build. I reviewed and tested everything in the test plan myself and take complete responsibility for the submission. --------- Co-authored-by: Claude Opus 4.8 (1M context) <noreply@anthropic.com> Co-authored-by: Scott Roy <scroy@meta.com>
1 parent 73c259e commit 0cef6de

3 files changed

Lines changed: 98 additions & 117 deletions

File tree

backends/mlx/CMakeLists.txt

Lines changed: 85 additions & 84 deletions
Original file line numberDiff line numberDiff line change
@@ -162,83 +162,78 @@ if(NOT _mlx_deployment_target_ok)
162162
)
163163
endif()
164164

165-
# MLX build options - we only need the C++ library with Metal
166-
set(MLX_BUILD_PYTHON_BINDINGS
167-
OFF
168-
CACHE BOOL "" FORCE
165+
# Build MLX in its own isolated CMake scope via ExternalProject, then consume it
166+
# as a prebuilt static lib through an imported target (see below). Building MLX
167+
# with add_subdirectory would drop its whole project into ExecuTorch's
168+
# target/option namespace, which collides with shared deps MLX fetches (e.g.
169+
# nlohmann_json) and leaks MLX's MLX_BUILD_* options into our cache. The
170+
# isolated scope runs MLX's FetchContent in its own namespace, so no collision
171+
# and no submodule patching are needed.
172+
include(ExternalProject)
173+
174+
set(_mlx_binary_dir ${CMAKE_CURRENT_BINARY_DIR}/mlx)
175+
set(_mlx_static_lib ${_mlx_binary_dir}/libmlx.a)
176+
# With MLX_METAL_JIT=ON, MLX does not install the metallib; it is produced in
177+
# the build tree at this path.
178+
set(_mlx_metallib ${_mlx_binary_dir}/mlx/backend/metal/kernels/mlx.metallib)
179+
180+
message(
181+
STATUS "Building MLX from submodule (ExternalProject): ${MLX_SOURCE_DIR}"
169182
)
170-
set(MLX_BUILD_TESTS
171-
OFF
172-
CACHE BOOL "" FORCE
183+
ExternalProject_Add(
184+
mlx_external
185+
SOURCE_DIR ${MLX_SOURCE_DIR}
186+
BINARY_DIR ${_mlx_binary_dir}
187+
CMAKE_ARGS -DCMAKE_BUILD_TYPE=${CMAKE_BUILD_TYPE}
188+
-DCMAKE_CXX_STANDARD=${CMAKE_CXX_STANDARD}
189+
-DCMAKE_OSX_DEPLOYMENT_TARGET=${CMAKE_OSX_DEPLOYMENT_TARGET}
190+
-DCMAKE_TOOLCHAIN_FILE=${CMAKE_TOOLCHAIN_FILE}
191+
-DPLATFORM=${PLATFORM}
192+
-DDEPLOYMENT_TARGET=${DEPLOYMENT_TARGET}
193+
# We only need the static Metal C++ library.
194+
-DMLX_BUILD_METAL=ON
195+
-DMLX_BUILD_CPU=OFF
196+
-DMLX_BUILD_CUDA=OFF
197+
-DMLX_BUILD_SHARED_LIBS=OFF
198+
-DMLX_BUILD_PYTHON_BINDINGS=OFF
199+
-DMLX_BUILD_PYTHON_STUBS=OFF
200+
-DMLX_BUILD_TESTS=OFF
201+
-DMLX_BUILD_EXAMPLES=OFF
202+
-DMLX_BUILD_BENCHMARKS=OFF
203+
-DMLX_BUILD_GGUF=OFF
204+
-DMLX_BUILD_SAFETENSORS=OFF
205+
-DMLX_METAL_JIT=ON
206+
# MLX's own install() does not emit libmlx.a where we consume it or the
207+
# metallib at all, so skip the install step and read both from the build tree.
208+
INSTALL_COMMAND ""
209+
# ExternalProject stamps its build, so a bare MLX submodule bump (git
210+
# submodule update) would not invalidate the stamp and we'd link a stale
211+
# libmlx.a with no signal. BUILD_ALWAYS reruns the build step every configure;
212+
# it is a fast no-op under Ninja when nothing changed.
213+
BUILD_ALWAYS ON
214+
# Required so Ninja knows these are produced by the external build.
215+
BUILD_BYPRODUCTS ${_mlx_static_lib} ${_mlx_metallib}
173216
)
174-
set(MLX_BUILD_EXAMPLES
175-
OFF
176-
CACHE BOOL "" FORCE
177-
)
178-
set(MLX_BUILD_BENCHMARKS
179-
OFF
180-
CACHE BOOL "" FORCE
181-
)
182-
set(MLX_BUILD_PYTHON_STUBS
183-
OFF
184-
CACHE BOOL "" FORCE
185-
)
186-
set(MLX_BUILD_CUDA
187-
OFF
188-
CACHE BOOL "" FORCE
189-
)
190-
set(MLX_BUILD_CPU
191-
OFF
192-
CACHE BOOL "" FORCE
193-
)
194-
set(MLX_BUILD_METAL
195-
ON
196-
CACHE BOOL "" FORCE
197-
)
198-
set(MLX_BUILD_SHARED_LIBS
199-
OFF
200-
CACHE BOOL "" FORCE
201-
)
202-
set(MLX_BUILD_GGUF
203-
OFF
204-
CACHE BOOL "" FORCE
205-
)
206-
set(MLX_BUILD_SAFETENSORS
207-
OFF
208-
CACHE BOOL "" FORCE
209-
)
210-
set(MLX_METAL_JIT
211-
ON
212-
CACHE BOOL "Use JIT compiled Metal kernels"
213-
)
214-
215-
# Auto-apply patches to MLX submodule. Each patch is applied idempotently: `git
216-
# apply --check` tests whether the patch is still applicable (i.e. not yet
217-
# applied), and only then applies it.
218-
set(_mlx_patches "${CMAKE_CURRENT_SOURCE_DIR}/patches/mlx_json.patch")
219-
foreach(_patch IN LISTS _mlx_patches)
220-
if(EXISTS "${_patch}" AND EXISTS "${MLX_SOURCE_DIR}")
221-
get_filename_component(_patch_name "${_patch}" NAME)
222-
execute_process(
223-
COMMAND git apply --check "${_patch}"
224-
WORKING_DIRECTORY ${MLX_SOURCE_DIR}
225-
RESULT_VARIABLE _patch_check
226-
OUTPUT_QUIET ERROR_QUIET
227-
)
228-
if(_patch_check EQUAL 0)
229-
execute_process(
230-
COMMAND git apply "${_patch}" WORKING_DIRECTORY ${MLX_SOURCE_DIR}
231-
)
232-
message(STATUS "Applied ${_patch_name} to MLX submodule")
233-
else()
234-
message(STATUS "${_patch_name} already applied or not applicable")
235-
endif()
236-
endif()
237-
endforeach()
238217

239-
# Add MLX subdirectory
240-
message(STATUS "Adding MLX from submodule: ${MLX_SOURCE_DIR}")
241-
add_subdirectory(${MLX_SOURCE_DIR} ${CMAKE_CURRENT_BINARY_DIR}/mlx)
218+
# Imported target for the MLX static library produced by mlx_external. A static
219+
# libmlx.a carries no transitive link deps, so re-add the frameworks MLX itself
220+
# links (mirrors third-party/mlx/CMakeLists.txt:209). CPU is OFF, so Accelerate
221+
# is not needed.
222+
add_library(mlx STATIC IMPORTED GLOBAL)
223+
set_target_properties(mlx PROPERTIES IMPORTED_LOCATION ${_mlx_static_lib})
224+
# Headers come from the submodule source tree (always present), not the build
225+
# dir, so mlxdelegate compilation cannot race the external build. Verified: MLX
226+
# emits no public headers into the build tree (version.h is checked in; the
227+
# version string is a compile-def on version.cpp), so the source dir suffices.
228+
target_include_directories(mlx INTERFACE ${MLX_SOURCE_DIR})
229+
find_library(METAL_FRAMEWORK Metal)
230+
find_library(FOUNDATION_FRAMEWORK Foundation)
231+
find_library(QUARTZ_FRAMEWORK QuartzCore)
232+
set_target_properties(
233+
mlx
234+
PROPERTIES INTERFACE_LINK_LIBRARIES
235+
"${METAL_FRAMEWORK};${FOUNDATION_FRAMEWORK};${QUARTZ_FRAMEWORK}"
236+
)
242237

243238
# -----------------------------------------------------------------------------
244239
# MLX Backend library
@@ -266,6 +261,10 @@ add_library(mlxdelegate ${_mlx_backend__srcs})
266261
# Ensure schema is generated before compiling
267262
add_dependencies(mlxdelegate mlx_schema)
268263

264+
# mlx is an imported target, so a dependency on it would not order the build.
265+
# Depend on mlx_external directly so libmlx.a exists before mlxdelegate links.
266+
add_dependencies(mlxdelegate mlx_external)
267+
269268
# Add logging flag if enabled
270269
if(ET_MLX_ENABLE_OP_LOGGING)
271270
target_compile_definitions(mlxdelegate PRIVATE ET_MLX_ENABLE_OP_LOGGING=1)
@@ -283,8 +282,9 @@ target_include_directories(
283282
mlxdelegate PRIVATE ${CMAKE_CURRENT_SOURCE_DIR}/runtime
284283
)
285284

286-
# Link against MLX and executorch mlx is only available at BUILD_INTERFACE -
287-
# consumers must link to mlx separately
285+
# Link against MLX and executorch. mlx is an imported static lib (built by
286+
# mlx_external); it is only consumed at build time here, not re-exported, so
287+
# downstream consumers must link to mlx separately via the package config.
288288
target_link_libraries(
289289
mlxdelegate PRIVATE mlx_schema executorch_core $<BUILD_INTERFACE:mlx>
290290
)
@@ -301,8 +301,10 @@ install(
301301
DESTINATION ${CMAKE_INSTALL_LIBDIR}
302302
)
303303

304-
# Install mlx library for downstream consumers
305-
install(TARGETS mlx DESTINATION ${CMAKE_INSTALL_LIBDIR})
304+
# Install mlx library for downstream consumers. mlx is an imported target (built
305+
# by mlx_external), so install the static lib file directly rather than via
306+
# install(TARGETS ...), which only accepts built targets.
307+
install(FILES ${_mlx_static_lib} DESTINATION ${CMAKE_INSTALL_LIBDIR})
306308

307309
# Install mlx headers for downstream consumers that may need mlx types
308310
install(
@@ -325,16 +327,15 @@ install(
325327
# containing MLX code. When MLX is statically linked into _portable_lib.so, this
326328
# is the directory containing _portable_lib.so.
327329
#
328-
# For the installed library, we put metallib in lib/ alongside libmlx.a
329-
install(
330-
FILES ${CMAKE_CURRENT_BINARY_DIR}/mlx/mlx/backend/metal/kernels/mlx.metallib
331-
DESTINATION ${CMAKE_INSTALL_LIBDIR}
332-
)
330+
# For the installed library, we put metallib in lib/ alongside libmlx.a. The
331+
# metallib is produced in the mlx_external build tree (MLX_METAL_JIT=ON does not
332+
# install it); _mlx_metallib points there.
333+
install(FILES ${_mlx_metallib} DESTINATION ${CMAKE_INSTALL_LIBDIR})
333334

334335
# Cache the metallib path for pybindings to copy it next to _portable_lib.so
335336
# This enables editable installs to work correctly
336337
set(MLX_METALLIB_PATH
337-
"${CMAKE_CURRENT_BINARY_DIR}/mlx/mlx/backend/metal/kernels/mlx.metallib"
338+
"${_mlx_metallib}"
338339
CACHE INTERNAL "Path to mlx.metallib for pybindings"
339340
)
340341

backends/mlx/patches/mlx_json.patch

Lines changed: 0 additions & 29 deletions
This file was deleted.

tools/cmake/executorch-config.cmake

Lines changed: 13 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -134,14 +134,23 @@ if(TARGET mlxdelegate)
134134
if(_mlx_library)
135135
add_library(mlx STATIC IMPORTED)
136136
set_target_properties(mlx PROPERTIES IMPORTED_LOCATION "${_mlx_library}")
137-
# MLX requires Metal and Foundation frameworks on Apple platforms
137+
# libmlx.a is a static archive with no transitive link deps, so re-add the
138+
# frameworks MLX links PUBLIC (mirrors
139+
# third-party/mlx/CMakeLists.txt:209). Must match the in-tree imported
140+
# target in backends/mlx/CMakeLists.txt.
138141
if(APPLE)
139142
find_library(METAL_FRAMEWORK Metal)
140143
find_library(FOUNDATION_FRAMEWORK Foundation)
141-
if(METAL_FRAMEWORK AND FOUNDATION_FRAMEWORK)
144+
find_library(QUARTZ_FRAMEWORK QuartzCore)
145+
if(METAL_FRAMEWORK
146+
AND FOUNDATION_FRAMEWORK
147+
AND QUARTZ_FRAMEWORK
148+
)
142149
set_target_properties(
143-
mlx PROPERTIES INTERFACE_LINK_LIBRARIES
144-
"${METAL_FRAMEWORK};${FOUNDATION_FRAMEWORK}"
150+
mlx
151+
PROPERTIES
152+
INTERFACE_LINK_LIBRARIES
153+
"${METAL_FRAMEWORK};${FOUNDATION_FRAMEWORK};${QUARTZ_FRAMEWORK}"
145154
)
146155
endif()
147156
endif()

0 commit comments

Comments
 (0)