Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[MLX] Updated to 0.21.0 #9898

Merged
merged 7 commits into from
Dec 10, 2024
Merged

Conversation

stemann
Copy link
Contributor

@stemann stemann commented Nov 30, 2024

Summary:

  • Updated to v0.21.0.
  • Applied patches merged upstream (after v0.21.0 was tagged).
  • Only remaining patch is for io on win32 (which is a hack not to be upstreamed).
  • Enabled FreeBSD platforms - using OpenBLAS on aarch64.

@stemann stemann force-pushed the feature/mlx_0.21 branch 2 times, most recently from da76fdd to 1b15396 Compare December 3, 2024 22:32
@stemann
Copy link
Contributor Author

stemann commented Dec 3, 2024

When building MLX C (#9809), it was necessary to also have MacOSX14.0.sdk present in /workspace/srcdir/MacOSX14.0.sdk - likely due to the produced CMake config.

I am wondering whether the produced cmake config should be patched... but not sure what would be right.

The pre-built aarch64-apple-darwin PyPI wheel content of $prefix/share/cmake differs from the built x86_64-apple-darwin content of $prefix/share/cmake, notably wrt. INTERFACE_LINK_LIBRARIES:

$ diff -ru aarch64/share x86_64/share
diff -ru aarch64/share/cmake/MLX/MLXConfig.cmake x86_64/share/cmake/MLX/MLXConfig.cmake
--- aarch64/share/cmake/MLX/MLXConfig.cmake     1970-01-01 01:00:00
+++ x86_64/share/cmake/MLX/MLXConfig.cmake      1970-01-01 01:00:00
@@ -39,14 +39,14 @@
     set(MLX_CXX_FLAGS ${MLX_CXX_FLAGS} -DACCELERATE_NEW_LAPACK)
 endif()
 
-if (ON)
-    set(MLX_BUILD_METAL ON)
+if (OFF)
+    set(MLX_BUILD_METAL OFF)
     set(MLX_CXX_FLAGS ${MLX_CXX_FLAGS} -D_METAL_)
     set(MLX_INCLUDE_DIRS 
         "${MLX_INCLUDE_DIRS};"
         ${PACKAGE_PREFIX_DIR}/include/metal_cpp
     )
-    if(300 GREATER_EQUAL 310)
+    if( GREATER_EQUAL 310)
       set(MLX_INCLUDE_DIRS
         "${MLX_INCLUDE_DIRS};"
         ${PACKAGE_PREFIX_DIR}/include/mlx/backend/metal/kernels/metal_3_1)
diff -ru aarch64/share/cmake/MLX/MLXTargets.cmake x86_64/share/cmake/MLX/MLXTargets.cmake
--- aarch64/share/cmake/MLX/MLXTargets.cmake    1970-01-01 01:00:00
+++ x86_64/share/cmake/MLX/MLXTargets.cmake     1970-01-01 01:00:00
@@ -1,13 +1,13 @@
 # Generated by CMake
 
 if("${CMAKE_MAJOR_VERSION}.${CMAKE_MINOR_VERSION}" LESS 2.8)
-   message(FATAL_ERROR "CMake >= 2.8.12 required")
+   message(FATAL_ERROR "CMake >= 2.8.0 required")
 endif()
 if(CMAKE_VERSION VERSION_LESS "2.8.12")
    message(FATAL_ERROR "CMake >= 2.8.12 required")
 endif()
 cmake_policy(PUSH)
-cmake_policy(VERSION 2.8.12...3.29)
+cmake_policy(VERSION 2.8.12...3.28)
 #----------------------------------------------------------------
 # Generated CMake target import file.
 #----------------------------------------------------------------
@@ -59,8 +59,8 @@
 add_library(mlx SHARED IMPORTED)
 
 set_target_properties(mlx PROPERTIES
-  INTERFACE_INCLUDE_DIRECTORIES "${_IMPORT_PREFIX}/include/metal_cpp;${_IMPORT_PREFIX}/include;${_IMPORT_PREFIX}/include"
-  INTERFACE_LINK_LIBRARIES "-framework Metal;-framework Foundation;-framework QuartzCore;/Applications/Xcode-15.0.1.app/Contents/Developer/Platforms/MacOSX.platform/Developer/SDKs/MacOSX14.0.sdk/System/Library/Frameworks/Accelerate.framework"
+  INTERFACE_INCLUDE_DIRECTORIES "${_IMPORT_PREFIX}/include;${_IMPORT_PREFIX}/include"
+  INTERFACE_LINK_LIBRARIES "/workspace/srcdir/MacOSX14.0.sdk/System/Library/Frameworks/Accelerate.framework"
 )
 
 # Load information for each installed configuration.

@stemann stemann marked this pull request as ready for review December 3, 2024 23:01
git remote add upstream https://github.com/ml-explore/mlx.git
git fetch upstream
git checkout -B patches
git cherry-pick 698e63a608bbee43bd70a76602dbf2bd6636877f # CMake: Build with dlfcn-win32 to have dlopen etc. on win32 (#1628)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I haven't seen this pattern before – using the build script to download more code. Usually the recipes put patches into a bundled/patches directory.

Copy link
Contributor Author

@stemann stemann Dec 5, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nope, it’s new :-)

It’s just that these patches were already upstreamed, so it seems cleaner to just get them from the source, but we don’t want a newer commit than v0.21 as other (possibly breaking) changes has also been merged.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why not adding a patch (which are also cached, contrary to what you're doing here)? That looks much cleaner than doing all this needless dance.

Copy link
Contributor Author

@stemann stemann Dec 7, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I can re-add the patches, though they will be gone by v0.21.1 (just released).

… they should be in this branch already - but not at the latest commit.

I would still like to build v0.21.0 as this is the version used by MLX C v0.1.0.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I can re-add the patches, though they will be gone by v0.21.1 (just released).

That's just normal: we backport patches all the time. Fiddling with the git repo looks more messy to me.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I beg to differ, but I've restored the patches now :-)

@stemann
Copy link
Contributor Author

stemann commented Dec 7, 2024

@giordano Any thoughts on the cmake config comment: #9898 (comment)

* Reverted "Using proper v0.21.0 - cherry-picking merged upstream patches"
* Reverted "Using commit without need for patches"
* Removed cmake-win32-io patch from cmake-w64-mingw32 patch
* Made upstreamed patches unconditional
@stemann stemann requested a review from giordano December 10, 2024 10:12
@giordano giordano merged commit 50e91e9 into JuliaPackaging:master Dec 10, 2024
33 checks passed
@stemann stemann deleted the feature/mlx_0.21 branch December 11, 2024 07:19
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants