-
Notifications
You must be signed in to change notification settings - Fork 10.5k
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 memory: Small memory leak on init, dangling pointer, and unused autorelease pool in graph compute #5007
Conversation
… autorelease pool in graph compute
Thanks! Btw, do you have any idea why the Swift build is failing after the recent changes:
I can't figure it out - the function prototype is declared in |
It's definitely not a syntactic issue, as that exact same file compiles fine in my project (https://github.com/ptsochantaris/emeltal/blob/dd3173e53781def9bb757fa06a2c8d7ab05f381b/Emeltal/llama-cpp/llama.h#L235) so I strongly suspect it may have to do with some compile-time defines. I'll take a look to see if I can figure out a way to "break" it in my build tree... |
I see the SPM package uses an |
I've added the symlinks in this branch to see if this improves things. |
Even if it works, it's not desirable - I expect the |
Indeed, that makes sense - I'm not too familiar with the SPM package TBH and how it handles its dependencies. I can remove the symlinks from this PR if you'd prefer, since it's out of scope anyway. For what its worth, it does seem to be working, so for whatever reason the SPM build is probably missing those extra files - could it be the GGML SPM doesn't export those extra header files currently? |
(Actually, I've removed them anyway, since it's an unrelated change, to keep things tidy and in-scope for now, but it did prove that it's a question of missing include files, possibly in the ggml package) |
I think these warnings are related:
Need to understand what they mean |
An umbrella header is usually a manually created file that contains all the include files in whatever Swift module is being created. The file is usually specified using another file called a "modulemap" which tells the importing tool to "just import framework.h and it will contain all the includes" - however since I believe the ggml package does not have a manual one specified, I suspect it is somehow autogenerated, and there my knowledge of SPM is a bit limited, since I haven't worked a lot with C/C++ integration via SPM. My gut instinct is that possibly creating a module map file in the ggml package may do the trick... |
The section |
I just asked my local 8-bit Mixtral for some insight and I think it narrowed it down! 😄 make -j && ./main -m models/mixtral-instruct-8x7b-fast/ggml-model-q8_0.gguf -f build/task-swift-2.txt -n -1 -c 16384 -s 1 --temp 0 --repeat-penalty 1.0 --no-penalize-nl [INST] I have the following ggml repo structure: ./include/ggml/ggml.h I've created the following Swift Package.swift file:
When I use the ggml package as a dependency in another project called "llama", I get the following errors:
What do these errors mean and how do I fix them? [/INST] The errors you're encountering are related to the way Swift Package Manager (SPM) handles C and Objective-C headers. Here's a breakdown of the errors and some suggestions to resolve them:
These warnings indicate that the umbrella header for the 'ggml' module (ggml.h) does not directly include 'ggml-backend.h' and 'ggml-alloc.h'. SPM might not be able to find the necessary headers to build the module correctly. To fix this, you can modify your Swift Package Manager configuration to include these headers in the umbrella header (ggml.h) or create a module.modulemap file to explicitly list the required headers. Create a module ggml [system] {
header "/include/ggml/ggml.h"
export *
umbrella "/include/ggml"
} Then, update your Swift Package Manager configuration to include the // swift-tools-version: 5.5
import PackageDescription
let package = Package(
// ...
targets: [
.target(
name: "ggml",
path: ".",
exclude: [],
sources: [
"src/ggml.c",
"src/ggml-alloc.c",
"src/ggml-backend.c",
"src/ggml-quants.c",
"src/ggml-metal.m",
],
resources: [
.process("src/ggml-metal.metal")
],
publicHeadersPath: "include/ggml",
cSettings: [
// ...
],
linkerSettings: [
// ...
],
moduleMap: "include/ggml/module.modulemap" // Add this line
)
],
// ...
)
This error indicates that the type 'ggml_backend_sched_eval_callback' is not recognized. It's possible that the necessary header file containing the type definition is not being included properly. Since you've already addressed the header inclusion issue with the
This error is likely a result of the previous issues. Once you've resolved the warnings and errors related to the 'ggml' module, this error should be resolved as well. If the error persists, ensure that your 'llama' module is correctly configured and that all required headers are properly imported. [end of text] llama_print_timings: load time = 55003.60 ms Thanks @ptsochantaris for the assistance - will resolve this stuff tomorrow and merge the PR |
I suspect what needs to happen is something along these lines:
|
I've settled for this fix: |
…ov#5007) * Metal memory: Small memory leak on init, dangling pointer, and unused autorelease pool in graph compute * SPM header potential fix * Reverting symlinks
…ov#5007) * Metal memory: Small memory leak on init, dangling pointer, and unused autorelease pool in graph compute * SPM header potential fix * Reverting symlinks
There are two minor changes:
Copy
C method, and following the CF conventions this needs to be released in non-ARC code (and indeed the leak does show up in the leaks profiler). This PR changes it to:GGML_METAL_NDEBUG
is not defined -- otherwise theGGML_METAL_LOG_INFO
won't do anything in the first place.*s
and*device
references to avoid stale references into thedevices
list (which didn't break in the current code becausedevices
was leaked, but would break now that it is properly released)