ml-explore / mlx

MLX: An array framework for Apple silicon
https://ml-explore.github.io/mlx/
MIT License
17.21k stars 995 forks source link

Building a conda(-forge) package for this. #14

Closed xhochy closed 10 months ago

xhochy commented 11 months ago

It would also be nice to have a binary for this on conda-forge. You can see my start at implementing a recipe for this at https://github.com/conda-forge/staged-recipes/pull/24687/files As the CI over there doesn't build for Apple silicon, we don't see any failure. Thus I would use this issue to reach out for help with build issues.

xhochy commented 11 months ago

It currently fails with

  [2/74] Building arg_reduce.air
  FAILED: mlx/backend/metal/kernels/arg_reduce.air /Users/uwe/mambaforge/conda-bld/mlx_1701855483044/work/build/temp.macosx-11.0-arm64-cpython-39/mlx.core/mlx/backend/metal/kernels/arg_reduce.air
  cd /Users/uwe/mambaforge/conda-bld/mlx_1701855483044/work/build/temp.macosx-11.0-arm64-cpython-39/mlx.core/mlx/backend/metal/kernels && xcrun -sdk macosx metal -Wall -Wextra -fno-fast-math -c /Users/uwe/mambaforge/conda-bld/mlx_1701855483044/work/mlx/backend/metal/kernels/arg_reduce.metal -I/Users/uwe/mambaforge/conda-bld/mlx_1701855483044/work -o arg_reduce.air
  In file included from /Users/uwe/mambaforge/conda-bld/mlx_1701855483044/work/mlx/backend/metal/kernels/arg_reduce.metal:6:
  /Users/uwe/mambaforge/conda-bld/mlx_1701855483044/work/mlx/backend/metal/kernels/utils.h:15:27: error: variable in constant address space must be initialized
    static const constant U max;
                            ^
  /Users/uwe/mambaforge/conda-bld/mlx_1701855483044/work/mlx/backend/metal/kernels/utils.h:16:27: error: variable in constant address space must be initialized
    static const constant U min;
                            ^
  /Users/uwe/mambaforge/conda-bld/mlx_1701855483044/work/mlx/backend/metal/kernels/utils.h:17:27: error: variable in constant address space must be initialized
    static const constant U finite_max;
                            ^
  /Users/uwe/mambaforge/conda-bld/mlx_1701855483044/work/mlx/backend/metal/kernels/utils.h:18:27: error: variable in constant address space must be initialized
    static const constant U finite_min;

Full build log at https://gist.github.com/xhochy/3290e73eba3fce442f4ba453fe8def60

As I have no experience with metal, I'm unsure about the right fix for this.

jagrit06 commented 11 months ago

Looking at your build log, it might be the case that you on a MacOS version that is not supported We only support MacOS >= 13.4 - and have updated the docs with clearer instructions on that Also, we have updated the cmake to give a clearer indication of the macOS version being used for the build and throw a clear error when that is incompatible

xhochy commented 11 months ago

CMake correctly reports -- Detected macOS version 14.0 but this actually helped as I searched the log on why you might think that I have an older macOS and found that it still set the macos deployment target to 11.0. Setting it to 14.0 fixes the build.

xhochy commented 11 months ago

The build passes but sadly the import fails with

libc++abi: terminating due to uncaught exception of type std::runtime_error: Failed to load device

The issue is that MTLCreateSystemDefaultDevice doesn't return a default device. The following gives an indication how to fix it but adding -framework CoreGraphics to LDFLAGS had no impact.

In macOS, in order for the system to provide a default Metal device object, you must link to the Core Graphics framework. You usually need to do this explicitly if you’re writing apps that don’t use graphics by default, such as command line tools.

xhochy commented 11 months ago

I have a PR ready that builds it for Apple Silicon on conda-forge: https://github.com/conda-forge/mlx-feedstock/pull/3 Sadly this fails in cross-compilation mode. As we don't have Apple Silicon runners on conda-forge, this makes it hard to build. Locally (without cross-compilation), it passes.

awni commented 11 months ago

Cool! Would love to support install with Conda. Could you say more about how the distribution works? Right now we build w/ cross compilation for PyPI. Could we reuse that same machinery for Conda forge?

xhochy commented 11 months ago

