Honor user tensor_split when auto tensor-parallel planner returns an even split - #10884
Honor user tensor_split when auto tensor-parallel planner returns an even split#10884chakshu-dhannawat wants to merge 13 commits into
Conversation
…ven split In auto tensor-parallel mode the planner can decide the model fits evenly across GPUs and return tp_tensor_split=None. Previously the backend then omitted --tensor-split entirely, so a user-supplied per-GPU ratio was silently ignored. Fallback to the user-supplied tensor_split in that case, after sanity checking that the number of entries matches the active GPU count and that the total is non-zero. Regression test included. Fixes unslothai#10355
![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: 716d8a6e6a
ℹ️ 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".
| ] | ||
| ) | ||
| _emitted_tensor_split = ",".join(str(int(x)) for x in tp_tensor_split) | ||
| elif gpu_memory_mode != "manual" and tensor_split: |
There was a problem hiding this comment.
When the auto planner returns None, it has established only that an even split fits; it has not validated the caller's ratio. For example, on two cards where the model/context consumes about 60% of each card under the even plan, a valid [3, 1] ratio passes this gate and assigns roughly 75% of the weights to one card, causing llama-server startup to OOM. Re-plan or reject a custom ratio against each selected GPU's usable budget rather than forwarding it after an even-only plan.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Correct, and fixed. 0492a22 added _tensor_split_fits_budget, which re-checks the requested ratio against each selected GPU's usable budget before the fallback forwards it. f7b93a0 then brought that check to parity with the planner's own arithmetic: it was missing the per-device compute-buffer term the planner charges, so a ratio the planner would have rejected could still pass the gate, and an index missing from the free-memory map now fails closed instead of being treated as unbounded. Covered by studio/backend/tests/test_pr10884_auto_tensor_split_sim.py.
| _emitted_tensor_split = ",".join( | ||
| f"{x:g}" for x in _sanitized_split | ||
| ) | ||
| if _emitted_tensor_split is not None: | ||
| cmd.extend(["--tensor-split", _emitted_tensor_split]) |
There was a problem hiding this comment.
This makes tensor_split affect an auto launch, but auto mode still clears _tensor_split and _runtime_matches_intent compares the split only in the manual branch. Consequently, after an auto tensor-parallel load with [3, 1], a request for the same model/settings with [1, 3] is adopted as already loaded and never respawns, leaving the old distribution active. Store and compare the emitted auto ratio (or otherwise include it in the auto intent matcher).
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Correct, and fixed. 0492a22 added _auto_tensor_split plus an auto-mode arm in _runtime_matches_intent, so a changed ratio no longer adopts the resident server. f7b93a0 corrected what gets recorded: the field held the ratio that was EMITTED, which is the planner's ratio whenever the planner produced one, so a later request carrying the user's own ratio compared unequal to it and every identical repeat reloaded forever. It now records the requested ratio, normalized, and the matcher compares the same normalization (3,1 and 75,25 are one intent to llama.cpp). Hardware evidence on two T4s in #10884 (comment): an identical repeat reuses pid 2730, a reversed ratio respawns as pid 2769 with --tensor-split 1,3.
for more information, see https://pre-commit.ci
… dedupe - Budget validation: when the auto tensor-parallel planner returns an even split (None) and the user supplied a ratio, check the ratio against the same per-GPU usable budget the planner uses. If it would overshoot a GPU, drop the ratio instead of forwarding it to llama-server. - Reload matching: record the emitted auto-mode ratio in _auto_tensor_split and compare it in _runtime_matches_intent so a later request with a different ratio does not silently reuse the wrong split. - Add regression tests for both behaviors.
|
Pushed updates for both review points:\n\n- The user fallback ratio is now validated against the same per-GPU budget the planner uses, so an overshoot is dropped instead of forwarded.\n- Auto mode records the emitted ratio and compares it in reload deduplication, so a later request with a different ratio does not silently reuse the wrong split.\n\nAdded regression tests for both. Let me know if anything still looks off. |
for more information, see https://pre-commit.ci
|
Confirmed this hits the path in studio/backend/core/inference/llama_cpp.py where auto tensor-parallel emits --split-mode tensor without --tensor-split, so a user ratio never reaches llama-server, and the budget check plus reload matching you added both look right. Will get this reviewed. |
… loop, budget parity The fallback itself is right and unslothai#10355 reproduces exactly as described: on two equal cards with a model that fits, the planner returns no ratio of its own and the user's --tensor-split never reached llama-server. Four problems in how it was wired, each reproduced against 0d9952f and each covered by a new test. 1. load_model raised instead of degrading. The placement price lives in one long try whose except arm is a designed fallback: it drops the plan, sets --fit on and launches anyway. It clears tp_tensor_split but deliberately not tensor_parallel, so it steered straight into the new elif with gpu_indices either None or rebuilt from every device rather than the ones the reserve filter admitted, and with tp_gpus and the MTP terms possibly unbound. That is KeyError, TypeError and UnboundLocalError out of load_model on inputs that launched fine before, on Vulkan (AMD and Intel), ROCm and CUDA alike. Gated on _tp_planned, which is now bound before the try like the other locals that arm restores, and _tensor_split_fits_budget fails closed on a device the survey does not cover. 2. Identical requests reloaded forever. _auto_tensor_split recorded what was EMITTED, and the matcher compared it against what was REQUESTED. Those are not the same units: the planner's own split is a list of per-device MiB and a ratio the budget check declines is emitted as nothing at all. So an ordinary multi-GPU auto tensor load with no user ratio at all mismatched on every repeat and tore down a multi-gigabyte model, forever, since the next launch recorded the same thing again. Record a normalized fingerprint of the request, per the requested-against-requested rule the rest of the matcher already uses, which also makes 3,1 and 75,25 the same intent as they are to llama.cpp. 3. The budget check was not the planner's budget, in both directions. It charged the replicated context buffer twice, declining ratios the planner itself would accept, which to a user who typed one reads as the ratio being ignored again. It also omitted the planner's soft overhead and priced the dequant scratch from the heavier half of an asymmetric K/V pair, which is optimistic - and a tensor load has no --fit valve to spill into. 4. The API contract still said "Manual mode only ... Ignored unless gpu_memory_mode is 'manual'", which this change makes false. Also emit the ratio in plain decimal so a legal 1000000,1 does not reach the log and the argv as 1e+06,1, and clear the recorded ratio on the manual arm beside the arch-gate resets that are there for the same reason. tests/kaggle/studio_gpu/run_studio_gpu.py gains the real-hardware half: assert_auto_tensor_split reads the live llama-server argv from /proc, because /api/inference/status reports tensor_split: null on this path whatever the child was launched with, and drives the reuse-and-reload pair that a fix can regress. --base-sha runs the same probe against the merge base first, in the same session with one file swapped, and calls the comparison void unless the base reproduces the defect.
…ded nothing Measured on kernel unsloth-t4-ci-d15ea193: two real T4s, --tensor-split 3,1 on the live argv, reuse and reload both correct, and the assertion still red because Qwen3-0.6B spent all 16 permitted tokens inside <think> and came back with an empty content and finish_reason length. Read both channels, count the usage, and give the model room to finish a sentence.
|
Tested this on real hardware and pushed fixes directly to the branch. Summary: the bug you are fixing is real and reproduces exactly as you describe, the approach is right, but the wiring had four problems that I have now fixed on top of your commits. The defect is realReproduced on two real Tesla T4s in a Kaggle session, auto mode, Read off the live What I changed
1. 2. Identical requests reloaded the model forever. That is a path that worked before this PR. Fixed by recording a normalised fingerprint of the request, which is the requested-against-requested rule the rest of the matcher already uses, and which also makes 3. The budget check was not the planner's budget, in both directions. It charged the replicated context buffer twice, once inside the distributed total and again per device, so it declined ratios the planner itself would accept. To a user who typed one, that reads as the ratio being ignored all over again. It also omitted the planner's 4. Small things. The ratio is emitted in plain decimal now, so a legal EvidenceReal 2x T4, Unsloth Studio installed the supported way (
Simulation matrix, 44 cases driving the real backend against faked inventories: CUDA and Vulkan, 0 to 4 cards, asymmetric cards, drivers that report no total (iGPU), paravirtual Metal, degenerate ratios (wrong length, zeros, negatives, NaN, inf), the planner's recovery arm, and the full reuse and reload matrix. Full Studio backend suite, 41,258 tests, compared against the merge base run on the same box. Identical failure sets apart from one load-sensitive flake in I also added Two things left for you to weigh in on
Nice find on the original issue, and thanks for the fix. With the above it looks good to me. |
- Add _auto_tensor_split_emitted to record the normalized ratio actually emitted to llama-server in auto tensor-parallel mode. - Update the tensor_split property to fall back to that emitted ratio, so /status no longer reports null for a server running a concrete split. - Reset the new field alongside the other split state. - Update the LoadStatus tensor_split description to remove the manual-only wording. - Add assertion that backend.tensor_split surfaces the emitted ratio.
|
Follow-up push addresses the /status reporting gap danielhanchen flagged:\n\n- Added _auto_tensor_split_emitted to capture the normalized ratio actually passed to llama-server in auto tensor-parallel mode.\n- The tensor_split property now falls back to that value, so /status and the /load response no longer report null for a server that is running a concrete split.\n- Updated the LoadStatus field description to drop the manual-only wording.\n\nTests still green: 236 placement + simulation tests pass, ruff clean. |
|
Cross-platform CI on the fixed branch, run off the org queue so the platforms this change does not touch are exercised too. All five green:
Worth stating what that does and does not cover, since only Linux has a GPU here. The change is backend Python only: no frontend file, no Tauri resource, no API schema change, and |
Reporting the auto ratio is the right call, but it feeds one comparison that was written when auto could only ever report null. resident-config-match's split rule is unconditionally pinned and compares the store's splitRatio against status.tensor_split, and applyInferenceStatusToStore clears splitRatio unless the mode is manual. So an auto tensor-parallel server now compares a field the applier cleared against a ratio the planner legitimately chose, disagrees, and declines to adopt a resident model that is exactly what was asked for. That is the reload loop from the backend fix reappearing one layer up, reached through the UI rather than the API. Judged on the mode the RESIDENT server ran, not the one this pick would send: the config's own mode cannot tell a manual load whose custom ratio the config forgot from an auto load carrying the planner's. A server too old to report gpu_memory_mode is still compared, so nothing that used to reload stops reloading, and the existing custom-split case still reloads.
|
Reviewed and tested One thing it does reach, which I have fixed in
Judged on the mode the resident server ran, not the one the pick would send. The config's own mode cannot tell a manual load whose custom ratio the config forgot from an auto load carrying the planner's, and a server too old to report Worth noting for whoever reviews next: the |
|
Pulled your frontend fix and re-ran everything:\n\n- Backend: 236 placement + simulation tests pass, ruff clean.\n- Frontend: 356 resident-config-match tests pass.\n\nThanks for catching the resident-config-match loop. Keeping _auto_tensor_split_emitted separate from _tensor_split looks like the right call since it avoids touching manual dedupe and VRAM accounting. |
…t this one
The sibling simulation file varies the ratio, the card count and the planner's
answer, all on whatever host happens to run it. This varies the host: the
Cartesian product of {Linux, Windows, WSL, macOS} x {NVIDIA, AMD ROCm, AMD or
Intel via Vulkan, CPU only}. The code the fallback sits in reads sys.platform,
_is_wsl and torch's ROCm markers on the way to a launch, and the crash it used
to cause was first seen on the Vulkan route, so a table that only ever asks
one host is not evidence about the others.
Simulated at the level the production code actually reads: sys.platform and
_is_wsl for the OS, a fake torch carrying or not carrying version.hip for the
vendor (which is exactly what _host_torch_is_rocm looks at), and the
(index, free, total) rows _get_gpu_memory returns for the cards. No torch is
imported, so the whole file runs in a bare venv in about five seconds.
The failed-plan case uses three cards with one below the tensor-parallel
compute-buffer reserve, because that is what makes the recovery arm's rebuilt
gpu_indices wider than the tp_gpus the planner filtered. Written first with two
equal cards, where it passed against the unfixed revision and proved nothing;
with the third card it fails on all twelve GPU hosts there and passes here.
The device-pin case asserts the AGREEMENT between the ratio and the pin rather
than a spelling, since each vendor pins through a different channel and this
change touches none of them: NVIDIA masks with CUDA_VISIBLE_DEVICES, ROCm on
Linux and WSL masks at the ROCr layer and re-indexes the CUDA mask, Windows has
no ROCr so it keeps the HIP mask, and Vulkan does not mask at all but pins
--device VulkanN.
Stated in the file, and worth repeating: this proves the placement decision,
the argv and the mask on each host. It does not prove a real ROCm or Metal
driver then behaves, and sys.platform cannot make the stdlib's Windows paths
work on Linux, which is why the reuse case stubs the capability probe. The real
Windows and macOS answers are the runners; the real GPU answer is the T4s.
The change alters what a load does with a tensor_split it is handed, so the question for an install that already exists is narrower: can anything already on its disk start handing it one? Saved per-model settings are one JSON row in app_settings, and the only things that reach a load from there are what normalize_model_override allow-lists in and what model_override_load_kwargs emits out, so the upgrade question is answerable exactly and without a database. It cannot. A ratio is not in the allow-list, and writing one into the row by hand -- which is possible, it is JSON in sqlite -- is dropped rather than forwarded. Measured against the merge base as well as asserted here: the same saved override replays byte-identical load kwargs on both revisions. Forwards compatibility is the same argument backwards. Nothing new is persisted, so a row written by this version is what an older one already understands, and the normalizer is idempotent, so an install that upgrades and rolls back does not find its settings rewritten.
|
Confirmed against llama_cpp.py, where auto tensor-parallel emits --split-mode tensor but drops the user ratio when the planner returns nothing, and the follow-up budget validation plus reload matching both look right. Will get this reviewed. |
|
Closing out the coverage gaps I left open earlier. Three additions, all pushed. 1. An isolated sandbox, and it needs no torch. The whole simulation set runs in a bare 2. Every supported host, not just the one running the tests. The first version of the crash case used two equal cards and passed against It also pins that the ratio and the device pin agree on each host, since each vendor pins through a different channel and this change touches none of them: NVIDIA masks with 3. What an existing install does on its first load after updating. Answerable exactly, because saved per-model settings are one JSON row and the only things that reach a load from there are what On browsers
What is still outstandingThe re-staged cross-platform run on |
The argv is ground truth but a user cannot read /proc, and for the whole life of the auto path the status property returned the manual-mode field, which auto never writes -- so a server launched with --tensor-split 3,1 reported tensor_split: null and no client could tell a forwarded ratio from a dropped one. The GPU leg now reads both and fails when they disagree, which is what makes the /status change answerable on two real cards rather than only in a unit test.
|
Pulled the latest additions and ran them locally:\n\n- test_llama_cpp_placement.py + test_pr10884_auto_tensor_split_sim.py + test_pr10884_platform_vendor_matrix.py + test_pr10884_upgrade_compat.py: 301 passed\n- resident-config-match tests: 356 passed\n- ruff check: clean\n\nGood to know the simulation set runs without torch. Let me know if the queued cross-platform run surfaces anything. |
|
Re-ran the GPU leg on two real Tesla T4s against the current head
The base leg still reproduces the defect, so the comparison is not void. The This covers the head that includes the |
|
Great to see the T4 run confirm the API now matches the child's argv. Pulled e2bbd7e and the local suite still passes (301 backend tests, ruff clean). |
|
@codex review |
|
@codex security review |
Codex Review SummaryThis comment shows the latest Codex review activity on this pull request.
ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
Codex reacts with 👀 while any review is running, comments if it has suggestions, and reacts with 👍 once all reviews finish with no findings. |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: e2bbd7e23b
ℹ️ 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".
| self._auto_tensor_split_emitted = self._auto_split_fingerprint( | ||
| _emitted_values | ||
| ) |
There was a problem hiding this comment.
When an auto tensor-parallel startup hits the unsupported-GPU arch-crash retry, _without_tensor_split(cmd) removes --tensor-split from the retried command, but that path only clears the legacy _tensor_split field. This newly recorded _auto_tensor_split_emitted therefore survives and makes /status report a ratio the live child is not using; if recovery leaves one GPU, it can even report a split while tensor_parallel is false. Clear or recompute this field whenever a recovery rewrites the emitted split.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Correct, and fixed in 102e6d6. Reproduced first: three cards with the third below the tensor-parallel reserve, so the planner pins two and the arch-crash retry has somewhere to narrow to. Before the fix the retry left the argv with no --tensor-split while the property answered [0.75, 0.25] with tensor_parallel false, exactly as described. The proactive arch gate is not reachable for this field, since its arm requires gpu_indices to be None and the auto fallback only emits when gpu_indices is truthy, so the clear goes on the reactive retry alone. The REQUESTED fingerprint is deliberately kept: the next Apply re-sends the same ratio and would be gated down here again, so reloading on it would rebuild the server on every Apply. Regression tests: test_the_arch_crash_retry_stops_reporting_the_ratio_it_dropped and its non-crashing control in studio/backend/tests/test_pr10884_auto_tensor_split_sim.py.
| rendered = _render(values) | ||
| if values and all(part == "0" for part in rendered): | ||
| peak = max(values) | ||
| if peak > 0: | ||
| rendered = _render(v / peak for v in values) |
There was a problem hiding this comment.
For a valid ratio containing values on opposite sides of the six-decimal rounding threshold, such as [4e-7, 6e-7], this renders 0,0.000001. Because the rescaling branch runs only when every rendered part is zero, the requested 40:60 distribution becomes 0:100 and one GPU receives no model share. Rescale whenever a positive input is rounded to zero, or use a non-lossy decimal representation.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Not taking this one. The reading of the code is right: [4e-7, 6e-7] does render 0,0.000001, because the rescale only fires when every part renders zero. It is not reachable on real input, though. A tensor split is scale free by construction, llama.cpp normalizes the list before use, so any ratio a user can mean has an equivalent expression at any magnitude and only weights below 5e-7 lose anything here. The all-zero case is guarded because it is the one that makes llama.cpp divide by a zero total; a ratio whose largest weight is under a millionth is not a placement anyone asks for, and widening the rescale would change the emitted text for no input a session produces.
| agrees: (_c, s, standing) => | ||
| s.gpu_memory_mode === "auto" || sameList(standing.splitRatio, s.tensor_split), |
There was a problem hiding this comment.
This unconditionally treats every auto-mode resident split as agreeable, but the outgoing load path sends standing.splitRatio even in auto mode and switching the GPU memory mode does not clear that store field. Thus, after setting a manual ratio and switching to Auto, selecting an already-resident auto model can skip /load even when the resident uses a different ratio, bypassing the backend's new auto-ratio matcher and silently discarding the pending placement change. Ignore the status split only when the pending ratio is actually absent.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Correct, and fixed in 102e6d6. Confirmed the premise in the code rather than by argument: applyInferenceStatusToStore keeps prevState.splitRatio whenever preserveSameModelEdits and gpuMemoryEditsPending hold, and gpuMemoryEditsPending is true precisely when the mode knob has been moved off the loaded mode, so a ratio set under Manual does survive the switch to Auto; and use-chat-model-runtime sends stateBeforeUnload.splitRatio with no mode gate. The rule now reads (s.gpu_memory_mode === 'auto' && standing.splitRatio == null) || sameList(...), so a cleared store still adopts a planner ratio while a pending one is still compared. Both directions are pinned in studio/frontend/tests/resident-config-match.test.ts; the reload case fails on the previous line and passes on this one.
…atio a reload The arch-crash retry re-masks the child onto a narrowed device set and takes --tensor-split off the argv, but only cleared the manual field, so the status route kept answering with a ratio the live child did not have, and beside tensor_parallel false when the narrowing left one GPU. The picker's side of the same field: applyInferenceStatusToStore preserves a pending splitRatio across a status apply, and the load path sends it in either mode, so adopting on the resident mode alone discarded a placement change the backend now honours in auto.
|
@codex review |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 102e6d6cd2
ℹ️ 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".
| scratch_cache_type_kv = _scratch_cache_type_kv, | ||
| ) | ||
| ): | ||
| _emitted_tensor_split = self._format_tensor_split(_sanitized_split) |
There was a problem hiding this comment.
When an auto tensor-parallel load of Gemma 3n (or another architecture that rejects weighted tensor splits) fits evenly and the user supplies a non-even ratio such as [3, 1], this fallback replaces the planner's deliberate None with a weighted --tensor-split. _plan_tensor_parallel explicitly leaves the split unset in this case because Gemma 3n can hit a GGML_ASSERT on weighted splits, so the first server startup now crashes and is downgraded to layer splitting instead of retaining the safe tensor-parallel launch; suppress the user fallback for architectures known not to support weighted splits.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Not taking this one: the premise about the planner does not hold. _plan_tensor_parallel decides the split on capacity alone, if even_share_mib > (min_usable_mib - reserve_mib), with no architecture in the condition. The docstring line about Gemma 3n describes why the None case happens to be safe for every arch, not a rule the planner is enforcing, and the proof is the other branch: on uneven cards the planner emits a WEIGHTED split for Gemma 3n itself, so there is no arch guard here for this fallback to preserve. Manual mode has always forwarded a ratio for any architecture too, so suppressing it only in the auto fallback would make the same request behave differently in the two modes. The abort is already handled where it belongs and for all three paths: _should_record_tensor_split_abort catches the GGML_ASSERT, _record_tensor_split_abort memoizes it per (binary, model, cache pair), and the next load skips tensor mode up front and keeps the multi-GPU request on a layer split.
|
@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: 102e6d6cd2
ℹ️ 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".
| total_weight = sum(split) | ||
| for i, idx in enumerate(gpu_indices): | ||
| alloc_bytes = total_bytes * split[i] / total_weight | ||
| capacity_bytes = (usable_by_idx[idx] - reserve_mib) * 1024 * 1024 |
There was a problem hiding this comment.
For an uneven user split at a large context, this treats the aggregate cc_bytes as though --tensor-split distributes it by the requested ratio. _plan_tensor_parallel documents and handles this buffer as replicated on every selected GPU, so a low-weight device is charged less than its mandatory full context buffer here. If that device has enough VRAM for the flat reserve but not the full context-linear buffer, the check accepts the ratio and launches tensor mode with --fit off, causing startup OOM. Subtract one per-device context-buffer allocation from every device's capacity, then weight only the remaining footprint.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Correct, and fixed in 2957ab0. The check now subtracts one per-device context buffer from each card's capacity and weights only the remaining footprint, which is the planner's own arithmetic: its even-share gate compares (model + kv + mtp + cc_bytes)/n against min_usable - reserve, and since cc_bytes is n per-device buffers that is identical to charging the buffer flat, while its weighted branch subtracts cc_per_dev_mib from every card before ranking. Weighting the aggregate matched only at an even ratio. Worth recording that the error ran both ways: the low-weight card was under-charged as you describe, and the high-weight card was charged close to twice the buffer, so a ratio the planner itself would have accepted was refused, which is the original complaint again. The new cell test_the_context_buffer_is_charged_flat_not_by_the_ratio pins the 1:9 case that the aggregate form refused and the planner accepts; it fails on the previous revision.
…heck Every device allocates the whole context-linear compute buffer whatever weight it carries, which is why the planner subtracts one per-device slice from each card before weighting. Distributing the aggregate by the ratio is the same arithmetic only at an even share: away from it the low-weight card is charged less than the buffer it must allocate and the high-weight card nearly twice it, so a ratio the planner's own rule accepts was refused.
|
@codex review |
1 similar comment
|
@codex review |
|
Codex Review: Didn't find any major issues. Nice work! Reviewed commit: ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
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". |
2 participants
In auto tensor-parallel mode, the planner can decide the model fits evenly across GPUs and returns
tp_tensor_split=None. The backend then emitted--split-mode tensorbut omitted--tensor-split, so a user-supplied ratio was silently ignored.This change falls back to the user-supplied
tensor_splitwhen the planner emits nothing, after checking the entry count matches the active GPU count and the total is non-zero.Includes a regression test.
Fixes #10355
UI evidence
The PR touches one line of
studio/frontend/src/features/chat/lib/resident-config-match.ts, so the question a reviewer asks is whether the chat behaves differently. Net of the merge base it does not, and that is what this pair shows: two isolated Studio installs, BEFORE built from the merge base0d9952f8and AFTER from the head, each loaded with the same GGUF in auto GPU-memory mode, then driven through the model picker to select the model that is already resident.Both halves show the same chat:
Qwen3-0.6B-GGUF / GGUF - Q4_K_Mloaded in the header, the context meter at24 / 4.1k, and the composer enabled. The claim behind the picture is a measurement rather than the pixels, since an adoption and a reload end up looking the same: thellama-serverpid running before the selection is the same pid after it on both sides, so the picker adopted the resident server instead of tearing it down. Scene facts came out identical on both halves:status_gpu_memory_mode auto,status_tensor_parallel false,status_tensor_split null,server_count 1,resident_server_survived_pick true.Scope of this pair: the changed line's tensor-split arm needs a real tensor-parallel load across two or more devices, and the host that took these shots has one GPU visible, so the line is evaluated on both builds with the split still absent on both. The tensor-parallel behaviour itself is evidenced on two real T4s in the Kaggle run linked in the comments, and pinned in
studio/frontend/tests/resident-config-match.test.ts.