[staging CI] unslothai/unsloth#8709 - #315
Conversation
llama.cpp reads "-c 0" as fit_params_min_ctx = UINT32_MAX, which pins the model's full native context and disables the reduction --fit would otherwise do. load_model already knows this and says so in a comment. On Apple Silicon no GPU is enumerated, so the Apple unified-memory cap is the only thing holding the context down, and two paths reach the command builder with a zero context after that cap has been skipped or discarded. The cap is guarded on effective_ctx > 0, so a GGUF whose metadata carries no context length is never capped at all. The `except Exception` around GPU selection restores the original request, which is 0 when context is on Auto. That throws away a context the cap had already computed, and the handler logs "using --fit on" while emitting the one argument that turns --fit off. Either way llama-server starts at the model's native length on unified memory and dies in KV or compute allocation, which is the over-commit #5118 and #6529 describe. _metal_zero_ctx_floor returns the context to start at instead, the same 4096 the cap itself falls back to when it cannot estimate KV, or the model's own length when that is shorter. It is a separate helper rather than an inline condition because the existing coverage in this area is source-string inspection, and a decision worth making is worth testing directly. Left alone: Auto-layers, which omits -c entirely and lets --fit size the context, and manual offload, where the user owns memory management including the cap. Inert off Apple Silicon, since the budget is 0 there. The floor sits above the base cmd list rather than between it and the -c emission. test_llama_cpp_no_context_shift pins those two as adjacent, and it was right to complain when the first version of this separated them. Tests: 21 new. 125 in the three suites that own this behaviour, 2257 across the llama.cpp, memory-mode, context-fit, placement and vram suites. The five video failures are present on main unchanged.
User extras are appended after Studio's own -c, and llama.cpp is last-wins
("only last value will be used"), so a pass-through "-c 0" outlived both the
new floor and the existing Apple cap and re-pinned the model's native length.
Verified against llama.cpp common/arg.cpp: the -c handler sets
fit_params_min_ctx = UINT32_MAX on value 0, which disables the context
reduction --fit would otherwise do, so "-c 0" is strictly worse than passing
no -c at all when memory is tight.
Drop a zero context override from the extras on Metal. Only zero: a positive
-c stays honored exactly as before. This matches a decision load_model
already makes, since explicit_ctx is requested_ctx > 0, so "-c 0" never
counted as an honored override and the cap already ran for it; the trailing
copy in the extras was silently undoing the cap's answer. Whatever context
Studio computed is what survives, so a model whose native length is 2048
still launches at 2048 rather than a flat floor.
Inert off Apple Silicon and on manual memory management, same scope as the
floor. Also corrects the floor docstring: the cited reports surface as
llama-server's "Compute error." at decode, not a start-time allocation death.
Tests: argv-level coverage driving the real load_model for the floor path,
the capped path, both -c spellings, a positive override, and the off-Metal
no-op, plus unit tests for the new strip helper.
![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: 7a7fa65911
ℹ️ 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".
|
|
||
| payload = { | ||
| "unsloth_auth_token": auth.access_token, | ||
| "unsloth_refresh_token": auth.refresh_token, |
There was a problem hiding this comment.
When a seeded browser session lasts beyond the 60-minute access-token lifetime, refresh fails because the frontend reads unsloth_auth_refresh_token in studio/frontend/src/features/auth/session.ts, while this script writes unsloth_refresh_token. The access token initially masks the error, but longer end-to-end runs will lose authentication instead of refreshing.
Useful? React with 👍 / 👎.
| if self._metal_drops_zero_ctx_override(ctx_override, auto_fit, gpu_memory_mode): | ||
| extra_args = strip_context_only(extra_args) |
There was a problem hiding this comment.
On Metal, an explicit extra_args=["-c", "0"] is stripped here and the mutated list is later persisted into both _extra_args and _requested_extra_args. A subsequent identical load is then compared against the original -c 0 list by _runtime_matches_intent, fails the extras equality check, and unnecessarily tears down and reloads the healthy server on every Apply. Keep the invoked extras separately while using the stripped copy only for launch.
Useful? React with 👍 / 👎.
| try: | ||
| await stop.wait_for(state="visible", timeout=timeout_ms) | ||
| except Exception: | ||
| pass # Some flows finish faster than the button appears. |
There was a problem hiding this comment.
If a response finishes before Playwright observes the stop button—a scenario the comment explicitly allows—the visible wait still blocks for the entire timeout_ms before the exception is swallowed. With the default this adds 90 seconds per fast response, and multi-turn/tool flows can exceed their CI timeout despite already being complete; use a short start-detection window or also observe completed output.
Useful? React with 👍 / 👎.
| nohup unsloth studio -H 127.0.0.1 -p 8888 > studio.log 2>&1 & | ||
| for i in $(seq 1 60); do curl -sf http://127.0.0.1:8888/healthz && break; sleep 5; done | ||
| - name: Playwright smoke (studio_test_kit) | ||
| run: PYTHONPATH=.github/scripts python -m studio_test_kit._smoke_ui || true |
There was a problem hiding this comment.
In the inspected staging-8709-playwright.yml workflow, || true converts every smoke-test assertion, browser-launch error, and transcode failure into a successful step. The later artifact upload already has if: always(), so swallowing the exit status is unnecessary and leaves this CI workflow green even when the test it was added to run is completely broken.
Useful? React with 👍 / 👎.
| # Only consider webms that did not exist when we started. | ||
| new_webms = sorted( | ||
| p for p in video_dir.glob("page@*.webm") | ||
| if p not in pre_existing | ||
| ) |
There was a problem hiding this comment.
When two open_chat contexts overlap in the same video_dir and both take their snapshots before either recording file appears, each context considers both eventual page@*.webm files new. Whichever closes first can therefore rename the other session's recording and delete its own as a supposed stale extra; the generated session_token is never used to disambiguate them. Capture the page's actual video path or isolate each session in a unique directory before renaming.
Useful? React with 👍 / 👎.
2 participants
Disposable CI run for unslothai/unsloth#8709. Do not merge; closed after CI.