It works similar to the PyPI machinery, but the main difference is that conda-forge brings its own compiler infrastructure. With Metal, this should be too much of a difference as it simply calls out into the SDK.

The current problem is though that compilation stops with:

2023-12-20T09:24:46.5479330Z   [4/74] Building binary.air
2023-12-20T09:24:46.5600500Z   FAILED: mlx/backend/metal/kernels/binary.air /Users/runner/miniforge3/conda-bld/mlx_1703063827515/work/build/temp.macosx-11.0-arm64-cpython-311/mlx.core/mlx/backend/metal/kernels/binary.air
2023-12-20T09:24:46.5700970Z   cd /Users/runner/miniforge3/conda-bld/mlx_1703063827515/work/build/temp.macosx-11.0-arm64-cpython-311/mlx.core/mlx/backend/metal/kernels && xcrun -sdk macosx metal -Wall -Wextra -fno-fast-math -c /Users/runner/miniforge3/conda-bld/mlx_1703063827515/work/mlx/backend/metal/kernels/binary.metal -I/Users/runner/miniforge3/conda-bld/mlx_1703063827515/work -o binary.air
2023-12-20T09:24:46.5801130Z   In file included from /Users/runner/miniforge3/conda-bld/mlx_1703063827515/work/mlx/backend/metal/kernels/binary.metal:6:
2023-12-20T09:24:46.5901600Z   /Users/runner/miniforge3/conda-bld/mlx_1703063827515/work/mlx/backend/metal/kernels/utils.h:15:27: error: variable in constant address space must be initialized
2023-12-20T09:24:46.6004170Z     static const constant U max;
2023-12-20T09:24:46.6106660Z                             ^
2023-12-20T09:24:46.6207310Z   /Users/runner/miniforge3/conda-bld/mlx_1703063827515/work/mlx/backend/metal/kernels/utils.h:16:27: error: variable in constant address space must be initialized
2023-12-20T09:24:46.6307040Z     static const constant U min;
2023-12-20T09:24:46.6414930Z                             ^
2023-12-20T09:24:46.6517150Z   /Users/runner/miniforge3/conda-bld/mlx_1703063827515/work/mlx/backend/metal/kernels/utils.h:17:27: error: variable in constant address space must be initialized
2023-12-20T09:24:46.6598180Z     static const constant U finite_max;
2023-12-20T09:24:46.6699680Z                             ^
2023-12-20T09:24:46.6777010Z   /Users/runner/miniforge3/conda-bld/mlx_1703063827515/work/mlx/backend/metal/kernels/utils.h:18:27: error: variable in constant address space must be initialized
2023-12-20T09:24:46.6879650Z     static const constant U finite_min;
2023-12-20T09:24:46.6983390Z                             ^
2023-12-20T09:24:46.7084820Z   4 errors generated.

If I compile that natively (on Apple Silicon), I get the same error if I use "11.0" as the Xcode SDK and Deployment target. If I increase this then to 13.3, it passes. In the cross-compilation setup, the error sadly persists even with the more modern SDK and deployment target.

xhochy commented 11 months ago

One issue could also be the 13.3 SDK as only 13.4 is supported. Sadly, I don't know of a reliable way to retrieve a newer SDK in CI.

ngam commented 10 months ago

Sadly, I don't know of a reliable way to retrieve a newer SDK in CI.

Newer SDKs are usually available readily in the newer vm images we could use.

ngam commented 10 months ago

Should be resolved in https://github.com/conda-forge/mlx-feedstock/pull/4

ngam commented 10 months ago

The build passes but sadly the import fails with

libc++abi: terminating due to uncaught exception of type std::runtime_error: Failed to load device

The issue is that MTLCreateSystemDefaultDevice doesn't return a default device. The following gives an indication how to fix it but adding -framework CoreGraphics to LDFLAGS had no impact.

In macOS, in order for the system to provide a default Metal device object, you must link to the Core Graphics framework. You usually need to do this explicitly if you’re writing apps that don’t use graphics by default, such as command line tools.

@awni, we are again facing this error on arm64. Build passes, but runtime error...

awni commented 10 months ago

I've never seen that issue before (not being able to load the default device). You are compiling natively on an Apple silicon machine? Can you say more about the setup / environment.

