-
Notifications
You must be signed in to change notification settings - Fork 9.7k
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
Metal support for Swift #3078
Metal support for Swift #3078
Conversation
@@ -140,12 +140,22 @@ @implementation GGMLMetalClass | |||
|
|||
ctx->d_queue = dispatch_queue_create("llama.cpp", DISPATCH_QUEUE_CONCURRENT); | |||
|
|||
#if 0 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thought it would be ok to replace this since it looked unfinished.
615b02c
to
ce92d75
Compare
Package.swift
Outdated
], | ||
publicHeadersPath: "spm-headers", | ||
cSettings: [ | ||
.unsafeFlags(["-Wno-shorten-64-to-32"]), | ||
.unsafeFlags(["-fno-objc-arc"]), | ||
.define("GGML_SWIFT"), | ||
.define("GGML_USE_METAL"), |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Isn't this change forcing everyone to use metal? Can this be a flag defined by the customer instead? For instance, I can't run llamacpp with metal ON with my old MacBook and an AMD card
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@kchro3 Let's address this comment and we can merge
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
how does it look now? i don't have an old macbook, but i was able to build it if i switched the if/else condition
# in Package.swift
#if arch(x86_64) // instead of arch(arm) || arch(arm64)
// demo that it's not using metal anymore
llm_load_tensors: ggml ctx size = 0.12 MB
llm_load_tensors: mem required = 7500.97 MB (+ 400.00 MB per state)
...................................................................................................
llama_new_context_with_model: kv self size = 400.00 MB
llama_new_context_with_model: compute buffer total size = 75.47 MB
what is the capital of japanToken received in Swift:
Token received in Swift: The
Token received in Swift: capital
Package.swift
Outdated
@@ -4,23 +4,27 @@ import PackageDescription | |||
|
|||
let package = Package( | |||
name: "llama", | |||
platforms: [.macOS(.v11)], |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I haven't tried iOS but I do wonder if we are OK by limiting this package to macOS only. Can llamacpp run on iOS, tvOS or even watchOS @ggerganov?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It can run even on a refrigerator 😄
Jokes aside - I see no reason to limit this to just macOS
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
i'm not sure why, but the compiler was not happy if platforms was not included, although that could be a metal thing?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
i don't have a way to test on watchOS or tvOS... i can set it to the minimum non-deprecated version and if someone tries to cross that bridge, we could update it?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
do you mind sharing the compiler error?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hey folks, I would appreciate help on this. I'm seeing in my build logs that it's compiling the .metal file even if I put it in resources
.
For example, if I do:
#if arch(arm) || arch(arm64)
let platforms: [SupportedPlatform]? = [
.macOS(.v11),
.iOS(.v11),
.watchOS(.v4),
.tvOS(.v11)
]
let exclude: [String] = []
let resources: [Resource]? = [
.copy("ggml-metal.metal"),
.copy("README.md") // just to validate that files get copied
]
let additionalSources: [String] = ["ggml-metal.m"]
let additionalSettings: [CSetting] = [
.unsafeFlags(["-fno-objc-arc"]),
.define("GGML_SWIFT"),
.define("GGML_USE_METAL")
]
#else
I still see the default.metallib in my resources:
ls /Users/.../Library/Developer/Xcode/DerivedData/.../Build/Products/Debug/....app/Contents/Resources/llama_llama.bundle/Contents/Resources/
README.md default.metallib
Could it be because the file is in the project root and the target path is "."? tried copying it into a new directory & excluding the original, but it still compiled...
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
what if you exclude ggml-metal.metal like it is done in master right now, but then add it in the resources section. That should do it I think
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
https://developer.apple.com/documentation/packagedescription/target/exclude#discussion
I think it doesn't work because exclude takes precedence over resources. For example, I pushed a branch https://github.com/ggerganov/llama.cpp/pull/3091/files, and my build logs don't show the .metal file:
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Just a sanity check, but this documentation is saying that .metal files get automatically compiled, and that seems to be the case from what I've tried. Is there a specific reason why we need to compile from source?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Just a sanity check, but this documentation is saying that .metal files get automatically compiled, and that seems to be the case from what I've tried.
Got it, it looks like a better way if we use it on Xcode.
cc: @ggerganov, i think that all comments are now addressed |
After I give a try in a new Xcode app project, figured out the reason of the
Also, Metal may not be available on watchOS. |
Co-authored-by: Jhen-Jie Hong <[email protected]>
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@jhen0409
Can't give this a test atm - if all works merge it
I have a working demo of using Metal w/ a Swift Mac app with these changes. Hopefully, this is a welcome contribution!