studio: refuse a gguf that cannot fit in free vram plus available ram - #8883
studio: refuse a gguf that cannot fit in free vram plus available ram#8883mahiatlinux wants to merge 31 commits into
Conversation
`--fit on` places what it can in VRAM and spills the rest into host memory with no ceiling of its own, and nothing upstream of llama-server prices that spill. The weights are memory-mapped, so an oversized remainder does not fail an allocation: the kernel evicts and re-reads the mapping every token until the machine stops responding, and systemd-oomd then kills the whole app slice. llama_cpp.py already refuses this on Apple unified memory and on AMD APUs, but _amd_apu_wants_unified_memory is false for a discrete card, so CUDA and ROCm hosts had no guard. _host_offload_shortfall_message adds the same ceiling, priced on the spill rather than on the whole model.
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: d6bdeef85b
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
… guard Three cases where the host-offload guard read a footprint the launch does not produce. Manual + Auto layers omits -c and leaves effective_ctx at 0, so _kv_bytes(0) returned zero while the launch still floors --fit-ctx at 8192 and allocates that KV. Price the floor, now a shared _AUTO_FIT_MIN_CTX so the guard and the emitted flag cannot drift. The Model Memory page-lock setting emits mmap+mlock for a --fit on launch, which pins the whole mapping in host RAM including the layers copied to the GPU. Subtracting free VRAM there under-prices by the GPU-resident portion, so charge the whole mapping when should_mlock() is set. A Vulkan iGPU reports shared system RAM as its free VRAM and total 0. Subtracting it and then charging the remainder against that same RAM counts the pool twice, so a 20 GB model on a 14 GB host read as a 6 GB requirement. Exclude a total-0 Vulkan device from the VRAM offset.
|
@codex review |
for more information, see https://pre-commit.ci
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 20369b674b
ℹ️ About Codex in GitHub
Codex has been enabled to automatically review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".
| _offload_msg = self._host_offload_shortfall_message( | ||
| model_size | ||
| + _guard_kv_bytes | ||
| + _mtp_reserve_bytes | ||
| - _fit_vram_mib * 1024 * 1024, |
There was a problem hiding this comment.
For a --fit on load at a large context or across multiple GPUs, _compute_buffer_pipeline, the context-linear _cc_bytes(...), and _soft_overhead consume VRAM before weights can be placed; the placement logic includes these terms in model_size_fit at line 14326. This calculation instead subtracts all raw free VRAM from only the model, KV, and MTP sizes, so every omitted buffer byte can displace another weight byte into host RAM. Once those buffers exceed the fixed 2 GiB headroom, a load whose real spill is larger than available RAM can still pass this preflight and enter the paging/OOM failure being guarded against.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Not taking this one. The guard's documented contract is to under-state the spill so that only an unambiguous shortfall refuses: it subtracts raw free VRAM with no margin removed, which already omits llama.cpp's own 1024 MiB --fit-target. Folding in _compute_buffer_pipeline, _cc_bytes and _soft_overhead would turn a deliberate floor into an exact predictor built on estimates, and every one of those terms overshooting refuses a load that would have run. The miss you describe needs the real spill to land inside a narrow band between the 2 GiB headroom and the buffer total; a false refusal costs a user a model that works. Staying conservative.
| if ( | ||
| not _guard_kv_bytes | ||
| and effective_ctx <= 0 | ||
| and gpu_memory_mode == "manual" | ||
| and (gpu_layers or 0) < 0 | ||
| ): | ||
| _guard_kv_bytes = _kv_bytes(_AUTO_FIT_MIN_CTX) |
There was a problem hiding this comment.
The remaining file-size-only placement path is not covered by the new zero-context fix: when _can_estimate_kv() is false, _estimate_kv_cache_bytes returns 0 even for a positive native or requested context, and this fallback is restricted to Manual + Auto with effective_ctx <= 0. Such metadata-sparse GGUFs are deliberately supported by the elif gpus file-size-only branch, but their launched server still allocates a KV cache; at a large context that unpriced cache can push the host spill past available RAM while this guard allows the load.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Not taking this one. When _can_estimate_kv() is false the GGUF is missing the layer, head and embedding metadata the estimate is built from, so there is no principled reserve to apply, only an invented constant. That is why the file-size-only branch exists at all. Guessing high refuses exactly the metadata-sparse models that branch is there to support, and guessing low buys nothing. Leaving the KV term at zero there is the same under-statement the guard is documented to make.
| use_fit | ||
| and model_size is not None | ||
| and kv_cache_bytes is not None | ||
| and not self._amd_apu_wants_unified_memory(gpu_indices) |
There was a problem hiding this comment.
If any exception occurs after the model size or GPU pool is known but before the final kv_cache_bytes = _kv_bytes(effective_ctx) assignment, the broad handler at line 14772 deliberately falls back to use_fit = True while leaving kv_cache_bytes as None. This condition then skips the RAM guard entirely, so an estimator, capability, or GPU-selection error sends a potentially oversized model into llama.cpp's fitter with no host ceiling at all. Apply the guard to the known model and GPU data on this fallback, using a conservative or zero KV term when no estimate survived, rather than using KV completion as the sentinel for whether to check RAM.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Not taking this one, and the sentinel is not really about KV. That handler runs when the GPU probe or selection threw, and _detected_gpus is initialized to [] before the try, so on most of those paths the pool never enumerated. Pricing a spill against an empty pool charges the whole model to host RAM and refuses loads that would have run fine on the GPU the probe failed to read. The fallback is deliberately fail-open: an error in our own estimator must not become a refusal for the user. kv_cache_bytes is the sentinel because it is assigned last, so it is the one value that proves the whole block completed.
Manual mode with an explicit layer count emits --gpu-layers N --fit off, but that only clears use_fit while the command is built, long after the guard reads the seeded True. At 0 layers the GPUs hold nothing, so subtracting their free VRAM under-priced the whole model; a positive count cannot be sized per layer here, so the guard abstains instead of pricing a placement it cannot compute. --no-kv-offload keeps the whole KV cache in host RAM, where it is not fungible with weights the GPUs can take. Charge it after the VRAM subtraction rather than inside it, so 12 GB of weights that fit the card no longer absorb an 8 GB CPU-resident cache.
|
@codex review |
for more information, see https://pre-commit.ci
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: fc14be3ac0
ℹ️ About Codex in GitHub
Codex has been enabled to automatically review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".
| _kv_on_host = not _kv_offload_from_args(extra_args) | ||
| _host_bytes = ( | ||
| model_size | ||
| + _mtp_reserve_bytes |
There was a problem hiding this comment.
When speculative mode is Auto and the target itself needs some CPU offload, _spec_dropped_no_vram does not remove the optional drafter because that fallback only runs when the target fits fully on a GPU subset. Adding _mtp_reserve_bytes here can therefore make the RAM guard reject a target whose spill fits by itself—for example, a 4 GiB target spill on a host with 7 GiB available passes the 2 GiB headroom check, but a 2 GiB automatic MTP reserve turns it into a refusal. Since the existing placement policy explicitly drops Auto speculative decoding rather than sacrifice a viable target load, retry this host check with the automatic drafter disabled before raising.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Not taking this one. Skipping _mtp_reserve_bytes in the arithmetic alone would pass a load whose drafter then loads anyway and takes the memory, which is worse than the refusal: the guard would be pricing a placement the launch does not build. Making it right means actually dropping the drafter, which is _spec_dropped_no_vram's job in the speculative fallback, not a RAM preflight's. Charging what the launch will really load is the correct behaviour here. If Auto should also drop the drafter when the target needs CPU offload, that belongs in the fallback that owns the decision, and this guard will price whatever it settles on.
…check On a mixed ROCm host the arch gate can leave gpu_indices None while pinning the launch to a discrete card. _amd_apu_wants_unified_memory then saw the whole physical set, read the unsupported APU as unified memory and skipped this guard, while the APU guard above had already cleared its own refusal for the same placement. Ask about the devices the launch will pin instead. A drafter pinned with --spec-draft-ngl 0 leaves the VRAM budget but keeps its weights in host RAM, and nothing was charging them. Size it before the CPU nulling and add it to the host figure. Both need the spill clamped at zero before host-pinned bytes are added, or spare VRAM absorbs them: a 4 GB target on a 14 GB card was cancelling an 8 GB CPU drafter to a negative footprint.
|
@codex review |
for more information, see https://pre-commit.ci
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 89a93adf63
ℹ️ About Codex in GitHub
Codex has been enabled to automatically review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".
… guard With both Model Memory toggles off, apply_model_memory_policy preserves a caller's --mlock or --no-mmap, so the launch keeps a full host copy while should_mlock() reports false. Ask resolve_effective_memory_state, which already answers for argv and env together, instead of the setting alone. A --device naming no GPU leaves the child nothing to offload onto, so its free VRAM is credit the launch never gets. _device_selection_is_cpu now zeroes the offset, alongside the paravirtual and arch-gate cases already there. A CPU-pinned drafter's KV cache was dropped with its weights when _mtp_draft_for_budget is nulled; size both before that and charge them to the host. _available_system_memory_mib now caps its reading by _cgroup_free_bytes. Inside a container or systemd scope the binding memory.max is what the OOM killer enforces, and the host MemAvailable can be tens of GiB above it. The APU guard reads the same helper and gains the ceiling too.
|
@codex review |
for more information, see https://pre-commit.ci
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 83cebc7a3d
ℹ️ About Codex in GitHub
Codex has been enabled to automatically review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".
A CPU device override now triggers the host check on its own. A successful fit clears use_fit, so gating the whole guard on it skipped a retained --device none that still loads the model on the CPU. resolve_effective_memory_state gets the launch environment, not just argv. scrub_memory_env keeps an inherited LLAMA_ARG_MLOCK or a reserving LLAMA_ARG_LOAD_MODE when both toggles are off, and argv alone cannot see them. The CPU drafter's KV is sized at the fit floor, matching the main KV term: Manual + Auto leaves the context at 0, where _mtp_draft_kv_bytes returns None while the launch still emits --fit-ctx. The cgroup reading goes through _shared_policy instead of importing unsloth.dataset_num_proc directly, which would run the package __init__ and load the model stack in the middle of a llama.cpp load.
|
@codex review |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 704eb779cb
ℹ️ About Codex in GitHub
Codex has been enabled to automatically review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".
…will run at Three ways the previous rounds' fixes over-charged a viable load. An explicit gpu_ids pick strips the device arguments in the command builder, so a stale --device none no longer zeroes the VRAM credit for a launch that does use the GPUs. Page-locking duplicates the weight mapping in host RAM, not the KV cache. The mlock branch charged the whole combined footprint, so a partially offloaded model could be refused over a KV cache that never leaves the card. Charge the mapping and let the GPU pool still cover the GPU-resident KV and MTP terms. "Don't reserve system RAM" strips --mlock and --no-mmap in apply_model_memory_policy, so resolving the raw flags claimed a full host copy the child never keeps. Read both settings and let no-reserve win, as should_mlock does. Alongside those, the guard context now honours a pass-through --fit-ctx instead of assuming the 8192 floor, the MTP reserve is recomputed at that same context, and the CPU drafter's KV is sized with the cache types, SWA, unified and ubatch options the GPU-side call already passes.
|
@codex review |
for more information, see https://pre-commit.ci
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 49e8215518
ℹ️ About Codex in GitHub
Codex has been enabled to automatically review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".
… the charge An -ngl 0 in the extras is appended after Studio's own flags and wins, so it now counts as a CPU-only placement alongside --device none. A device override also beats the positive-manual exemption: that exemption exists because a partial split cannot be sized, which stops being true once the launch reaches no GPU. Manual mode at 0 layers leaves the weights in host RAM but a companion-visible launch still holds its KV and drafter on the card. Zeroing the whole VRAM credit there charged those to RAM and could refuse a CPU model that fits, so it now shares the page-lock treatment: the weight mapping goes to host, the GPU pool still covers the GPU-resident terms. The auto-fit floor is priced only when the build supports --fit-ctx, since _ctx_integrity_flags omits the flag otherwise and the fitter may choose less. _available_system_memory_mib adds back the cgroup's reclaimable inactive_file. memory.current charges the GGUF's own page cache, which the kernel reclaims rather than OOM on, so subtracting it counted those pages twice.
|
@codex review |
for more information, see https://pre-commit.ci
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 516a8bd8fa
ℹ️ About Codex in GitHub
Codex has been enabled to automatically review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".
_backend_lacks_gpu_lib identifies a split-library llama-server whose lib directory carries cpu or base and none of cuda, hip or vulkan. That build cannot offload, but the hardware probe still enumerates the host's cards, so the guard was subtracting VRAM the child can never allocate on: a 20 GiB model with 16 GiB free read as a 4 GiB spill an 8 GiB host holds, while the child placed all 20 GiB in RAM. Fold it into the same flag the arch gate and manual zero-offload already set, renamed to child_has_no_gpu since a CPU-only build is not masked off anything. The helper fails open on a static or unrecognised layout, so a custom GPU build keeps its credit and cannot be refused. Reported by Codex.
|
@codex review |
1 similar comment
|
@codex review |
for more information, see https://pre-commit.ci
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 0b22ec8d27
ℹ️ About Codex in GitHub
Codex has been enabled to automatically review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".
_get_gpu_memory returns [] for "no supported GPU is reachable", not only for a probe that raised: the torch fallback exists so callers can read [] as "no GPU" and drop to CPU. The abstention treated both the same, so an ordinary CPU-only machine running a GPU-capable build skipped the guard while the whole GGUF went to host RAM. Record whether the enumeration ran and let the launch vouch for an empty pool. A probe that throws leaves the flag unset and still abstains. _detected_gpus is captured before manual mode empties the planner pool, so a GPU host cannot reach this with an empty list. Drops test_an_unprobed_gpu_pool_abstains_rather_than_refusing, which asserted the old reading of an empty pool. test_a_failed_enumeration_still_abstains covers the case it was written for. Reported by Codex.
|
@codex review |
1 similar comment
|
@codex review |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: b658052edd
ℹ️ About Codex in GitHub
Codex has been enabled to automatically review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".
_get_gpu_memory catches its own probe failures and returns [] (the torch fallback at llama_cpp.py:6673), so a flag set on "the call did not raise" marks a host whose every probe failed as GPU-less. The guard then priced the full model and could refuse a load llama-server's own enumeration still places on a card. That is a false refusal, which the floor is built never to produce, so the inference comes out and an empty pool abstains again. A host with no GPU at all stays uncovered, in the permissive direction the check is documented to take. The refusal message also read as contradictory arithmetic when the spill fit in available RAM but not inside the headroom: 7 GB refused against 8 GB available. Name the reserve and the usable figure, rounding the need up and the usable down so the printed pair keeps the ordering that produced the refusal. Both reported by Codex.
|
@codex review |
1 similar comment
|
@codex review |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 447767c5a0
ℹ️ About Codex in GitHub
Codex has been enabled to automatically review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".
Two false refusals in the CPU-only signal. _installed_ggml_backends scans for base, cpu, cuda, hip and vulkan only, so a split-library build shipping SYCL, OpenCL, MUSA or CANN recorded no GPU backend and _backend_lacks_gpu_lib called it CPU-only. The guard then priced the whole GGUF against RAM and refused a load the accelerator can hold. _binary_ships_no_gpu_backend reads the same lib directory through _GGML_GPU_BACKEND_RE, which already knows all nine names, and still requires a proven split-library layout so a static build keeps its credit. The narrower helper is left alone: its other caller gates a device pin, not a refusal. --rpc places layers on remote devices, and the pass-through is not stripped, so sizing a launch against local capacity alone refused a viable distributed run. Abstain when it carries a value. Both reported by Codex.
|
@codex review |
1 similar comment
|
@codex review |
for more information, see https://pre-commit.ci
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 6657410749
ℹ️ About Codex in GitHub
Codex has been enabled to automatically review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".
Both remaining false refusals were the environment form of something already read from argv or from disk. llama.cpp accepts RPC servers through LLAMA_ARG_RPC as well as --rpc, so a distributed launch configured that way was still priced against local capacity. GGML_BACKEND_PATH points the child at backend plugins outside the directory beside the executable, so a cpu-only layout there is no longer proof the child cannot offload. The guard now takes the finished child environment and both answer by abstaining. The refusal also told users to lower the context length. This prices weights only and omits the KV cache entirely, so context changes none of its inputs and the advice sent them through a reload that fails identically. All three reported by Codex.
|
@codex review |
1 similar comment
|
@codex review |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 8ade74692d
ℹ️ About Codex in GitHub
Codex has been enabled to automatically review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".
| binary = binary or LlamaCppBackend._find_llama_server_binary() | ||
| if not binary: | ||
| return False | ||
| source = os.environ if env is None else env | ||
| if str(source.get("GGML_BACKEND_PATH", "") or "").strip(): |
There was a problem hiding this comment.
Fresh evidence beyond the inherited-GGML_BACKEND_PATH fix is the supported custom-wrapper flow: a wrapper may export backend variables before executing llama-server, but env here is captured before that wrapper runs while _llama_lib_dir(binary) resolves through it to the target directory. If that directory contains only libggml-cpu/libggml-base and the wrapper exports GGML_BACKEND_PATH pointing at an accelerator plugin, this method incorrectly returns true, causing the guard to discard usable accelerator memory and falsely refuse a viable model. Treat launch-preserved custom wrappers as an unknown layout rather than proven CPU-only.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Not taking this one. It rests on a wrapper that exports GGML_BACKEND_PATH after the environment this reads is captured, which is a flow no launch path here constructs and nothing in the repo produces: the fix would be hardening against a configuration we cannot observe, and the only way to be safe against it is to never conclude CPU-only from the lib directory at all, which discards the accelerator-name fix from the previous round.
Worth saying plainly that this is where I am stopping rather than that the reasoning is bad. Across the last six rounds this file has produced 3, 1, 2, 2, 3 and 2 items, every one a narrower instance of the same question about which signals prove a placement. Each fix has been correct and each has exposed one more variant. The remaining ones are P2 and increasingly hypothetical, and the guard is a floor whose failure mode in every one of them is to refuse less. I would rather hand a maintainer a stable diff than keep trading rounds for that.
_paravirtual_cpu_forced rewrites the finished command to --gpu-layers 0 --device none on a virtualised Apple GPU, and Metal hosts leave the probed pool empty, so the guard read the placement as inconclusive and abstained. It is the fourth state the launch already knows reaches no card, so it belongs beside the other three rather than falling through to the empty-pool abstention. Reported by Codex.
|
@codex review |
for more information, see https://pre-commit.ci
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: d0f2ec82a5
ℹ️ About Codex in GitHub
Codex has been enabled to automatically review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".
| _offload_msg = self._launch_host_shortfall_message( | ||
| cmd, | ||
| _detected_gpus, | ||
| env, | ||
| child_has_no_gpu = ( |
There was a problem hiding this comment.
When an auto-selected Vulkan launch crashes, _try_auto_vulkan_cpu_fallback later replaces this checked command with a --gpu-layers 0 --device none replay (lines 16299-16320), without invoking the guard again; the persisted intent.cpu_fallback path does the same before its first spawn (lines 16400-16424). For example, a 20 GiB model with 16 GiB free VRAM and 8 GiB available RAM passes here as a 4 GiB spill, then the fallback attempts to place all 20 GiB in RAM. Fresh evidence beyond the previously rejected respawn finding is this revision's new child_has_no_gpu input: the guard is now explicitly placement-dependent, but neither CPU replay supplies the required true value.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
The reasoning is right and I want to concede it precisely, because it invalidates my earlier rejection. I turned down the respawn finding on the grounds that the guard's inputs were identical across a replay. That was true of the weights-only floor as it stood then. It stopped being true when I added child_has_no_gpu, which is exactly what this points out: the check is placement-dependent again, and neither CPU replay supplies the value.
Not taking it in this PR, and the reason is cost rather than validity. The fix belongs inside crash-recovery control flow: _try_auto_vulkan_cpu_fallback builds its replay and hands it to _spawn_and_wait, and a refusal there has to choose between raising through a nested handler and returning False, which loses the actionable message. Exercising either path needs a real Vulkan startup crash, so I would be changing recovery behaviour on reasoning alone.
The failure direction is the guard refusing less on a path that is already a crash recovery, so nothing is blocked meanwhile. Flagging it as the one known follow-up on this PR rather than leaving it implied.
There was a problem hiding this comment.
Follow-up: I attempted the fix and backed it out, so the record is accurate about why it is still open.
Both respawn sites take the guard cleanly on paper. The persisted intent.cpu_fallback path is linear and already raises through _raise_terminal_load_failure, and _try_auto_vulkan_cpu_fallback already has a "not viable" return False exit that a shortfall fits. I wrote both, and the change did not break the 140 existing tests in the crash-message suite.
What I could not do is prove either one fires. Driving them needs _run_cpu_fallback_load to carry a model size and a RAM figure it does not currently take, and my attempts to reach the branches produced a fallback that was never entered and a spawn count the harness did not expect. Shipping an unverified change to crash recovery is worse than the gap it closes, so the working tree is back at this head.
The gap stands as described, in the permissive direction, and needs someone who can exercise a Vulkan startup crash or extend that harness properly.
|
@codex review |
1 similar comment
|
@codex review |
|
Codex Review: Didn't find any major issues. Another round soon, please! Reviewed commit: ℹ️ About Codex in GitHubCodex has been enabled to automatically review pull requests in this repo. Reviews are triggered when you
If Codex has suggestions, it will comment; otherwise it will react with 👍. When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback". |
mahiatlinux
changed the title
1 participant
An Ubuntu user reports Unsloth Desktop freezing and then dying with the machine.
--fit onplaces what it can in VRAM and spills the rest into host memory withno ceiling of its own, and nothing upstream of llama-server prices that spill.
The weights are memory-mapped, so an oversized remainder does not fail an
allocation: the kernel evicts and re-reads the mapping every token until the
machine stops responding, and systemd-oomd then kills the whole app slice.
The reported session loaded a 13.3 GB
gemma-4-26B-A4B-it-qatGGUF plus a1.1 GB mmproj and a 1.8 GB KV cache onto a card with 4877 MiB free, leaving
about 11 GB to run from system RAM. The server log ends mid-poll with no
shutdown line and no
parent_watchdog.parent_exited, which is what a SIGKILL ofthe whole process group looks like.
llama_cpp.pyalready refuses this on Apple unified memory(
_apple_metal_memory_budget_bytes) and on AMD APUs(
_apu_ram_shortfall_message), but_amd_apu_wants_unified_memoryis false fora discrete card, so CUDA and ROCm hosts had no guard at all.
What it checks
Weights only, against the whole free VRAM pool, read from the finished argv so
the model path is the one the child opens. That is a strict lower bound on what
the launch must hold: the KV cache, projector, drafter and compute buffers all
add to residency, and a layer or device pin only narrows the VRAM actually
reachable.
Every term left out therefore moves the estimate down, never up, so no missing
term can turn an allowed load into a refused one. The guard models no placement
at all, which is what keeps it from having to track llama.cpp's placement
surface flag by flag. It abstains on an unsized model, an unprobed GPU pool, or
unknown available RAM.
Four signals say the child reaches no GPU at all and take no VRAM credit: a
launch masked off every device, a build shipping no GPU backend, an RPC launch,
and a paravirtual Metal launch.
An earlier revision of this PR modelled the placement in full and was rewritten;
2220d79deand the comment above it record why.Testing
test_host_offload_ram_guard.pycovers the arithmetic. The placement suitedrives
load_modelend to end: the field case refuses, the same load on a largeRAM host launches, free VRAM offsets the charge, and each abstention holds. A
parametrised case asserts the floor property directly, that
-ngl 0,--mlock,--no-mmap,--device noneand--no-kv-offloadeach move bytes onto the hostor narrow reachable VRAM and so can never produce a refusal the floor would not
already reach.
Every fix in this PR carries a regression test verified to fail against the code
before it. 202 tests pass across the guard and crash-message suites, and the
wide-sweep failure set is identical to a clean tree.