From 351ee54ec9e6a51048fcdf52e859ac6f1a980510 Mon Sep 17 00:00:00 2001 From: Suryansh Sijwali Date: Sun, 28 Jun 2026 14:08:24 -0400 Subject: [PATCH 1/2] [MLX] Isolate submodule build with ExternalProject MLX was pulled into ExecuTorch's build via add_subdirectory, dropping MLX's whole CMake project into ET's target/option namespace. That shared scope collides with deps MLX fetches (today nlohmann_json), worked around by patching MLX's own CMakeLists.txt at configure time. Patching a submodule is fragile: the patch is pinned to specific lines and silently stops applying when an MLX bump touches that region, at which point the json collision returns with no clear signal. It also dirties the submodule and leaks MLX's MLX_BUILD_* options into ET's cache. Build MLX in its own isolated CMake scope via ExternalProject and consume it as a prebuilt static lib + metallib through an imported `mlx` target. MLX then runs its FetchContent in its own namespace, so the json collision cannot happen and the patch is deleted. The imported target re-adds the Metal/Foundation/ QuartzCore frameworks a static libmlx.a does not carry (matching the imported mlx target already in tools/cmake/executorch-config.cmake), and mlxdelegate depends on mlx_external directly since add_dependencies on an imported target does not order the build. The ExternalProject BINARY_DIR is kept at the same location add_subdirectory used, so libmlx.a and mlx.metallib land at their existing paths; downstream consumers (package config, metallib copy helper, the pybindings wheel in setup.py) need no changes. - Replace MLX_BUILD_* options + patch loop + add_subdirectory with ExternalProject_Add(mlx_external) and an imported `mlx` target. - install(TARGETS mlx) -> install(FILES ${_mlx_static_lib}), since an imported target cannot be installed via install(TARGETS). - Point the metallib install and MLX_METALLIB_PATH at ${_mlx_metallib}. - Delete backends/mlx/patches/mlx_json.patch and the patch-apply loop. Verified: cmake --preset mlx-release configures, builds, and installs; libmlx.a, libmlxdelegate.a, and mlx.metallib land in cmake-out/lib/; the MLX submodule stays pristine through configure/build/install; the delegate tests (op_test_runner, multi_thread_test_runner, mlx_mutable_state_test) build and link mlx directly, and mlx_mutable_state_test passes. Fixes #20556. Co-Authored-By: Claude Opus 4.8 (1M context) --- backends/mlx/CMakeLists.txt | 164 ++++++++++++++-------------- backends/mlx/patches/mlx_json.patch | 29 ----- 2 files changed, 80 insertions(+), 113 deletions(-) delete mode 100644 backends/mlx/patches/mlx_json.patch diff --git a/backends/mlx/CMakeLists.txt b/backends/mlx/CMakeLists.txt index acb96fb1ed9..fad729ecbc3 100644 --- a/backends/mlx/CMakeLists.txt +++ b/backends/mlx/CMakeLists.txt @@ -162,83 +162,72 @@ if(NOT _mlx_deployment_target_ok) ) endif() -# MLX build options - we only need the C++ library with Metal -set(MLX_BUILD_PYTHON_BINDINGS - OFF - CACHE BOOL "" FORCE +# Build MLX in its own isolated CMake scope via ExternalProject, then consume it +# as a prebuilt static lib through an imported target (see below). Building MLX +# with add_subdirectory would drop its whole project into ExecuTorch's +# target/option namespace, which collides with shared deps MLX fetches (e.g. +# nlohmann_json) and leaks MLX's MLX_BUILD_* options into our cache. The +# isolated scope runs MLX's FetchContent in its own namespace, so no collision +# and no submodule patching are needed. +include(ExternalProject) + +set(_mlx_binary_dir ${CMAKE_CURRENT_BINARY_DIR}/mlx) +set(_mlx_static_lib ${_mlx_binary_dir}/libmlx.a) +# With MLX_METAL_JIT=ON, MLX does not install the metallib; it is produced in +# the build tree at this path. +set(_mlx_metallib ${_mlx_binary_dir}/mlx/backend/metal/kernels/mlx.metallib) + +message( + STATUS "Building MLX from submodule (ExternalProject): ${MLX_SOURCE_DIR}" ) -set(MLX_BUILD_TESTS - OFF - CACHE BOOL "" FORCE +ExternalProject_Add( + mlx_external + SOURCE_DIR ${MLX_SOURCE_DIR} + BINARY_DIR ${_mlx_binary_dir} + CMAKE_ARGS -DCMAKE_BUILD_TYPE=${CMAKE_BUILD_TYPE} + -DCMAKE_CXX_STANDARD=${CMAKE_CXX_STANDARD} + -DCMAKE_OSX_DEPLOYMENT_TARGET=${CMAKE_OSX_DEPLOYMENT_TARGET} + -DCMAKE_TOOLCHAIN_FILE=${CMAKE_TOOLCHAIN_FILE} + -DPLATFORM=${PLATFORM} + -DDEPLOYMENT_TARGET=${DEPLOYMENT_TARGET} + # We only need the static Metal C++ library. + -DMLX_BUILD_METAL=ON + -DMLX_BUILD_CPU=OFF + -DMLX_BUILD_CUDA=OFF + -DMLX_BUILD_SHARED_LIBS=OFF + -DMLX_BUILD_PYTHON_BINDINGS=OFF + -DMLX_BUILD_PYTHON_STUBS=OFF + -DMLX_BUILD_TESTS=OFF + -DMLX_BUILD_EXAMPLES=OFF + -DMLX_BUILD_BENCHMARKS=OFF + -DMLX_BUILD_GGUF=OFF + -DMLX_BUILD_SAFETENSORS=OFF + -DMLX_METAL_JIT=ON + # MLX's own install() does not emit libmlx.a where we consume it or the + # metallib at all, so skip the install step and read both from the build tree. + INSTALL_COMMAND "" + # Required so Ninja knows these are produced by the external build. + BUILD_BYPRODUCTS ${_mlx_static_lib} ${_mlx_metallib} ) -set(MLX_BUILD_EXAMPLES - OFF - CACHE BOOL "" FORCE -) -set(MLX_BUILD_BENCHMARKS - OFF - CACHE BOOL "" FORCE -) -set(MLX_BUILD_PYTHON_STUBS - OFF - CACHE BOOL "" FORCE -) -set(MLX_BUILD_CUDA - OFF - CACHE BOOL "" FORCE -) -set(MLX_BUILD_CPU - OFF - CACHE BOOL "" FORCE -) -set(MLX_BUILD_METAL - ON - CACHE BOOL "" FORCE -) -set(MLX_BUILD_SHARED_LIBS - OFF - CACHE BOOL "" FORCE -) -set(MLX_BUILD_GGUF - OFF - CACHE BOOL "" FORCE -) -set(MLX_BUILD_SAFETENSORS - OFF - CACHE BOOL "" FORCE -) -set(MLX_METAL_JIT - ON - CACHE BOOL "Use JIT compiled Metal kernels" -) - -# Auto-apply patches to MLX submodule. Each patch is applied idempotently: `git -# apply --check` tests whether the patch is still applicable (i.e. not yet -# applied), and only then applies it. -set(_mlx_patches "${CMAKE_CURRENT_SOURCE_DIR}/patches/mlx_json.patch") -foreach(_patch IN LISTS _mlx_patches) - if(EXISTS "${_patch}" AND EXISTS "${MLX_SOURCE_DIR}") - get_filename_component(_patch_name "${_patch}" NAME) - execute_process( - COMMAND git apply --check "${_patch}" - WORKING_DIRECTORY ${MLX_SOURCE_DIR} - RESULT_VARIABLE _patch_check - OUTPUT_QUIET ERROR_QUIET - ) - if(_patch_check EQUAL 0) - execute_process( - COMMAND git apply "${_patch}" WORKING_DIRECTORY ${MLX_SOURCE_DIR} - ) - message(STATUS "Applied ${_patch_name} to MLX submodule") - else() - message(STATUS "${_patch_name} already applied or not applicable") - endif() - endif() -endforeach() -# Add MLX subdirectory -message(STATUS "Adding MLX from submodule: ${MLX_SOURCE_DIR}") -add_subdirectory(${MLX_SOURCE_DIR} ${CMAKE_CURRENT_BINARY_DIR}/mlx) +# Imported target for the MLX static library produced by mlx_external. A static +# libmlx.a carries no transitive link deps, so re-add the frameworks MLX itself +# links (mirrors third-party/mlx/CMakeLists.txt:209). CPU is OFF, so Accelerate +# is not needed. +add_library(mlx STATIC IMPORTED GLOBAL) +set_target_properties(mlx PROPERTIES IMPORTED_LOCATION ${_mlx_static_lib}) +# Headers come from the submodule source tree (always present), not the build +# dir, so mlxdelegate compilation cannot race the external build. +target_include_directories(mlx INTERFACE ${MLX_SOURCE_DIR}) +find_library(METAL_FRAMEWORK Metal) +find_library(FOUNDATION_FRAMEWORK Foundation) +find_library(QUARTZ_FRAMEWORK QuartzCore) +set_target_properties( + mlx + PROPERTIES INTERFACE_LINK_LIBRARIES + "${METAL_FRAMEWORK};${FOUNDATION_FRAMEWORK};${QUARTZ_FRAMEWORK}" +) +add_dependencies(mlx mlx_external) # ----------------------------------------------------------------------------- # MLX Backend library @@ -266,6 +255,11 @@ add_library(mlxdelegate ${_mlx_backend__srcs}) # Ensure schema is generated before compiling add_dependencies(mlxdelegate mlx_schema) +# mlx is an imported target, so add_dependencies(mlx mlx_external) above does +# not order the build. Depend on mlx_external directly so libmlx.a exists before +# mlxdelegate links. +add_dependencies(mlxdelegate mlx_external) + # Add logging flag if enabled if(ET_MLX_ENABLE_OP_LOGGING) target_compile_definitions(mlxdelegate PRIVATE ET_MLX_ENABLE_OP_LOGGING=1) @@ -283,8 +277,9 @@ target_include_directories( mlxdelegate PRIVATE ${CMAKE_CURRENT_SOURCE_DIR}/runtime ) -# Link against MLX and executorch mlx is only available at BUILD_INTERFACE - -# consumers must link to mlx separately +# Link against MLX and executorch. mlx is an imported static lib (built by +# mlx_external); it is only consumed at build time here, not re-exported, so +# downstream consumers must link to mlx separately via the package config. target_link_libraries( mlxdelegate PRIVATE mlx_schema executorch_core $ ) @@ -301,8 +296,10 @@ install( DESTINATION ${CMAKE_INSTALL_LIBDIR} ) -# Install mlx library for downstream consumers -install(TARGETS mlx DESTINATION ${CMAKE_INSTALL_LIBDIR}) +# Install mlx library for downstream consumers. mlx is an imported target (built +# by mlx_external), so install the static lib file directly rather than via +# install(TARGETS ...), which only accepts built targets. +install(FILES ${_mlx_static_lib} DESTINATION ${CMAKE_INSTALL_LIBDIR}) # Install mlx headers for downstream consumers that may need mlx types install( @@ -325,16 +322,15 @@ install( # containing MLX code. When MLX is statically linked into _portable_lib.so, this # is the directory containing _portable_lib.so. # -# For the installed library, we put metallib in lib/ alongside libmlx.a -install( - FILES ${CMAKE_CURRENT_BINARY_DIR}/mlx/mlx/backend/metal/kernels/mlx.metallib - DESTINATION ${CMAKE_INSTALL_LIBDIR} -) +# For the installed library, we put metallib in lib/ alongside libmlx.a. The +# metallib is produced in the mlx_external build tree (MLX_METAL_JIT=ON does not +# install it); _mlx_metallib points there. +install(FILES ${_mlx_metallib} DESTINATION ${CMAKE_INSTALL_LIBDIR}) # Cache the metallib path for pybindings to copy it next to _portable_lib.so # This enables editable installs to work correctly set(MLX_METALLIB_PATH - "${CMAKE_CURRENT_BINARY_DIR}/mlx/mlx/backend/metal/kernels/mlx.metallib" + "${_mlx_metallib}" CACHE INTERNAL "Path to mlx.metallib for pybindings" ) diff --git a/backends/mlx/patches/mlx_json.patch b/backends/mlx/patches/mlx_json.patch deleted file mode 100644 index 4760403c8e6..00000000000 --- a/backends/mlx/patches/mlx_json.patch +++ /dev/null @@ -1,29 +0,0 @@ -diff --git a/CMakeLists.txt b/CMakeLists.txt ---- a/CMakeLists.txt -+++ b/CMakeLists.txt -@@ -304,12 +304,18 @@ else() - set(MLX_BUILD_ACCELERATE OFF) - endif() - --message(STATUS "Downloading json") --FetchContent_Declare( -- json -- URL https://github.com/nlohmann/json/releases/download/v3.11.3/json.tar.xz) --FetchContent_MakeAvailable(json) --target_include_directories( -- mlx PRIVATE $) -+# Only fetch json if nlohmann_json target doesn't already exist -+# (ExecuTorch provides its own copy) -+if(NOT TARGET nlohmann_json) -+ message(STATUS "Downloading json") -+ FetchContent_Declare( -+ json -+ URL https://github.com/nlohmann/json/releases/download/v3.11.3/json.tar.xz) -+ FetchContent_MakeAvailable(json) -+ target_include_directories( -+ mlx PRIVATE $) -+else() -+ message(STATUS "Using existing nlohmann_json target") -+endif() - - add_subdirectory(${CMAKE_CURRENT_LIST_DIR}/mlx) From 84d2a1d75ed78b4c4c94dc5320776e6591de9831 Mon Sep 17 00:00:00 2001 From: Scott Roy Date: Mon, 29 Jun 2026 12:13:45 -0700 Subject: [PATCH 2/2] nits --- backends/mlx/CMakeLists.txt | 15 ++++++++++----- tools/cmake/executorch-config.cmake | 17 +++++++++++++---- 2 files changed, 23 insertions(+), 9 deletions(-) diff --git a/backends/mlx/CMakeLists.txt b/backends/mlx/CMakeLists.txt index fad729ecbc3..15db7ca0cb0 100644 --- a/backends/mlx/CMakeLists.txt +++ b/backends/mlx/CMakeLists.txt @@ -206,6 +206,11 @@ ExternalProject_Add( # MLX's own install() does not emit libmlx.a where we consume it or the # metallib at all, so skip the install step and read both from the build tree. INSTALL_COMMAND "" + # ExternalProject stamps its build, so a bare MLX submodule bump (git + # submodule update) would not invalidate the stamp and we'd link a stale + # libmlx.a with no signal. BUILD_ALWAYS reruns the build step every configure; + # it is a fast no-op under Ninja when nothing changed. + BUILD_ALWAYS ON # Required so Ninja knows these are produced by the external build. BUILD_BYPRODUCTS ${_mlx_static_lib} ${_mlx_metallib} ) @@ -217,7 +222,9 @@ ExternalProject_Add( add_library(mlx STATIC IMPORTED GLOBAL) set_target_properties(mlx PROPERTIES IMPORTED_LOCATION ${_mlx_static_lib}) # Headers come from the submodule source tree (always present), not the build -# dir, so mlxdelegate compilation cannot race the external build. +# dir, so mlxdelegate compilation cannot race the external build. Verified: MLX +# emits no public headers into the build tree (version.h is checked in; the +# version string is a compile-def on version.cpp), so the source dir suffices. target_include_directories(mlx INTERFACE ${MLX_SOURCE_DIR}) find_library(METAL_FRAMEWORK Metal) find_library(FOUNDATION_FRAMEWORK Foundation) @@ -227,7 +234,6 @@ set_target_properties( PROPERTIES INTERFACE_LINK_LIBRARIES "${METAL_FRAMEWORK};${FOUNDATION_FRAMEWORK};${QUARTZ_FRAMEWORK}" ) -add_dependencies(mlx mlx_external) # ----------------------------------------------------------------------------- # MLX Backend library @@ -255,9 +261,8 @@ add_library(mlxdelegate ${_mlx_backend__srcs}) # Ensure schema is generated before compiling add_dependencies(mlxdelegate mlx_schema) -# mlx is an imported target, so add_dependencies(mlx mlx_external) above does -# not order the build. Depend on mlx_external directly so libmlx.a exists before -# mlxdelegate links. +# mlx is an imported target, so a dependency on it would not order the build. +# Depend on mlx_external directly so libmlx.a exists before mlxdelegate links. add_dependencies(mlxdelegate mlx_external) # Add logging flag if enabled diff --git a/tools/cmake/executorch-config.cmake b/tools/cmake/executorch-config.cmake index 524e1be36ec..946436e3083 100644 --- a/tools/cmake/executorch-config.cmake +++ b/tools/cmake/executorch-config.cmake @@ -134,14 +134,23 @@ if(TARGET mlxdelegate) if(_mlx_library) add_library(mlx STATIC IMPORTED) set_target_properties(mlx PROPERTIES IMPORTED_LOCATION "${_mlx_library}") - # MLX requires Metal and Foundation frameworks on Apple platforms + # libmlx.a is a static archive with no transitive link deps, so re-add the + # frameworks MLX links PUBLIC (mirrors + # third-party/mlx/CMakeLists.txt:209). Must match the in-tree imported + # target in backends/mlx/CMakeLists.txt. if(APPLE) find_library(METAL_FRAMEWORK Metal) find_library(FOUNDATION_FRAMEWORK Foundation) - if(METAL_FRAMEWORK AND FOUNDATION_FRAMEWORK) + find_library(QUARTZ_FRAMEWORK QuartzCore) + if(METAL_FRAMEWORK + AND FOUNDATION_FRAMEWORK + AND QUARTZ_FRAMEWORK + ) set_target_properties( - mlx PROPERTIES INTERFACE_LINK_LIBRARIES - "${METAL_FRAMEWORK};${FOUNDATION_FRAMEWORK}" + mlx + PROPERTIES + INTERFACE_LINK_LIBRARIES + "${METAL_FRAMEWORK};${FOUNDATION_FRAMEWORK};${QUARTZ_FRAMEWORK}" ) endif() endif()