Conversation
This is usually undesirable, as it indicates a leak, but can be useful in simple applications or tests where storing the model / context in a static is more feasible.
| // We _could_ end the residency of any extant residency sets here, but | ||
| // let's leave that to `ggml_metal_buffer_free`, the user is still in | ||
| // control of those buffers (and might free them later). | ||
|
|
There was a problem hiding this comment.
and might free them later
Specifically, since the order of global destructors across different translation units is complex to get right, I think it's possible for the user to not even have a leak with something like:
#include "llama-cpp.h"
static llama_model_ptr model;
int main() {
llama_backend_init();
model = llama_model_load_from_file("./model.gguf", llama_model_default_params());
llama_backend_free();
return 0;
}And still hit the assertion in case the Metal destructors runs before the user's model destructor.
|
/bot review |
Automated code reviewThe diff is small and self-contained: removal of an exit-time Summary of findings No blocking issues. The change is correct and minimal:
Suggestions:
Everything else (style, ASCII, naming, CMake integration) looks clean and consistent with the surrounding code. This review was generated automatically by pi coding agent using |
nikwen
left a comment
There was a problem hiding this comment.
To be honest, I'm not sure if dropping the free calls is something we should support. It comes at the cost of potentially not catching real errors.
That said, if other people feel differently, I'm okay with my opinion being ignored here.
Yeah, and I do understand wanting to be conservative in such cases. I think my main arguments are:
|
|
CC @ggerganov WDYT? |
|
@nikwen I'm still quite convinced that doing this is the right solution, what would you propose that I do to move it forwards? Are there other people that I need to ping, or a Discord I can join to pester y'all? Alternatively, would you accept a PR doing the opposite; adding this assertion on all backends / somewhere global, so that it's no longer platform-specific? |
|
Personally, if I were you, I'd focus my energy on more important problems. I don't think this one is worth the effort/energy. And, due to the concerns I expressed earlier, I'd be against merging it. |
|
Yeah, fair enough, thanks for the advice! |
Overview
Code like this currently hits a
GGML_ABORTin the Metal backend, because it detects on exit that the model wasn't unloaded:Such leaks are usually undesirable, but there are situations where it's useful, such as in simple applications or tests where storing the model (or context) in a static is more feasible.
So in this PR, I've removed the assertion, and added a test to ensure that this use-case remains supported.
Additional information
Introduced in #17766.
Fixes #22593.
Fixes #19137.
Replaces #22595.
Replaces #26857.
Replaces #22595.
Replaces #19206.
I decided to write a new PR because the others lacked a test, and were still doing the wrong thing IMO.
Requirements