Studio: hold the Auto-layers context exemption only while --fit runs - #8825
Merged
Conversation
Follow-up to #8709. Auto-layers earns its exemption from the Metal context floor by leaving the context to --fit, so the exemption is only sound while --fit is actually going to run. User extras land after Studio's own --fit on and llama.cpp is last-wins, so a pass-through --fit off produced a command carrying neither a -c nor a fitter, which is llama.cpp's native context and the same over-commit the floor exists to prevent (#5118, #6529). Gate the exemption on fit_is_effectively_on over the extras, reusing the helper in llama_server_args.py rather than adding a second definition of what turns the fitter off. Extras alone are the right input because Studio's own --fit on already beats an inherited LLAMA_ARG_FIT, argv being parsed after the environment. Driving load_model on Metal in Auto-layers, before and after: --fit off [] -> ['4096'] --fit off -c 0 [] -> ['4096'] --fit on [] -> [] no extras [] -> [] --fit off, non-Apple [] -> [] A caller-owned fixed layer count is untouched.
for more information, see https://pre-commit.ci
Member
Author
|
@codex review |
|
Codex Review: Didn't find any major issues. Can't wait for the next one! 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". |
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
1 participant
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.
Follow-up to #8709, which landed while this last case was still being fixed.
The problem
#8709 stops Metal ever being sent
-c 0, because llama.cpp reads it asfit_params_min_ctx = UINT32_MAX, which pins the model's full native context and disables the reduction--fitwould do (#5118, #6529).Auto-layers (
gpu_memory_modemanual with a negative layer count) is exempted from that floor, and correctly so: it deliberately omits-cand lets--fitsize the context. But that exemption is only as good as the fitter it defers to. User extras are appended after Studio's own--fit onand llama.cpp is last-wins, so a pass-through--fit offleaves a command carrying neither a-cnor a fitter. llama.cpp then falls back to the model's native context, which is the same over-commit the floor exists to prevent.--fitis not on the pass-through denylist, so this is reachable from the extra-arguments box.The fix
Gate the exemption on the effective fit state: the floor now receives
auto_fit and fit_is_effectively_on(extra_args). That reuses the helper already inllama_server_args.pyrather than adding a second definition of what turns the fitter off.Extras alone are the right input here. Studio's own
--fit onalready beats an inheritedLLAMA_ARG_FIT, since llama.cpp applies the environment before parsing argv, so only the trailing extras can win.Evidence
Driving the real
load_modelon Metal in Auto-layers, context values in the emitted argv, before and after:--fit off[]['4096']--fit off -c 0[]['4096']--fit on[][][][]--fit off, non-Apple[][]A caller-owned fixed manual layer count still emits
['0']untouched, since there the user owns the memory budget.Tests
New
TestAutoLayersWithTheFitterTurnedOffcovers all five rows at argv level. 2254 tests pass across the llama.cpp, Metal, paravirtual, context and launch-flag suites.