Repository navigation
Conversation
| } | ||
| } | ||
|
|
||
| static uint64_t get_model_memory_mb(const common_preset& preset) { |
There was a problem hiding this comment.
IIRC this is mostly the same logic with -fit, right? If so, is it possible to merge them into a new function called common_fit_get_model_memory()
Btw, I think it can be useful to return more details about mem usage, for example: mem usage by context and by weight. In the future, we can also return usage per backend.
| add_opt(common_arg( | ||
| {"--models-memory-max"}, "N", | ||
| string_format("for router server, maximum memory usage in MB (default: %d, 0 = unlimited)", params.models_memory_max), | ||
| [](common_params & params, int value) { | ||
| params.models_memory_max = value; | ||
| } | ||
| ).set_examples({LLAMA_EXAMPLE_SERVER}).set_env("LLAMA_ARG_MODELS_MEMORY_MAX")); |
There was a problem hiding this comment.
UX-wise, I would prefer specifying the reverse like what --fit-target does. Instead of specifying max used memory, it's more intuitive to specify margin. But I'm not quite sure how hard it is to implement such logic here
There was a problem hiding this comment.
Good idea, this router feature can likely share both the memory calculation and the defaults with the autofit code.
|
@0cc4m I don't understand the description and the use case that you have. Can you provide an example with some sample model sizes? |
|
Sure, I have a DGX Spark (~120GB shared memory) running llama-server in router mode. It serves a Qwen3 0.8B Embedding model (~6GB), a Qwen3.5 35B Q6_K (~37GB) and a Qwen3.5 122B Q4_K_M (~81GB). By default, llama-server will load up to 4 models before starting to unload. A basic agent requires the Embedding model + the 35B, their combined ~43GB can be loaded simultaneously no problem. For a coding agent I use the 122B. Once I load that, it tries to add 81GB to the existing 43, the combined requirement of 124GB exceeds the limit and it will OOM the server (and freeze the system until the process is killed), currently. I can prevent that by only allowing a single model to be loaded at a time, but then simultaneous use of the 35B + the Embedding model will constantly load+unload them while they get used. This PR adds a memory limit to the llama-server, which allows it to recognize when loading a further model will overflow any device's memory, and start unloading models beforehand to try to make enough space. That way I can simultaneously use multiple small models, but also a large model that needs all memory to work. Does that help? |
0cc4m
changed the title
|
Here's an example debug log of the current state when I load the Qwen3 Embedding model, the 35B, the 122B and gpt-oss 120B: log |
| // Returns the projected memory use (model + context + compute) in bytes | ||
| // for the given device within this context. Returns 0 if the device is not used. | ||
| LLAMA_API uint64_t llama_context_device_memory( | ||
| const struct llama_context * ctx, | ||
| ggml_backend_dev_t device); | ||
|
|
There was a problem hiding this comment.
Most likely the device querying API should be more generic, so this signature is likely to be obsoleted. Let's move to llama-ext.h for now.
4312ed2 to
1d4a5f9
Compare
| void add_model(server_model_meta && meta); | ||
|
|
||
| // not thread-safe, caller must hold mutex | ||
| uint64_t get_memory_exceeded(const model_memory_map& new_model_memory_per_device) const; |
There was a problem hiding this comment.
| uint64_t get_memory_exceeded(const model_memory_map& new_model_memory_per_device) const; | |
| uint64_t get_memory_exceeded(const model_memory_map & new_model_memory_per_device) const; |
|
|
||
| // unload least recently used models if the limit is reached | ||
| void unload_lru(); | ||
| void unload_lru(const model_memory_map& new_model_memory_per_device); |
There was a problem hiding this comment.
| void unload_lru(const model_memory_map& new_model_memory_per_device); | |
| void unload_lru(const model_memory_map & new_model_memory_per_device); |
| ).set_examples({LLAMA_EXAMPLE_SERVER}).set_env("LLAMA_ARG_MODELS_MAX")); | ||
| add_opt(common_arg( | ||
| {"--models-memory-margin"}, "N", | ||
| string_format("for router server, MB of memory to leave free, per device (default: %d, 0 = unlimited)", params.models_memory_margin), |
There was a problem hiding this comment.
| string_format("for router server, MB of memory to leave free, per device (default: %d, 0 = unlimited)", params.models_memory_margin), | |
| string_format("for router server, MiB of memory to leave free, per device (default: %d, 0 = unlimited)", params.models_memory_margin), |
| if (total > 0) { | ||
| const uint64_t available = (free > memory_margin) ? free - memory_margin : 0; | ||
| memory_per_device[dev] = available; | ||
| SRV_DBG("device %s: available memory after margin=%lu MB\n", |
There was a problem hiding this comment.
| SRV_DBG("device %s: available memory after margin=%lu MB\n", | |
| SRV_DBG("device %s: available memory after margin=%lu MiB\n", |
| model_memory_map total_memory_per_device; | ||
| for (const auto & m : mapping) { | ||
| if (m.second.meta.is_running()) { | ||
| for (const auto& [key, value] : m.second.meta.memory_per_device) { |
There was a problem hiding this comment.
| for (const auto& [key, value] : m.second.meta.memory_per_device) { | |
| for (const auto & [key, value] : m.second.meta.memory_per_device) { |
|
|
||
| uint64_t memory_exceeded = 0; | ||
|
|
||
| for (const auto& [key, limit] : memory_per_device) { |
There was a problem hiding this comment.
| for (const auto& [key, limit] : memory_per_device) { | |
| for (const auto & [key, limit] : memory_per_device) { |
|
|
||
| void server_models::unload_lru() { | ||
| if (base_params.models_max <= 0) { | ||
| uint64_t server_models::get_memory_exceeded(const model_memory_map& new_model_memory_per_device) const { |
There was a problem hiding this comment.
| uint64_t server_models::get_memory_exceeded(const model_memory_map& new_model_memory_per_device) const { | |
| uint64_t server_models::get_memory_exceeded(const model_memory_map & new_model_memory_per_device) const { |
| common_preset base_preset; // base preset from llama-server CLI args | ||
|
|
||
| // available memory per device | ||
| std::map<ggml_backend_dev_t, uint64_t> memory_per_device; |
There was a problem hiding this comment.
| std::map<ggml_backend_dev_t, uint64_t> memory_per_device; | |
| model_memory_map memory_per_device; |
| return it != m.end() ? it->second : 0; | ||
| }; | ||
|
|
||
| uint64_t memory_exceeded = 0; |
There was a problem hiding this comment.
Let's keep all memory sizes in size_t type for consistency.
| int port = 0; | ||
| server_model_status status = SERVER_MODEL_STATUS_UNLOADED; | ||
| int64_t last_used = 0; // for LRU unloading | ||
| model_memory_map memory_per_device; // projected bytes per device |
There was a problem hiding this comment.
I am still not sure why we have to keep a memory map both in server_model_meta and server_models. Isn't one map in struct server_models going to be enough?
There was a problem hiding this comment.
The idea here was to create a map of how much memory is available within the margin for each device when the server is started. That's what is stored in the server_models struct. It should at least get a clearer name.
server_model_meta then stores the requirement per device for that model. That way it can be summed up and compared to the values in server_models.
There was a problem hiding this comment.
I see. Yes, better names are needed.
|
I've addressed the feedback. |
|
I'll take a look next week. |
0124ec9 to
3c53be1
Compare
| if (params.model.path.empty()) { | ||
| return {}; | ||
| } |
There was a problem hiding this comment.
@0cc4m This check prevents using the functionality for models downloaded with -hf because they don't have a path. Should be fixed before merging.
There was a problem hiding this comment.
You're right, I hadn't considered that. Currently it will let it through, but estimate it as having no size, which is not ideal. I could fill the map on second load, but that risks an OOM when it's loading for the first time. I could try to estimate with file size before downloading, but that's unreliable and hard to map to devices. Or I could trigger the download before actually loading with common_download_model, estimate and then load. Not sure if that would cause issues in other places, I'd have to try it. What do you think?
There was a problem hiding this comment.
Hm not sure. Seems complicated.
Maybe the right way is to have a --download-only CLI argument. If set, the llama-server (and any other tool) would just download the model and exit.
This way, we can split the model loading in stages:
llama-server ... --download-only- Perform memory size checks
llama-server ... --offline
There was a problem hiding this comment.
I implemented this, please take a look when you find some time.
61c2568 to
cf0ebc4
Compare
cf0ebc4 to
da1f168
Compare
|
This is an interesting thing, I use both small and large models as well, on different systems. |
|
@ggerganov This is still waiting for your review. |
|
|
||
| // Returns the projected memory use (model + context + compute) in bytes | ||
| // for the given device within this context. Returns 0 if the device is not used. | ||
| LLAMA_API uint64_t llama_context_device_memory( | ||
| const struct llama_context * ctx, | ||
| ggml_backend_dev_t device); |
There was a problem hiding this comment.
Instead of adding this new function, can you reuse the llama_get_memory_breakdown that we added recently (#22171)?
| device_memory_map mem; | ||
| if (base_params.models_memory_margin > 0) { | ||
| std::lock_guard<std::mutex> lk(mutex); | ||
| auto & meta = mapping[name].meta; | ||
| meta.dmm_req = get_model_memory_per_device(meta.preset); | ||
| if (meta.dmm_req.empty()) { | ||
| SRV_WRN("failed to estimate memory for model %s, memory limits will not apply\n", name.c_str()); | ||
| } | ||
| mem = meta.dmm_req; | ||
| } |
There was a problem hiding this comment.
Should this chunk of code be moved at the start of _load() so you can deduplicate it here?
| SRV_ERR("failed to load model %s after download: %s\n", name.c_str(), e.what()); | ||
| update_status(name, SERVER_MODEL_STATUS_UNLOADED, 1); | ||
| } | ||
| }).detach(); |
There was a problem hiding this comment.
In general, I consider detaching threads an anti-pattern. Add a TODO to keep track of the threads in server_models and join them periodically (maybe on each load() and/or update_status()).
| return ""; | ||
| } | ||
|
|
||
| static device_memory_map get_model_memory_per_device(const common_preset & preset) { |
There was a problem hiding this comment.
Re-thinking about this, I think it's better to make a new mode on llama-server that simply measure the memory usage then exit (without loading model weights). Probably adding a new env variable LLAMA_SERVER_MEASURE_ONLY=1 when launching the child process.
The problem with calling llama_model_load_from_file here is that a broken GGUF can trigger GGML_ASSERT which crash the whole router process.
|
#23604 (comment) cross-posting my thoughts here for viz. |
6adf964 to
82403fd
Compare
82403fd to
645d17e
Compare
|
The CUDA memory issue should be resolved now. |
645d17e to
37a767f
Compare
| common_log_pause(common_log_main()); | ||
| for (const auto & [buft, data] : llama_get_memory_breakdown(ctx.get())) { | ||
| size_t total = data.total(); | ||
| if (total > 0) { | ||
| fprintf(stdout, "measure:%s %zu\n", ggml_backend_buft_name(buft), total); | ||
| } | ||
| } | ||
| fflush(stdout); | ||
| common_log_resume(common_log_main()); |
There was a problem hiding this comment.
Wrap this logic in a dedicated function in common/fit.h/.cpp to avoid including llama-ext.h here.
Also, probably need to have more unique string prefix than measure:. For example something like: "__TAG_FIT_MEMORY_TOTAL__:%s %zu\n"
|
The CUDA issue is not yet resolved, it caused some unforeseen issues. (#24715) Maybe there is no good way but to read the GPU memory state from a different process as well? Any other ideas @ggerganov @JohannesGaessler ? I'm not sure if there's a safe way to reset the CUDA context, and it'd likely still have to be within a backend context if we go that way (so backend init -> memory read -> backend free, for the router case), which is unnecessary overhead for any other backend, though maybe not terribly much? |
d1b5a68 to
5a74622
Compare
|
FYI @0cc4m , this PR could be a bit related to your works here: #24821 I'm planning to make it such that the router code stays as clean as possible. With the recent additions of model management API, downloading thread management is added to the router itself and turns out it's not a good idea. The code complexity blows up too much. My new plan would be that: anything model-related will be handled in child process, including:
The way it works is: router spawns a child process with a special flag, child instance calculates memory requirements (--fit), then exit without loading the model So I will go back to this after I finish #24822 ; no actions are needed from your side now, but I will ping you when I get there |
5a74622 to
e4d2e19
Compare
|
@0cc4m sorry for the late reply, my opinion is that the issues with the previous solution for CUDA are in principle fixable. However, I think that I underestimated the potential for unforeseen and difficult to debug issues. In general, I think that we do not actually need a 100% accurate estimate of the memory use, some amount of inprecision if fine as long as it's sufficiently small vs. the safety margin that we are already using anyways. How about this: in the ggml backend device interface it is possible to retrieve PCI bus IDs. In the llama.cpp user code, if both CUDA and Vulkan are available, fetch free memory for "Vulkan devices" first and use those values for "other devices" with the same PCI bus IDs (+ maybe some rough estimate of how much memory the CUDA context would need). This would not fix persistent memory use after unloading a model but it would fix CUDA stealing VRAM from other backends. |
|
That would just point back at the NVML memory query implementation. But the problem with that is that CUDA uses unusually much context memory compared to other backends, so the "underestimation" through these is worse, unusably so judging by the feedback we got. |
…ing models when they exceed a memory size threshold estimate with to-be-loaded model size included use no_alloc to get memory requirements for model load only set model memory_mb if not previously calculated use memory margin instead of total size limit, apply to each device separately add server memory debug logging move llama_context_device_memory function to llama-ext.h fix model count exceeded check improve memory_per_device map naming improve variable naming, fix style also strip models memory margin from child processes cont : clean-up replace device memory map with buft memory map. Use llama_get_memory_breakdown extract duplicated check into helper function move model memory estimation to subprocess precompute name->buft map, map GPU host types to CPU buft cleanup unused variable remove duplicated init calls
e4d2e19 to
f370471
Compare
|
I will have a look on new changes here this week |
|
ok so I just done a review pass on this PR and roughly understand what it does:
that's actually a cleaner approach than my initial proposal of having every model to report their mem usage (no matter if they are loaded). however, I still want to see if we can push it further: as explained in the issue, it can be useful for downstream (like for example pi-agent) to know exactly the context length that it can have if it starts a given model, and this number can changed dynamically based on how much system memory is available. a quick check around the code base suggests that it could be possible to use also, I will see if it's possible to directly use I will push my changes directly to this PR |
|
Why not just do that in a follow-up PR? |
|
my suggestion and the current PR are too overlapped so it's better to be done here, rather than a follow-up for example, if memory usage is measured once at server boot up instead of per-instance launch, then the will see if I can focus on this later this week. I already had the details in mind, will not take too long to implement all the changes |
|
I'll go back to this right after #28555 |
|
@ngxson Any ETA for that? |
This branch has not been deployed
8 participants
Overview
I have a server running which serves small embedding models and large text models. Currently there's no way to allow multiple small models to coexist in memory, but to unload them when a large model needs space. This PR solves that by adding a
--models-memory-marginparameter which works similarly to the autofit feature memory margin. The router keeps track of memory requirements per model and uses it to unload models in the same order as previously defined for the max number of models, once that margin is exceeded on any device.I've been running this on my server and it solves the issue I had.
Requirements