xhochy commented 10 months ago

It happens both in cross- and non-cross-compilation mode. You can see the environment in the CI logs of https://github.com/conda-forge/mlx-feedstock/pull/4 . This is a cross-compilation log but as the error is equal on whether cross-compiled or not, it should give you the relevant information. Happy to provide you with the relevant information, but I'm not sure what you would need.

ngam commented 10 months ago

The only key difference (I can think of) is that we are using the LLVM Clang 16.0.6 (not the default Apple Clang 15.0.0 found on MacOS nowadays, at least on mine).

We can potentially test with the Apple Clang provided in the images, but that may require us some additional work...

awni commented 10 months ago

I tried building with Clang 17.0.6 and did not encounter this issue:

-- The CXX compiler identification is Clang 17.0.6

I see you are continuing to work on this!! How is it going, any updates?

ngam commented 10 months ago

From https://github.com/conda-forge/mlx-feedstock/pull/4#issuecomment-1868532090:

Current status: as is,

  • ✅ builds and runs successfully without cf compilers (i.e., with apple's arm64 compilers) using both native and cross compiling (note that the compilers are arm64, so it is not really cross compiling). Also, python -m unittest discover python/tests passes
  • ❌❌ does not run when built with conda-forge's clang compilers (not native and not cross compiling); that is, it builds fine, but doesn't actually work
  • ❌ does not run when built with apple's x86_64 clang; that is, it builds fine, but doesn't actually work

For the latter two, the error is the same: libc++abi: terminating due to uncaught exception of type std::runtime_error: Failed to load device

Has anyone tested cross-compiling here?

I can test with Clang 17 we have available in conda-forge (update: tested, fails ❌)

awni commented 10 months ago

Hi I wonder if you try again on main it might find the device now? We changed the way we find the default device in #370

ngam commented 10 months ago

@awni looks like it should be closer to working (I will test more thoroughly soon, but it passes locally for me). However, this PR introduces problems https://github.com/ml-explore/mlx/pull/350 (need to package the new lib correctly)

awni commented 10 months ago

@ngam could you say more about packaging the library correctly? It seems to work fine with PyPi? I tested it here: https://test.pypi.org/project/awni-test-mlx/0.0.7/

ngam commented 10 months ago

Oh... I only tested for conda-forge (not pypi) and assumed it would be the same for pypi. This may be limited to conda packaging; and if so, @xhochy or I would submit a patch if needed. For now, I believe we have this working on conda-forge (pending a new release from you and pending @xhochy's review). See https://github.com/conda-forge/mlx-feedstock/pull/4#issuecomment-1887842301.

awni commented 10 months ago

Oh ok, let me know if we need to change anything on our end!

awni commented 10 months ago

@ngam it turns out your were 100% right, the packaging was broken (🤦 I didn't consider that I had gguf already installed on the machines I tested on). We fixed this in the latest release. Hopefully that makes it easier to get the conda package working!

ngam commented 10 months ago

0.0.9 builds and passes. We should be able to release mlx on conda-forge soon 🎉

xhochy commented 10 months ago

…and pushed to conda-forge.

Verified that it works. I successfully fine-tuned Mistral-7B-v0.1 on my M1 GPU.

awni commented 10 months ago

This is so awesome!

One more dumb question: as we do new releases, how does the conda forge distribution get updated?

xhochy commented 10 months ago

conda-forge has a bot called @regro-cf-autotick-bot that will issue new PRs to https://github.com/conda-forge/mlx-feedstock where I will review and merge (and I have hopes that @ngam will continue to support me there). Once the PR is merged, the package will be available roughly 1h later on conda-forge.

awni commented 10 months ago

Thanks! And thanks for setting this up. I will plan to add the new install path to our docs

leofang commented 10 months ago

Thank you all for working on this! Very happy to see it landing on conda-forge.

ngam commented 10 months ago

Thanks @xhochy and @awni. I think we will benefit from someone on the mlx side getting involved as well (anyone interested we can add them as maintainers on the conda-forge side). As always @awni, don't hesitate to ping one of us if needed

We are compiling with our own LLVM clang compilers and we are building against macos sdk 13.3. If at any point, we need to increase the sdk version, e.g., for new features in metal, we can do that