fix(server): skip memory guard when the model footprint is indeterminate - #308
Merged
Merged
Conversation
Follow-up to 0xShug0#306 per maintainer review: a model directory holding several GGUFs and no model.gguf is ambiguous, and the loader rejects it with its own "contains N GGUF files" error. The estimator already contributes no weights for such a directory, but ensure_model_fits_memory still compared the remaining fixed floor plus the configured headroom against free memory, so a large headroom (e.g. min_free_memory_mb=1000000) still answered 503 and masked the real loader error. estimate_model_memory_bytes now returns nullopt for that ambiguous case, and ensure_model_fits_memory skips the guard entirely when the footprint is indeterminate: the load can never allocate anyway, so the loader's error surfaces no matter how large the headroom is. Determinate footprints (single file, selected GGUF, safetensors/HF tree) still guard as before. Verified on macOS: ambiguous 2-GGUF dir with min_free_memory_mb=1000000 now fails with the loader's "contains 2 GGUF files" error (was 503); the same headroom on a single-GGUF dir still 503s; guard-off behavior unchanged; server_config_test passes.
Code-review follow-up to the ambiguous-directory skip: - extract estimate_model_memory_bytes into app/server/model_memory.[h|cpp] so the estimator is unit-testable instead of a private ServerState member - list the directory once, mirroring the loader's selection: model.gguf wins, the sole *.gguf is used alone, and several GGUFs without model.gguf stay ambiguous - ignore files whose size cannot be read instead of folding the file_size failure value into the sum - log when the guard skips an indeterminate model instead of failing silently: family-specific layouts (e.g. minimax_music3) may load such a directory successfully, so a skipped guard is worth surfacing - fix the --min-free-memory-mb help text and README to describe the opt-in default and the skip behavior - add estimator tests to server_config_test: single file, sole GGUF, model.gguf disambiguation, ambiguous directory, checkpoint tree, and relative aux resolution
"aux" is a reserved DOS device name, so creating .../aux under the temp root throws on Windows and fails server_config_test there. Rename the test directory to "sidecar".
Owner
|
@gqf2008 Thanks! PR merged. |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Motivation
Follow-up to #306 per the post-merge review: with
min_free_memory_mb: 1000000, a model directory holding two.gguffiles still returned503 insufficient_memory(estimated 0.12 GiB + 1000000 MiB headroom ...) before the loader could report its owncontains 2 GGUF fileserror. The estimator already contributes no weights for such an ambiguous directory, but the guard still compared the remaining fixed floor plus the configured headroom against free memory, so a large headroom masked the real loader error.Changes
estimate_model_memory_bytesreturnsstd::optional<size_t>:std::nulloptwhenmodel.pathis a directory with several GGUFs and nomodel.gguf(the footprint is indeterminate).ensure_model_fits_memoryskips the guard entirely when the estimate isnullopt, so the loader's own error surfaces no matter how large the headroom is. The skip is logged to stderr rather than silent. Determinate footprints (single file, selected GGUF, safetensors/HF tree) still guard exactly as before.app/server/model_memory.{h,cpp}so it is unit-testable, and now lists the directory once (mirroring the loader's selection:model.ggufwins, the sole*.ggufis used alone) instead of two passes.server_config_testgains estimator cases: single file, sole GGUF,model.ggufdisambiguation, ambiguous directory →nullopt, checkpoint tree sum, relative aux resolution.--min-free-memory-mbhelp text (still said "default 512").Known limitation
The ambiguity skip is deliberately permissive: model families with custom directory layouts (e.g.
minimax_music3, which assembles several named GGUFs) may load a multi-GGUF directory successfully, and the guard then makes no estimate for it — the load proceeds unguarded. There is no loader-level signal to distinguish those layouts from the spec-driven ones the loader rejects, so fully covering them would need per-family layout metadata in the loader interface; left for a follow-up if you want it.Verification
macOS (Apple M4), real GGUF symlinks:
min_free_memory_mb: 1000000→model directory contains 2 GGUF files: ...plus[server] memory guard skipped for model ...(was503 insufficient_memory).min_free_memory_mb: 1000000→ still503 insufficient_memory(guard intact for determinate footprints).server_config_testpasses (including the new estimator cases).