[staging CI] unslothai/unsloth#7670 - #295
Open
danielhanchen wants to merge 46 commits into
Open
Conversation
… built archs (#7624) The free-memory rank could pick a device the installed llama.cpp binary has no kernels for (e.g. an iGPU reporting shared system RAM outranking the dGPU), crashing llama-server with 'device kernel image is invalid'. - install_llama_prebuilt: record gfx_target + mapped_targets in UNSLOTH_PREBUILT_INFO.json for published ROCm bundles - llama_cpp: drop devices whose gcnArchName is not in the recorded list before ranking; unknown coverage or unknown device arch fails open
…nel image is invalid' crash (#7624) Custom-linked llama.cpp builds have no mapped_targets manifest, so the proactive arch gate cannot see them. Catch ROCm's arch-mismatch crash on an auto-pinned device and respawn once with that device excluded, letting an arch-compatible sibling (e.g. the dGPU next to an unsupported iGPU) take the load. Explicit user device picks keep their error.
for more information, see https://pre-commit.ci
- _installed_llama_gfx_archs now reads the install marker via llama_cpp_freshness.read_install_marker (walks up from the resolved binary), replacing a hand-rolled root list that missed custom studio homes and duplicated the freshness util - extract _rocm_arch_by_physical_id from _amd_apu_wants_unified_memory and share it with the enumeration gate instead of a second inline gcnArchName loop
for more information, see https://pre-commit.ci
Merge conflict was in studio/backend/core/inference/llama_cpp.py, where main has since grown unified-memory APU accounting in the same torch fallback loop this PR filters in. Both behaviours are kept: a shared-pool device still gets its host-RAM reserve and reports total 0, and an unsupported device is still dropped. Review items addressed: - The arch gate is now opt-in (_get_gpu_memory / _get_gpu_free_memory take for_llama_server). It answers "what can llama-server run on", not "what GPUs exist", and the same probe backs core/rag/embeddings.py::_resolve_auto, which picks sentence-transformers (PyTorch). A device the installed llama.cpp prebuilt lacks kernels for is usually still a fine torch device, so gating that probe would have pushed RAG embeddings onto the CPU. The llama-server call sites opt in: automatic placement in load_model, and the GGUF embedding backend's own GPU check. - _rocm_arch_by_physical_id reads the arch through the training worker's classifier, so the alternate spellings AMD SDK / Radeon wheels use (gcn_arch_name, arch_name, gfx_arch_name) are covered as well as the canonical gcnArchName. Reading only the canonical name left the map empty on those wheels and failed the gate open. - The ROCm check uses the shared _torch_is_rocm predicate instead of a bare torch.version.hip test, so AMD SDK wheels that leave version.hip unset are gated too. - The kernel-image-invalid recovery set moved into _arch_crash_retry_gpu_ids. It still prefers never-selected devices, and when the failed launch used every enumerated GPU it narrows the selection by dropping the unified-memory devices instead of giving up. - _installed_llama_gfx_archs no longer lets a marker read raise: it runs inside the GPU probe, where an exception would have dropped every device. tests/test_gpu_arch_gate_7624.py grew coverage for the torch-caller path, the arch attribute fallback, the broader ROCm wheel predicate, the APU reserve and the gate coexisting, and the retry set.
…#7624) mapped_targets is remote data: it comes from llama-prebuilt-manifest.json, which the release pipeline versions independently of this code. The gate is exact-set membership against the arch a device reports, so a token no device can ever report matches nothing and drops EVERY GPU, silently forcing CPU. That is worse than not gating at all, and it is reachable today from a malformed marker ([null] and [123] both produce such a set) as well as from a future publish recording a ROCm generic code object (gfx11-generic) or an umbrella family label (gfx110X). Treat an arch list that is not entirely concrete gfx tokens as unknown and keep every device. All-or-nothing on purpose: dropping just the odd token out of [gfx1100, gfx11-generic] would under-report what the build covers. Also pin the compatibility contract that had no coverage: a marker corpus (absent, empty, invalid, truncated, list-not-dict, null/string/dict/int targets, unreadable, directory-in-place-of-file, huge list) that must resolve to a sane arch set or None and never raise; that the new keys leave the install fingerprint byte-identical, so existing installs do not refresh on upgrade; that non-ROCm bundles record no archs; that the marker rewrite path preserves the fields; and that the family-label match still records the concrete list.
The gate returning an empty pool left gpu_indices None, so the launch took the --fit on arm, the pin block never ran, and no HIP/ROCR/CUDA mask was written. The child still enumerated every unsupported card and died exactly as #7624 reports, and the reactive arch-crash retry could not help because its guard needs a truthy gpu_indices. Mask the child onto the CPU instead and say why, on auto placement on a ROCm host only. Also: - the arch-crash retry enumerates _detected_gpus, not gpus, matching what the region's own comments say to use; - the retry withdraws GGML_CUDA_ENABLE_UNIFIED_MEMORY when it moves to a discrete card, but only when this launch set it, so a user value stands; - test_single_gpu_host_has_no_retry counts probe calls instead of raising: the narrowing branch runs under except Exception, which swallowed the raising spy and left the test green with the guard removed; - correct the justification for gating automatic placement only.
for more information, see https://pre-commit.ci
…retry (#7624) Two ways the gate stopped short of the process it was protecting. The embedding llama-server probe reduced the gated device list to a bool, so on a mixed host (unsupported iGPU plus supported dGPU) the gate passed on the dGPU and the child still enumerated every ROCm agent -- and that HSA enumeration is what dies on an arch the prebuilt has no kernels for. Pin the survivors with the ROCr-layer mask instead. Inert unless the install marker reports concrete mapped_targets AND the gate actually narrows the set, so NVIDIA, CPU-only, Vulkan and macOS write no mask and skip the probe entirely. The arch-crash retry masked devices out but respawned the planned argv verbatim. --tensor-split shares are positional over the child's visible devices (llama.cpp parses them by index, then copies the first n_devices), so the narrowed set re-indexes the survivors and hands them the crashed set's weights, which can overcommit a card. Drop the flag for the respawn and fall back to llama.cpp's free-VRAM split, which is correct for any device set.
for more information, see https://pre-commit.ci
…624) Two more places the gate stopped one step short of the child process. When the gate narrows a pool without emptying it, and the model is then too large for the planner, _select_gpus returns (None, True) and gpu_indices stays None. No branch in the env block writes a visibility mask on that arm, so the child enumerated the very card the gate had just dropped and died on it, and the reactive retry could not help either since its guard needs a truthy gpu_indices. That is the same reasoning the forced-CPU branch already spells out, one card short of forcing CPU. Mask the survivors in, last in the chain so a manual --tensor-split sized over the full visible count is not re-indexed under it. The probes behind it are lazy, so an ordinary pinned launch still pays for neither, and _arch_gate_survivors now backs the embed pin too. A forced-CPU launch also left _gpu_offload_active at None, because the gated probe leaves the detected list empty and the counted classifier answers None there. routes/training_vram.py spares a server only on 'is not False', so a server masked onto the CPU with no device visible at all was unloaded before training even though its death frees no VRAM. Record it as False, the contract the neighbouring comment already states.
for more information, see https://pre-commit.ci
…fallback (#7624) The manual per-GPU ratio took its own env branch, which re-emits the WHOLE visible GPU order, so the arch gate's answer was thrown away and the child got handed the card the build has no kernels for. Manual memory mode is not an explicit device pick: the probe already opts into the gate on 'not gpu_ids', and the ratio was sized from the full enumeration, not chosen per device. Fold that arm into the survivor branch: mask the survivors, and drop the ratio, which was sized over the full visible count and cannot survive the re-indexing that masking causes. Same trade the arch-crash retry already makes. A host the build fully covers keeps both its ratio and its PCI order pin. The forced-CPU branch could also be reached with --split-mode tensor still in the argv. Manual mode admits tensor parallelism on the full torch device count, which still counts the cards the gate dropped, so neither the <2-GPU manual guard nor the auto planner's zero-GPU guard fires. llama.cpp's llama_prepare_model_devices then fails the load with 'LLAMA_SPLIT_MODE_TENSOR needs >= 1 devices' and llama-server exits 1, turning the CPU fallback into a server that refuses to start. Strip the split flags there. Scoped to the gate: the zero-offload arm already drops Studio's own flags, and a user --split-mode there is deliberately overridden rather than stripped.
…7624) The embed server's CPU arm only blanked CUDA_VISIBLE_DEVICES. HIP consults that variable only when HIP_VISIBLE_DEVICES is unset, and clr treats an empty value as no mask at all, so on ROCm the blank mask hid nothing and a load that had chosen the CPU still handed the child devices and the VRAM their contexts cost. Set the -1 sentinel at the HIP layer as well. An inherited ROCR mask is deliberately left alone: it hides agents below HIP, so clearing it would expose more of them to the HSA enumeration that dies on an uncovered arch. GGML_CUDA_ENABLE_UNIFIED_MEMORY is decided against gpu_indices, which is None on both arms the survivor mask serves, so an uncovered APU anywhere on the host turned it on and the narrowing then handed the child only discrete cards, where the same code calls the setting harmful. Withdraw it there, with the arch-crash retry's ownership check, so only a value this launch set is taken back and a deliberate user one stands.
…#7624) The respawn only handled one direction of each transition. A markerless mixed host can crash on the discrete card and land on the APU, where the first launch correctly left GGML_CUDA_ENABLE_UNIFIED_MEMORY unset, so the retry now sets it as well as withdrawing it. And narrowing to a single device makes --split-mode tensor a no-op that was still reported as active through the tensor_parallel property, which drives the UI and the MTP tensor watchdog.
The unified-memory RAM guard runs once, against the selection the first spawn used. On the mirror shape -- crash on the discrete card, retry on the unified-memory sibling -- that set is discrete, so the guard never fires, and the retry then switches GGML_CUDA_ENABLE_UNIFIED_MEMORY on for a load that now goes to system RAM. An oversized GGUF was OOM-killed mid-load instead of getting the actionable refusal the same host returns when the APU is picked first. Ask the same guard again for the set we are about to spawn on.
…#7624) The gate's survivor pin dropped only --tensor-split, so a manual load that kept tensor parallelism on the full device count ran one device under --split-mode tensor while /status still advertised the mode and the MTP tensor watchdog stayed armed. Drop the mode with the ratio, as the arch-crash retry already does. The APU RAM preflight runs with gpu_indices None on an unpinned launch, so an uncovered APU refused a load the surviving discrete card would have held. Re-ask it for the survivors, on the refusal branch only. The forced-CPU mask went through the default HIP arm, which clears an inherited ROCR mask. HIP -1 hides every device on its own, so the clear only re-exposed agents the parent hid to the HSA enumeration that dies on an uncovered arch. Keep the mask, matching the embedding CPU launch.
…(#7624)
The reactive retry keyed on hipErrorInvalidImage ("device kernel image is
invalid") alone, which is the wording both #7624 and #7669 field logs
happened to show. hipErrorNoBinaryForGpu is the same arch mismatch and is
the code whose documented cause is code compiled for a different GPU
architecture, so a build that raises it fell through to the fit and
flash-attn retries and landed back on the misleading GGUF/memory error.
Match both wordings, case-folded: hipGetErrorString is lowercase but the
layers that reprint it are not consistent about it. Three tests, including
one that drives the whole recovery end to end on the new wording.
…e (#7624) Regression tests only, for the two "did this break my working setup" questions the gate raises and nothing pinned yet. HSA_OVERRIDE_GFX_VERSION is the long standing workaround for an arch the ROCm stack does not build for: the user sets it, ROCr reports the spoofed arch, and code compiled for that arch really does run on the card. The gate reads the arch through the same device properties HIP acts on, so it follows the override and the covered-by-spoofing card survives. Worth pinning because the raw silicon is uncovered and only the presented arch says otherwise. An install written before this PR has no mapped_targets in its marker, and the field is deliberately outside the install fingerprint, so it is never refreshed for this reason alone. Such a host must behave exactly as it did before: the probe keeps every device and the launch is not masked onto the CPU. #7669's mis-pick stays there until the install is refreshed, which is the intended trade, and the test says so rather than leaving it implied. Also three more crash strings on top of the ones the previous commit added: the same error raised during ggml's backend init, which prints through a different format string, and the CUDA spelling (cudaErrorNoKernelImageForDevice) since the retry is not ROCm-gated. And a correction to the comment on the "--alias -ts" case in test_a_command_without_a_split_reports_nothing_to_do: the scan is positional, so a VALUE spelled like the flag IS removed. Nothing Studio builds can reach it, so the limitation is now documented where it lives rather than described as behaviour that does not happen.
…ed (#7624) write_prebuilt_metadata only runs on a real install, and mapped_targets is deliberately outside the install fingerprint so recording it never forces an existing install to reinstall. The two together meant the runtime arch gate read whatever the marker happened to hold, indefinitely. That is worst for the users this PR is for. An install made before the field existed never gains it, so on an already-up-to-date host the gate keeps failing open and #7624 / #7669 stay broken until an unrelated llama.cpp release happens to trigger a real reinstall. A manifest that corrects mapped_targets for an unchanged asset never lands either, and stale coverage is worse than none: too narrow forces a supported GPU to CPU, too wide leaves an unsupported one visible to crash. sync_marker_arch_coverage follows the ggml_tree backfill exactly, for exactly that reason, and is called from all three reuse paths beside the rocm_gfx sync, which has the same reach. A bundle that declares no targets (CUDA, Vulkan, CPU, source) leaves the marker alone rather than clearing it: reuse requires the fingerprint to match, so the asset is the one the marker already describes. It never raises, since the reuse path runs after the install is already valid and an exception there no longer falls back to a source build. Five tests, each confirmed by reverting the behaviour it claims.
for more information, see https://pre-commit.ci
Two more places the arch gate's CPU-masked launch had to reach. holds_no_vram asks for manual mode with gpu_layers 0, so an arch-gated launch answered False even though it is the stronger case: the gate found the installed build has no kernels for any card here, the child was masked with the "-1" sentinel and cannot see a device at all. It arrives through an automatic request, which is exactly what the caller at routes/inference.py:8206 already documents as "recovery may turn an automatic GPU request into a zero-VRAM load". Left as it was, a server holding no VRAM kept the CHAT claim, blocked an image or video pipeline from coexisting, and could be unloaded mid-load by an owner it never competed with. The flag is per-load: set where the launch records its offload state, cleared on commit and on unload. llama.cpp reads LLAMA_ARG_DEVICE and LLAMA_ARG_MAIN_GPU as the env spelling of --device and --main-gpu (common/arg.cpp set_env), and neither the chat forced-CPU launch nor the embedding CPU launch passes those flags, so a value exported into the parent environment reaches the child. Hiding every device while leaving that pick in place is the one combination llama.cpp cannot serve: parse_device_list rejects a name that no longer enumerates, so the child exits instead of running on the CPU we just chose for it. Cleared with the "-1" sentinel and on the embedding CPU arm. This file already treats an inherited LLAMA_ARG_SPLIT_MODE and LLAMA_ARG_FIT as live input, so it is the same rule rather than a new one. Nine tests, each confirmed by reverting the behaviour it claims.
When manual mode supplies a per-GPU ratio across every visible card and the arch gate drops one of them, the launch strips --tensor-split from the argv and leaves self._tensor_split None. The request that asked for the ratio is unchanged, and the UI re-sends it verbatim on every Apply, so _runtime_matches_intent compared the live None against the requested tuple and read it as a different load every time. The result was a teardown and relaunch of the same already-normalized server on each identical Apply, forever, on the one host shape the gate exists for. Record the ratio the gate dropped and accept it in the comparator as this request normalized. The record is guarded on an actual drop rather than on None == None, so a launch that dropped nothing still cannot excuse a live split against a request that asks for none, and any other ratio still reloads. It is cleared unconditionally at the top of each load, so a drop earned by one launch cannot excuse a mismatch on the next. Seven tests, each confirmed by reverting the behaviour it claims.
…t them (#7624)
Three misses, all in the arch gate's own new paths.
The forced-CPU flag was only ever assigned True. load_model phase 1 kills
the old process without running the unload reset, so the value outlived
its launch: a host that gains coverage (a llama.cpp update, or just the
next model) reported a VRAM-holding server as holding none, and the GPU
arbiter left it unclaimed beside a competing workload. That is worse than
before the flag existed. It is now published on every load, and cleared in
the diffusion state block beside the sibling chat fields it already resets
for the same reason. The reset I had put in _apply_cpu_fallback_state is
gone with it: that runs only on the Vulkan CPU-fallback path, which is why
it never covered this.
Stripping --split-mode / --tensor-split from argv cannot remove
LLAMA_ARG_SPLIT_MODE / LLAMA_ARG_TENSOR_SPLIT, and the existing
tensor-to-layer reconciliation only clears the pair when an inherited mode
is present and non-layer. So an inherited ratio survived a narrowing that
re-indexes the survivors under it, and an inherited tensor mode walked
straight back into the abort the forced-CPU strip exists to prevent
("LLAMA_SPLIT_MODE_TENSOR needs >= 1 devices"). Cleared at all three gate
branches, and not gated on the argv strip having found anything, since the
env twin is the hazard on its own.
Residency is a property of the devices, and the arch-crash retry changes
them. The canonical shape crashes on the discrete card and lands on the
unified-memory APU, where the weights are host-backed after all, so the
page-lock the first launch correctly skipped is the one the user asked
for. The respawn ran unlocked and recorded _memory_mlock_applicable False,
which reads the missing lock as deliberate and dedupes away the reload
that would apply it. Recomputed through the same helpers as the first
launch so the two cannot drift.
Also collapses the device-placement env clear onto the existing
_clear_device_placement_env instead of the near-duplicate helper I added
last round.
Eleven tests, each confirmed by reverting the behaviour it claims.
for more information, see https://pre-commit.ci
…alled (#7624) Cross-platform staging CI failed this test on all three runners, and the cause was the test, not the code: it patches _torch_is_rocm but not the `import torch` that guards it. _active_gpu_visibility_mask reads the ROCr mask only inside that try, so on a runner whose dependency set omits torch the except arm sets is_rocm False, the mask is read from CUDA_VISIBLE_DEVICES instead, an unmappable ROCr value reads back as "no mask", and the gate pins the ordinals the test asserts it must not. So the assertion was exercising the CUDA branch and reporting it as the ROCm one. A fake torch module and an explicit CUDA_VISIBLE_DEVICES clear make it deterministic. Verified both ways: with torch importable, and under a meta_path finder that makes `import torch` raise, which is what the runners do and what reproduces the CI failure against the previous commit.
The recompute swept every --mlock / --no-mmap / --load-mode off argv before re-appending its own. That dropped hand-written user memory flags the policy deliberately preserves, and since --mlock and --no-mmap take no value in llama.cpp's parser, the value-consuming scan also ate the argv entry that followed them. Restrict it to the lock-adding direction, which is the only one where argv carries no policy-emitted lock to take back off, and append the flag so llama.cpp's last-wins parse gives it the final say. The reverse direction leaves the crashed launch's lock alone; the child really does hold it, so the record stays truthful.
for more information, see https://pre-commit.ci
Comments and docstrings only; no code changes.
…dependent state (#7624) Four review items, all the same shape as ones already fixed here. Record the dropped split MODE alongside the dropped ratio, and record the ratio on the reactive retry and the forced-CPU arm as the proactive gate already does. Without it the duplicate-load check reads a normalized server as a different one and tears down and reloads a multi-GB model on every Apply. Treat a forced-CPU launch as host-resident for Model Memory: the child runs entirely from host RAM, but _weights_in_host_memory answered for the original placement, so the page-lock was skipped and recorded as deliberate. Require real arch evidence before forcing CPU. _get_gpu_memory turns any probe error into an empty list, so "gated empty, ungated not" also describes a one-shot failure of the first probe on a host the gate never filters; the branch now re-derives the filter's own verdict instead.
for more information, see https://pre-commit.ci
Comments and docstrings only; no code changes.
![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: b5b7011e23
ℹ️ 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".
Comment on lines
+5103
to
+5104
| if pinned != "-1": | ||
| env.pop("ROCR_VISIBLE_DEVICES", None) |
There was a problem hiding this comment.
On Linux ROCm hosts where every GPU is excluded by the architecture gate, the forced-CPU path calls this with pinned == "-1", but the conditional leaves ROCR_VISIBLE_DEVICES unset when the parent did not already define it. HIP_VISIBLE_DEVICES=-1 filters devices only after ROCr/HSA enumeration, so llama-server can still enumerate the unsupported agent and suffer the same pre-start kernel-image crash this fallback is intended to avoid; the embedding CPU fallback has the same exposure. Apply a ROCr-level hide-all mask for the sentinel case before launching the child.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: dffd09b68e
ℹ️ 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".
| # restored from `gpu_ids` below so the pin happens anyway. A | ||
| # pinned uncovered GPU is the user's call and already reports its | ||
| # own clear "device kernel image is invalid". | ||
| _gpu_mem = self._get_gpu_memory(binary, for_llama_server = not gpu_ids) |
There was a problem hiding this comment.
On a mixed-architecture ROCm host, a caller that pins placement through the supported --device extra argument or LLAMA_ARG_DEVICE, rather than gpu_ids, still satisfies not gpu_ids and enters the architecture gate. The later survivor mask reindexes the remaining physical devices while the pass-through --device argument is retained, so a request such as ROCm0 can silently run on a different physical GPU; if every GPU is gated out, the retained pin instead makes the forced-CPU child reject the unavailable device. Treat every effective explicit device pin as exempt here, or normalize/remove that pin when forcing a different placement.
Useful? React with 👍 / 👎.
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.
Disposable CI run for unslothai/unsloth#7670. Do not merge; closed after CI.