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

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

Closed
xhochy opened this issue Dec 6, 2023 · 29 comments
Closed

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

xhochy opened this issue Dec 6, 2023 · 29 comments

Comments

@xhochy
Copy link

xhochy commented Dec 6, 2023

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
Copy link
Author

xhochy commented Dec 6, 2023

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
Copy link
Member

jagrit06 commented Dec 6, 2023

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
Copy link
Author

xhochy commented Dec 6, 2023

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
Copy link
Author

xhochy commented Dec 6, 2023

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
Copy link
Author

xhochy commented Dec 20, 2023

I have a PR ready that builds it for Apple Silicon on conda-forge: conda-forge/mlx-feedstock#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
Copy link
Member

awni commented Dec 20, 2023

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
Copy link
Author

xhochy commented Dec 20, 2023

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
Copy link
Author

xhochy commented Dec 20, 2023

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
Copy link

ngam commented Dec 21, 2023

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
Copy link

ngam commented Dec 21, 2023

Should be resolved in conda-forge/mlx-feedstock#4

@ngam
Copy link

ngam commented Dec 21, 2023

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
Copy link
Member

awni commented Dec 21, 2023

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
Copy link
Author

xhochy commented Dec 21, 2023

It happens both in cross- and non-cross-compilation mode. You can see the environment in the CI logs of conda-forge/mlx-feedstock#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
Copy link

ngam commented Dec 21, 2023

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
Copy link
Member

awni commented Dec 24, 2023

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
Copy link

ngam commented Dec 24, 2023

From conda-forge/mlx-feedstock#4 (comment):

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
Copy link
Member

awni commented Jan 11, 2024

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
Copy link

ngam commented Jan 11, 2024

@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 #350 (need to package the new lib correctly)

@awni
Copy link
Member

awni commented Jan 11, 2024

@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
Copy link

ngam commented Jan 11, 2024

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 conda-forge/mlx-feedstock#4 (comment).

@awni
Copy link
Member

awni commented Jan 11, 2024

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

@awni
Copy link
Member

awni commented Jan 11, 2024

@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
Copy link

ngam commented Jan 11, 2024

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

@xhochy
Copy link
Author

xhochy commented Jan 12, 2024

…and pushed to conda-forge.

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

@xhochy xhochy closed this as completed Jan 12, 2024
@awni
Copy link
Member

awni commented Jan 12, 2024

This is so awesome!

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

@xhochy
Copy link
Author

xhochy commented Jan 12, 2024

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
Copy link
Member

awni commented Jan 12, 2024

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

@leofang
Copy link

leofang commented Jan 12, 2024

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

@ngam
Copy link

ngam commented Jan 12, 2024

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

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

No branches or pull requests

5 participants