Skip to content

fix: library loading for swift dynamic frameworks#2568

Merged
angeloskath merged 2 commits intoml-explore:mainfrom
bilousoleksandr:main
Sep 18, 2025
Merged

fix: library loading for swift dynamic frameworks#2568
angeloskath merged 2 commits intoml-explore:mainfrom
bilousoleksandr:main

Conversation

@bilousoleksandr
Copy link
Copy Markdown
Contributor

@bilousoleksandr bilousoleksandr commented Sep 5, 2025

Proposed changes

By default, Dynamic Framework doesn't create a resources bundle and packs all resources inside its own Resources folder.

To fix the issue related to SwiftPM, add try loading metal library from Framework/../Resources/default.metallib when mlx codebase is built as a dynamic framework.

Checklist

Put an x in the boxes that apply.

  • I have read the CONTRIBUTING document
  • I have run pre-commit run --all-files to format my code / installed pre-commit prior to committing changes
  • I have added tests that prove my fix is effective or that my feature works
  • I have updated the necessary documentation (if needed)

@awni
Copy link
Copy Markdown
Member

awni commented Sep 5, 2025

@davidkoski do you mind checking this change is useful for MLX Swift?

std::tie(lib, error[3]) = load_colocated_library(device, "Resources/default");
if (lib) {
return lib;
}
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

load_colocated_library will get the directory of the binary that contains the get_binary_directory(). In the case of an embedded .framework it would be something like x.app/Contents/Frameworks/MLX.framework/Versions/A. This would then append Resources/default and .metallib onto that and load the file.

I think this works if the default.metallib is hoisted out of the mlx-swift_Cmlx.bundle/Contents/Resources. I am not sure it is because I don't have a .framework wrapping the MLX swiftpm library myself, but such a thing could surely be done.

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

That said, I am surprised that this does not do the same thing:

std::pair<MTL::Library*, NS::Error*> load_swiftpm_library(
    MTL::Device* device,
    const std::string& lib_name) {
#ifdef SWIFTPM_BUNDLE
...
  auto bundles = NS::Bundle::allBundles();
  for (int i = 0, c = (int)bundles->count(); i < c; i++) {
    auto bundle = reinterpret_cast<NS::Bundle*>(bundles->object(i));
    library = try_load_bundle(device, bundle->resourceURL(), lib_name);

That should scan all bundles (frameworks are considered bundles) looking for a default.metallib.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

It doesn't work this way, because bundles doesn't contain MLX framework path. In current configuration, it always looks for for main bundle and tries to load default.metallib from mlx-swift_Cmlx.bundle. But with dynamic frameworks, app directory looks next way:

  • App.app
    • Contents
      • Frameworks
        • MLX
          • Resources
            • default.metallib
      • Resources
NSBundle <~Work/DerivedData/Eney-dyyiohebtzyjuwbwalhqtrcvkvcn/Build/Products/Debug/App.app> (loaded),
NSBundle </System/Library/Extensions/AGXMetalG13X.bundle> (loaded),
NSBundle </System/Library/Input Methods/PressAndHold.app/Contents/PlugIns/PAH_Extension.appex> (not yet loaded),
NSBundle </System/Library/CoreServices/SystemAppearance.bundle> (not yet loaded)

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

@davidkoski I've covered a questions related to load_swiftpm_library above

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

OK, if the default.metallib is in the Resources directory, I agree this looks good.

@davidkoski
Copy link
Copy Markdown
Contributor

@davidkoski do you mind checking this change is useful for MLX Swift?

It might be, but:

  1. I don't have a good way to test it as I don't have a framework wrapped MLX and it seems to require a particular layout of the framework
  2. the existing load_swiftpm_library ought to do something identical when it scans the bundles -- I wonder why this didn't work?

Copy link
Copy Markdown
Member

@angeloskath angeloskath left a comment

Choose a reason for hiding this comment

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

This looks fine to me also given the discussion and @davidkoski 's agreement, so I 'll merge after the tests clear.

Copy link
Copy Markdown
Member

@angeloskath angeloskath left a comment

Choose a reason for hiding this comment

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

@bilousoleksandr the NS::Error error[4] needs to be NS::Error error[5].

@bilousoleksandr
Copy link
Copy Markdown
Contributor Author

@bilousoleksandr the NS::Error error[4] needs to be NS::Error error[5].

Fixed

@bilousoleksandr
Copy link
Copy Markdown
Contributor Author

@angeloskath could you re-approve the PR to enable CI run?

chore: pre-commit hook changes
@angeloskath angeloskath merged commit e8b604a into ml-explore:main Sep 18, 2025
7 checks passed
faisalmemon pushed a commit to faisalmemon/mlx that referenced this pull request Oct 30, 2025
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.

4 participants