Installer: opt-in Vulkan llama.cpp backend (and fallback when no AMD card is HIP-supported) - #7373
Conversation
![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: c0bc95e580
ℹ️ 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".
…nslothai#7357) Add UNSLOTH_LLAMA_BACKEND=vulkan and --llama-backend vulkan to force the upstream Vulkan prebuilt on any host, persist llama_backend in the install marker, and re-assert it during Studio updates. On Windows AMD, auto-fallback to Vulkan when no detected gfx arch is in the upstream win-hip-radeon GPU_TARGETS set (e.g. gfx803 / RX 480). Mixed setups where at least one card is HIP-supported still default to HIP unless opted in.
c0bc95e to
c7931df
Compare
for more information, see https://pre-commit.ci
Honor ROCm family tokens (gfx110X), include fork-supported gfx1103, require a known active gfx before auto-Vulkan, and base the HIP floor check on the visible-device target instead of every physical GPU in hipinfo.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: ce15f76e13
ℹ️ 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".
- llama_backend_from_env: stop reading UNSLOTH_LLAMA_CPP_BACKEND. That is a separate pre-existing setup variable meaning auto/cpu; setup.sh/setup.ps1 warn and ignore other values, so reading it here forced Vulkan behind that warning. Vulkan opt-in stays on UNSLOTH_LLAMA_BACKEND / UNSLOTH_FORCE_VULKAN. - _should_auto_vulkan_for_amd_windows: gate on not has_physical_nvidia (not merely has_usable_nvidia). A CUDA-masked NVIDIA card keeps has_physical_nvidia while has_usable_nvidia goes False; Vulkan ignores CUDA_VISIBLE_DEVICES and could enumerate the reserved card. Mirrors the Intel auto path. Explicit opt-in still overrides. - test fakes: validate_prebuilt_attempts/validate_prebuilt_choice gained a llama_backend kwarg; the four fake signatures in the fallback tests now accept it, clearing the TypeError that reddened Backend CI / Repo tests (CPU). Tests: UNSLOTH_LLAMA_CPP_BACKEND=vulkan no longer triggers Vulkan; hidden physical NVIDIA suppresses AMD auto-Vulkan while explicit opt-in overrides.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: acdba16726
ℹ️ 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 WINDOWS_HIP_PREBUILT_GFX_TARGETS allow-list omitted gfx1034, so _route_to_vulkan_prebuilt downgraded RX 6500/6400-class hosts to the upstream Vulkan prebuilt before published_rocm_choice_for_host could match the fork windows-rocm gfx103X bundle (whose members include gfx1034). Add gfx1034 to the allow-list and a regression test asserting it stays on the fork ROCm asset.
|
@codex review |
|
Codex Review: Didn't find any major issues. Keep them coming! 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". |
|
ci failures are unrelated to the pr |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 7a82351cdd
ℹ️ 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".
| "prebuilt instead of the detected GPU backend" | ||
| ) | ||
| host = _vulkan_only_host(host) | ||
| persist_backend = "vulkan" |
There was a problem hiding this comment.
On Windows ARM64, _vulkan_only_host routes an explicit Vulkan request upstream, but direct_upstream_release_plan's ARM64 branch only selects win-cpu-arm64; similarly, the x64 selector can fall back to CPU when the Vulkan archive is missing or fails validation. Persisting vulkan here therefore lets a CPU-only installation succeed while its marker claims Vulkan, causing later Studio updates to repeatedly reassert a backend that was never installed. Only persist/accept the request when the selected install_kind is Vulkan, or fail instead of falling back to CPU.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Confirmed and fixed in b09ad03.
Reproduced against the real ggml-org b10079 asset list: Windows arm64 with UNSLOTH_LLAMA_BACKEND=vulkan, UNSLOTH_FORCE_VULKAN=1 or --llama-backend vulkan installed llama-b10079-bin-win-cpu-arm64.zip (install_kind windows-arm64) while the marker recorded llama_backend=vulkan, which _plan_llama_phase then re-asserted on every later update, permanently pinning a CPU-only host to upstream. The x64 fall-through to win-cpu-x64 when the Vulkan archive is missing or fails validation had the same effect.
Upstream release.yml builds vulkan for arch x64 only; Windows arm64 gets win-cpu-arm64 plus win-opencl-adreno-arm64, so a Vulkan request there can never be satisfied. Two parts:
- _has_no_vulkan_prebuilt keeps Windows arm64 on the published bundle instead of rewriting the host to Vulkan-only, the same treatment macOS already gets. That host now stays on app-b10079-mix-fb3d4ca-windows-arm64-cpu.zip, identical to what it gets with no opt-in.
- persisted_llama_backend records the backend only when the attempt that actually won has a Vulkan install_kind, so a CPU fall-through leaves no marker to re-assert and the next update re-detects normally.
12 regression tests added in studio/backend/tests/test_install_resolve_prebuilt.py; reverting either half reddens them. Full install suite 1414 passed / 2 skipped.
|
@codex review |
|
Codex Review: Didn't find any major issues. Hooray! 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". |
|
@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: ad15bd1f48
ℹ️ 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".
| rocm_gfx_target = gfx, | ||
| rocm_gfx_targets = [gfx], |
There was a problem hiding this comment.
When the Windows setup runs on a mixed-AMD host with only CUDA_VISIBLE_DEVICES=1, or with a comma-separated HIP/ROCR mask such as 1,0, setup.ps1 selects GPU 0 (lines 1380 and 1435-1447) and forwards that arch through --rocm-gfx. This override then replaces the installer's correctly visible-device-aware rocm_gfx_target; if GPU 0 is HIP-supported but the selected GPU is not, auto_no_hip remains false and the unusable HIP bundle is installed instead of Vulkan. The forwarded value needs the same visibility semantics, or it should not overwrite a successfully detected active arch.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Confirmed, and fixed in eee9333.
Reproduced by running the cited setup.ps1 lines verbatim under pwsh 7.6 (extracted by line number, not retyped) against a mixed-AMD host of GPU 0 gfx1100 and GPU 1 gfx1010, then feeding the forwarded value through the real _apply_host_overrides / _should_auto_vulkan_for_amd_windows and asset resolver:
mask ps1 fwd installer active gfx auto_vulkan installed asset
no mask (control) gfx1100 gfx1100 gfx1100 False ...-windows-x64-rocm-gfx110X.zip ok
CUDA_VISIBLE_DEVICES=1 gfx1100 gfx1010 gfx1100 False ...-windows-x64-rocm-gfx110X.zip MISMATCH
HIP_VISIBLE_DEVICES=1 gfx1010 gfx1010 gfx1010 True llama-...-win-vulkan-x64.zip ok
HIP_VISIBLE_DEVICES=1,0 gfx1100 gfx1010 gfx1100 False ...-windows-x64-rocm-gfx110X.zip MISMATCH (amd-smi branch)
ROCR_VISIBLE_DEVICES=1 gfx1010 gfx1010 gfx1010 True llama-...-win-vulkan-x64.zip ok
ROCR_VISIBLE_DEVICES=1,0 gfx1100 gfx1010 gfx1100 False ...-windows-x64-rocm-gfx110X.zip MISMATCH (amd-smi branch)
4 of 12 combinations mismatched. Both failure modes are as described: the hipinfo branch does split comma masks but reads neither CUDA_VISIBLE_DEVICES, and the amd-smi branch matches a bare integer only, so 1,0 also lands on GPU 0. The comma case is reachable rather than theoretical because the two sides resolve hipinfo differently: setup.ps1 skips the venv copy via Test-HipinfoIsVenvInternal and falls through to amd-smi, while the installer deliberately accepts that same venv copy, so on a driver-only host with AMD torch wheels setup takes the amd-smi branch while the installer gets the full visible-aware arch list.
I took the narrower of your two remedies, not overwriting a successfully detected active arch, rather than porting visibility semantics into PowerShell. It is one condition at the single point where the forward is folded in, it is unit-testable where the PowerShell is not, and it matches what the override's own docstring says it is for (filling an arch the probe missed on amd-smi-only / name-inferred hosts). Porting the semantics would need edits at both setup.ps1 sites plus setup.sh, and would still leave the mismatch wherever the two sides disagree for any other reason, like the venv-hipinfo asymmetry above.
One carve-out: an explicit UNSLOTH_ROCM_GFX_ARCH remains authoritative. It is the argparse default for --rocm-gfx and is documented as the manual override ("Manual override" in install.ps1, "authoritative" in install.sh), so a blanket fill-only rule would have silently ignored it on any host where the probe works.
Scope, over an override-path profile matrix of 18 profiles resolved end to end to a real asset: 3 change, all of them the probe/setup disagreement (the two masks above plus the inverse case where the probe sees the supported GPU and setup forwards the unsupported one). The other 15 are byte-identical, including every negative control: amd-smi-only fill, driver-only fill, KFD-only Linux fill, name-inferred arch, UNSLOTH_ROCM_GFX_ARCH set, family label gfx110X, malformed forward, force_cpu, Linux ROCm, NVIDIA. The 43-profile matrix for this PR's own feature surface is unchanged.
Tests: 1391 passed / 2 skipped before, 1394 passed / 2 skipped after, no new failures (tests/studio/install/ excluding the env-broken test_rocm_rdna_routing.py, plus this PR's own suites). As a negative control the two fix-asserting tests fail against the unmodified head while the two preserved-behaviour tests pass on both sides.
Left alone deliberately: setup.ps1 and setup.sh themselves, and install_whisper_prebuilt.py's apply_host_overrides, which has the same overwrite shape but no Vulkan floor, so nothing in this PR's routing depends on it.
setup.ps1 resolves the gfx arch from its own probe, and that pick is not fully visible-device aware: neither the hipinfo nor the amd-smi branch reads CUDA_VISIBLE_DEVICES, and the amd-smi branch matches a bare integer only, so a comma-separated HIP/ROCR mask such as 1,0 also falls back to GPU 0. The resulting arch was then forwarded through --rocm-gfx and replaced the arch detect_host() had already resolved for the runtime-visible GPU. On a mixed-AMD Windows host that flipped the auto-Vulkan decision: with GPU 0 gfx1100 and a masked-in gfx1010, the forward reinstated gfx1100, _should_auto_vulkan_for_amd_windows() saw a HIP-supported arch and the HIP bundle was installed for a GPU that cannot run it. Fold the forward in as a fill rather than a replacement: it still supplies the arch on amd-smi-only, driver-only and name-inferred hosts where the probe reports none, which is what --rocm-gfx exists for, but no longer overwrites a successfully detected active arch. An explicit UNSLOTH_ROCM_GFX_ARCH stays authoritative, since it is the documented manual override for hosts whose arch the probes get wrong.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 4584218756
ℹ️ 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".
| _val = _val.strip() | ||
| return bool(_val) and _val != "-1" |
There was a problem hiding this comment.
On Windows setup flows, HIP_VISIBLE_DEVICES/ROCR_VISIBLE_DEVICES set to an empty string or -1 can still leave a detected or name-inferred gfx value forwarded through --rocm-gfx; _apply_host_overrides then reconstructs an active AMD target. Returning False for these masks enables the automatic Vulkan route, whose runtime ignores the HIP masks and can use the GPU the user explicitly hid. Treat any present all-hidden mask as suppressing automatic Vulkan, while still allowing an explicit Vulkan opt-in.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Correct, and it is a hole in the guard I added one commit earlier. Fixed in d2969d8.
I had exempted "" / -1 because _pick_rocm_gfx_target reports those as no active target. That reasoning only covers the probe. --rocm-gfx reconstructs an active arch independently, and setup resolves that arch from the display-adapter name through WMI, which no HIP mask touches, so the exemption was reachable exactly as described. Driving the real detect_host() over a stubbed masked hipinfo and then folding in a forwarded gfx803:
HIP_VISIBLE_DEVICES="" active=gfx803 auto_vulkan=True <- 50edf167c
HIP_VISIBLE_DEVICES="-1" active=gfx803 auto_vulkan=True
Both now False. Hiding every AMD GPU from HIP is the strongest form of the hazard the guard exists for, not an exemption from it: Vulkan honours none of these masks, so auto-routing there hands it all of them.
Presence of any of the three variables is now the whole test, which also removes the value parsing:
return any(
os.environ.get(_env) is not None
for _env in ("HIP_VISIBLE_DEVICES", "ROCR_VISIBLE_DEVICES", "CUDA_VISIBLE_DEVICES")
)Explicit opt-in is untouched, as you note it should be: --llama-backend vulkan and UNSLOTH_LLAMA_BACKEND=vulkan still route under any mask.
Scope. With no mask in the environment the 43-profile feature matrix and the 28-profile override matrix are byte-identical to 50edf16, so this changes only masked hosts. Tests stay at 591 passed, 2 skipped, 0 failed; the three all-hidden cases and the presence check fail on 50edf16 and pass here.
| "Force the llama.cpp prebuilt backend. vulkan installs the upstream Vulkan " | ||
| "bundle and records the choice so Studio updates keep it; ignored on hosts " | ||
| "with no Vulkan prebuilt (macOS, Windows arm64). " | ||
| "Same effect as UNSLOTH_LLAMA_BACKEND=vulkan / UNSLOTH_FORCE_VULKAN=1." |
There was a problem hiding this comment.
When this option is used through setup.sh or setup.ps1 and the upstream prebuilt is unavailable or fails validation, both setup scripts continue into their existing source-build fallback without honoring UNSLOTH_LLAMA_BACKEND=vulkan: Linux selects CUDA/ROCm from host detection, while Windows selects CUDA or CPU, and neither adds GGML_VULKAN. Thus an explicit Vulkan request can silently finish successfully with a different backend; thread the preference into the fallback CMake configuration or report that the requested backend could not be installed.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
The observation is accurate but it is not this PR's, so I am leaving it.
Neither setup script contains GGML_VULKAN at all, and neither reads UNSLOTH_LLAMA_BACKEND:
$ grep -rn "GGML_VULKAN" . # no hits anywhere in the repo
$ grep -n "UNSLOTH_LLAMA_BACKEND" studio/setup.ps1 studio/setup.sh # no hits
That is equally true on main today, where UNSLOTH_FORCE_VULKAN already exists (6 references in studio/install_llama_prebuilt.py at the merge base) and already routes to the upstream Vulkan prebuilt. So an explicit Vulkan request landing on a non-Vulkan source build when the prebuilt is unavailable is pre-existing behaviour of the pre-existing opt-in, not something this PR introduces.
The part this PR is responsible for is not lying about the outcome, and it does not: persisted_llama_backend() records llama_backend only when the bundle that actually landed is a Vulkan install kind, so a Vulkan request that ends on CPU or a source build leaves no vulkan marker for the updater to re-assert. Windows arm64 also logs explicitly that the request was ignored.
Threading a backend preference into the source-build CMake configuration of setup.sh and setup.ps1, or making the installer hard-fail an unsatisfiable explicit backend, is a real improvement and I would take it as its own change. It is not in scope here.
| _advisory = gfx in _physical or gfx in WINDOWS_ROCM_FAMILY_GFX_LABELS | ||
| if gfx != _manual and _active and gfx != _active and _advisory: | ||
| return dataclasses_replace(host, has_rocm = True) |
There was a problem hiding this comment.
On a mixed-architecture AMD host, an explicit --rocm-gfx targeting any non-active GPU already present in rocm_gfx_targets is classified as advisory and discarded unless the same value also happens to be in UNSLOTH_ROCM_GFX_ARCH. A direct caller selecting the second GPU therefore still gets the active GPU's bundle, which can fail validation or run against the wrong architecture. Distinguish setup-generated hints from an explicit CLI override, or otherwise let the CLI value remain authoritative.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Disagreeing here, on two grounds.
The installer cannot make that distinction. setup.ps1 forwards --rocm-gfx $script:ROCmGfxArch on essentially every Windows AMD run, and setup.sh does the same. A human typing --rocm-gfx gfx1100 and setup forwarding it produce identical argv, so "distinguish setup-generated hints from an explicit CLI override" has nothing to key on. Making the CLI value authoritative in this branch means making setup's forward authoritative, which is the mispick this carve-out was added for: setup's own pick is not fully visible-device aware (neither its hipinfo nor its amd-smi branch reads CUDA_VISIBLE_DEVICES, and the amd-smi branch matches a bare integer only, so a comma mask falls back to GPU 0).
The direction of harm is the other way round. HIP executes on the visible device, which is what _pick_rocm_gfx_target resolved into rocm_gfx_target by honouring all three mask variables with HIP's own semantics. Installing the bundle for a different, masked-off GPU's arch is the case that "runs against the wrong architecture" - the code would be built for a device HIP will not select. The carve-out only fires when the forwarded arch is another GPU the probe saw ON THIS HOST and the probe resolved a different active one, which is precisely the shape where the forward is a stale GPU-0 read rather than a considered choice.
There is a documented escape hatch for genuinely selecting a different GPU, and it is the one setup.ps1 prints a tip about: UNSLOTH_ROCM_GFX_ARCH. It is compared as _manual and beats the carve-out, so a caller that means it gets it. Adding a second, distinguishable flag for the same purpose would be new surface area on a PR whose subject is the llama.cpp backend.
…k too The mask guard exempted an empty or -1 value on the grounds that the probe reports no active target under it, but that only holds for the probe: a forwarded --rocm-gfx still reconstructs an active arch, and setup infers that arch from the display-adapter name, which no HIP mask touches. A user who hid every AMD GPU from HIP could therefore still be auto-routed to Vulkan, which honours none of these masks and would then use all of them. That is the strongest form of the hazard the guard exists for, not an exemption from it. Presence of any of the three variables is now the whole test, which also removes the value parsing. An explicit --llama-backend vulkan or UNSLOTH_LLAMA_BACKEND is still unaffected.
|
@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: d2969d8b2f
ℹ️ 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".
| active = _active_rocm_gfx_target(host) | ||
| if not active: | ||
| # ROCm confirmed but gfx unknown (--has-rocm only): keep the HIP / fork / source path. | ||
| return False |
There was a problem hiding this comment.
On the target AMD-only Windows case where the card is below the HIP prebuilt floor (for example, an RX 480 with only the Adrenalin driver), no active gfx target reaches this check: setup.ps1's WMI fallback maps only ROCm-supported gfx103X-and-newer names and therefore forwards neither --rocm-gfx nor --has-rocm, while detect_host() records a target only when hipinfo or the guarded amd-smi probe succeeds. This early return consequently leaves the host looking CPU-only and installs the CPU bundle rather than performing the advertised automatic Vulkan fallback; legacy AMD/display-adapter detection must supply a fallback signal before requiring active.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
The mechanism you describe is real, and worth having on the record: an Adrenalin-only RX 480 box reaches no active gfx, so the automatic fallback does not fire and the CPU bundle is installed. setup.ps1's name table maps only ROCm-supported arches (gfx1201/1200/1151/1150/1100/1102/1103/1030/1032/1034), and on that host $HasROCm is false too, so neither --rocm-gfx nor --has-rocm is forwarded; the installer's own probe needs hipinfo or the HIP-SDK-gated amd-smi, and the venv hipInfo.exe only exists when ROCm torch wheels were installed, which needs a resolved arch in the first place.
Where I disagree is that this is a defect rather than the documented boundary of the feature.
_should_auto_vulkan_for_amd_windows() states the rule in the branch you are commenting on: "ROCm confirmed but gfx unknown (--has-rocm only): keep the HIP / fork / source path." Requiring a known arch is deliberate, not an oversight, and #7357 scopes the auto-fallback the same way: "on Windows AMD, when no detected gfx arch is supported by the HIP prebuilt". The issue's own reporter is on an RX 9070 XT plus RX 480, a mixed host, and the issue explicitly rules out auto-preferring Vulkan there: "No auto-preferring Vulkan on mixed setups where at least one card is HIP-supported ... that stays the user's call via the opt-in." Their actual complaint was that a Studio update clobbered a manual Vulkan swap back to HIP, which the explicit opt-in plus llama_backend marker persistence in this PR does fix.
The auto path still covers the hosts where an arch is resolvable: a HIP SDK present, the amd-smi branch, UNSLOTH_ROCM_GFX_ARCH, or setup's name inference.
Closing the remaining gap means routing on display-adapter evidence, an AMD ven_1002 counterpart to windows_intel_gpu_in_registry(). That is a new hardware-detection path with its own failure mode, and this file already documents it: the display class key "lists installed display-driver configs, which can outlive removed hardware, where sysfs lists present devices". For Intel that misfire is benign; using it to decide the AMD backend on a host with no other GPU signal is not, and it needs a new HostInfo field, new routing and its own tests. Worth doing, not worth bolting onto a PR that is already changing the AMD Windows routing.
| if published_repo and published_repo.strip().lower() == UPSTREAM_REPO.lower(): | ||
| return UPSTREAM_WINDOWS_HIP_GFX_TARGETS | ||
| return WINDOWS_HIP_PREBUILT_GFX_TARGETS |
There was a problem hiding this comment.
When --published-repo names a custom repository or upstream mirror, resolve_simple_install_release_plans() sends it through direct_upstream_release_plan(), whose Windows AMD branch selects the standard win-hip-radeon asset and then CPU. This helper nevertheless grants the fork-only target superset to every repository except the exact ggml-org/llama.cpp spelling, so a custom mirror carrying upstream-standard assets is incorrectly considered HIP-capable on targets such as gfx1034 or gfx1103 and can land on HIP/CPU instead of being routed to Vulkan. The superset should be used only for DEFAULT_PUBLISHED_REPO; other direct-release repositories need upstream coverage or manifest-derived coverage.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Correct. Fixed in 29279b6.
The dispatch is repo-exact, not upstream-exact:
# resolve_simple_install_release_plans()
repo = published_repo or DEFAULT_PUBLISHED_REPO
if repo == DEFAULT_PUBLISHED_REPO:
return _fork_manifest_release_plans(...)
# everything else falls through to direct_upstream_release_plan()So the predicate had the polarity wrong: it exempted one name instead of gating on the one repo whose manifest the superset describes. A mirror got the fork's gfx1034 / gfx1103 / gfx908 coverage and was answered "HIP serves this" on a planner that only ever offers win-hip-radeon then CPU.
if (published_repo or DEFAULT_PUBLISHED_REPO) == DEFAULT_PUBLISHED_REPO:
return WINDOWS_HIP_PREBUILT_GFX_TARGETS
return UPSTREAM_WINDOWS_HIP_GFX_TARGETSMatching that comparison exactly rather than case-insensitively picks up a second case of the same bug: unslothAI/llama.cpp does not equal DEFAULT_PUBLISHED_REPO, so the resolver really does take the upstream path for it, and the old .strip().lower() on the ggml-org side would have granted it the fork superset. An empty repo still defaults to the fork, as the resolver does.
Scope. The 43-profile feature matrix and the 28-profile override matrix are byte-identical: both plan against the fork or against ggml-org, the two spellings whose answer is unchanged. Tests 591 -> 596 passed, 2 skipped, 0 failed. 5 new: a mirror, an oddly cased ggml-org and an oddly cased fork (two of which fail on d2969d8), plus None / "" negative controls asserting the default path keeps its fork-only archs.
…rror The floor set is a union of the fork's windows-rocm bundles and only the fork is planned from its manifest: resolve_simple_install_release_plans() compares == DEFAULT_PUBLISHED_REPO and sends every other --published-repo through direct_upstream_release_plan(), whose AMD branch offers win-hip-radeon then CPU and never Vulkan. Exempting only the exact ggml-org spelling therefore told a mirror carrying upstream-standard assets that fork-only archs such as gfx1034, gfx1103 and gfx908 were HIP-served, landing them on HIP or CPU instead of the Vulkan bundle that would actually run. Gate on the fork instead. Matching the dispatch exactly, spelling included, also fixes a differently cased repo: that really does take the upstream path, so it must be answered with upstream coverage rather than the fork superset. An empty repo still defaults to the fork, as the resolver does.
|
@codex review |
|
Codex Review: Didn't find any major issues. Already looking forward to the next diff. 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". |
The guard compared WINDOWS_HIP_PREBUILT_GFX_TARGETS against a second hardcoded tuple in the same test file, so a windows-rocm arch newly published by the fork passed both. Affected hosts would then be routed off the hash-approved fork ROCm bundle onto an unhashed upstream Vulkan build with nothing failing. Read the fork's llama-prebuilt-manifest.json through the installer's own resolver instead, and assert the floor, the family labels, and the routing tuple all still cover what it publishes. The manifest ships only as a release asset, so an unreachable release skips with an explicit reason rather than flaking. Both literals match the manifest as published today.
|
Tightened the Windows HIP gfx floor drift guard. It used to compare What it would have missed before: the fork publishing a new |
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: f063590c4b
ℹ️ 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".
| if token in WINDOWS_ROCM_FAMILY_GFX_LABELS: | ||
| # A family label names a fork bundle whose members are all built by upstream's | ||
| # windows-hip targets too (gfx103X -> gfx1030..1032, gfx110X -> gfx1100..1102, | ||
| # gfx120X -> gfx1200/1201), so HIP serves it whichever repo is planned against. | ||
| return True |
There was a problem hiding this comment.
When a non-default repository is selected with a forwarded family target such as gfx103X or gfx110X, this unconditional success bypasses _windows_hip_gfx_targets(published_repo). Those fork families also cover gfx1034 and gfx1103, respectively, while UPSTREAM_WINDOWS_HIP_GFX_TARGETS excludes both; therefore an update or direct invocation using the family label can retain the upstream HIP path and fall through to CPU instead of routing the unsupported card to Vulkan. Treat these family labels as fork coverage, or resolve them against repository-specific members.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Checked this by execution and it does not hold. Two of the three premises are wrong, and the suggested remedy breaks a proven invariant.
The counterfactual is false. "the fork path would have routed to Vulkan" is not what the fork path does. A family label is supported against the fork too, so auto_vulkan is False for both repositories, and the fork resolver then serves the family bundle over HIP. Against the live manifest (unslothai/llama.cpp@b10107-mix-1911198):
forward=gfx103X repo=unslothai/llama.cpp active=gfx103x hip_supported=True auto_vulkan=False
forward=gfx103X repo=ggml-org/llama.cpp active=gfx103x hip_supported=True auto_vulkan=False
forward=gfx110X repo=unslothai/llama.cpp active=gfx110x hip_supported=True auto_vulkan=False
forward=gfx110X repo=ggml-org/llama.cpp active=gfx110x hip_supported=True auto_vulkan=False
fork serves gfx103X -> app-b10107-mix-1911198-windows-x64-rocm-gfx103X.zip (windows-rocm)
fork serves gfx110X -> app-b10107-mix-1911198-windows-x64-rocm-gfx110X.zip (windows-rocm)
There is no fork-versus-mirror divergence to fix. Both keep HIP.
The precondition is not reachable from any shipped caller. A family label only ever enters --rocm-gfx from a fork asset name, and the repository travels with it from the same source:
setup.ps1:3468hard-codes$HelperReleaseRepo = "unslothai/llama.cpp", never reassigns it, and has noparam()block. The only other--published-repoin the file issetup.ps1:4087,--resolve-llama-tagwith no--rocm-gfx, which returns atinstall_llama_prebuilt.py:6686before any routing.setup.shis the same shape (_HELPER_RELEASE_REPOatsetup.sh:1203), and cannot reach a Windows gate anyway.- The update path derives the forward by regexing the marker asset (
update_flow.rocm_install_args), and readsrepoandassetfrom the same marker (llama_cpp_update.py:567-569) or the same resolver result (:612-614).ggml-orgpublishes no asset containinggfxat all:
repo=unslothai/llama.cpp asset=app-...-windows-x64-rocm-gfx110X.zip -> [--rocm-gfx gfx110x] family=True
repo=ggml-org/llama.cpp asset=llama-...-bin-win-hip-radeon-x64.zip -> [--has-rocm] family=False
repo=ggml-org/llama.cpp asset=llama-...-bin-win-vulkan-x64.zip -> [] family=False
On top of that, every shipped route that lands published_repo=ggml-org also lands a Vulkan asset, so llama_cpp_update.py:572 sets llama_backend=vulkan and explicit_backend short-circuits auto_no_hip to False at install_llama_prebuilt.py:6294 before this predicate runs. No unsloth CLI entry point forwards either flag.
The argv that would be required is hand-written:
python studio\install_llama_prebuilt.py --install-dir %USERPROFILE%\.unsloth\llama.cpp ^
--published-repo ggml-org/llama.cpp --rocm-gfx gfx110X
and even that only takes effect on a host whose own probe found no arch, since _apply_host_overrides:2636 demotes a family label to advisory whenever a concrete arch was probed.
The remedy would regress the covered majority. Resolving the labels against repository-specific members flips auto_vulkan False to True in 126 of 11700 swept cells, all of them on the non-default repository:
HEAD vs suggested remedy
rocm_gfx_target changed : 0
auto_vulkan False->True : 126
auto_vulkan True->False : 0
That is the forbidden direction. gfx110X covers gfx1100/1101/1102 as well as gfx1103, and the label cannot say which card is present, so answering unsupported to cover gfx1103 moves three upstream-built archs off a working HIP build onto Vulkan. gfx1034 and gfx1103 already reach Vulkan through the concrete-arch branch, which does answer per repository (test_auto_vulkan_is_repository_specific_for_fork_only_gfx).
One real thing here, and it is fixed in acb0664. The justification comment was wrong: it claimed upstream builds every member of these families. It does not build gfx1034 or gfx1103. The comment now states the actual reason. Comment only, no behaviour change.
Verification. The guard is load-bearing: dropping the family branch reddens test_auto_vulkan_is_repository_specific_for_fork_only_gfx and test_route_to_vulkan_prebuilt_family_gfx_token_keeps_rocm (594 passed, 2 failed); restoring it returns 596 passed, 2 skipped. The 5850-combination sweep is unchanged at 0 rocm_gfx_target changes, 0 False to True flips, 680 True to False flips all probe-confirmed. The 42-profile feature matrix and the 28-profile override matrix are both byte-identical. Suite 596 passed / 2 skipped before and after.
The comment justified serving gfx103X / gfx110X against any repository by claiming upstream's windows-hip targets build every member of those families. The fork manifest maps gfx103X to gfx1030..1032 plus gfx1034 and gfx110X to gfx1100..1102 plus gfx1103, and UPSTREAM_WINDOWS_HIP_GFX_TARGETS carries neither gfx1034 nor gfx1103, so the stated reason is wrong even though the answer is right. State the real reason instead. A family label is a bundle name, not an arch, so the concrete GPU is unknown at this point; answering unsupported to cover the two uncovered members would move gfx1030..1032 and gfx1100..1102 off a working HIP build onto Vulkan for a card the label cannot identify. Those two archs still reach Vulkan through the concrete-arch branch below, which does answer per repository. Comment only. No behaviour change: the 5850-combination override sweep still reports 0 rocm_gfx_target changes, 0 auto_vulkan False to True flips and 680 True to False flips all backed by a probe-confirmed HIP GPU, and both the feature and override profile matrices are byte-identical.
UNSLOTH_LLAMA_CPP_BACKEND (setup.sh / setup.ps1, "auto" or "cpu") and UNSLOTH_LLAMA_BACKEND (this module, a backend name) are separate variables at separate layers, and both accept "cpu". setup translates its own =cpu into --force-cpu, which is what pins the CPU-only bundle on a GPU host and keeps Intel iGPU Vulkan crashes away (unslothai#7213), so no trigger this PR adds may outrank it. _route_to_vulkan_prebuilt already gets this right, since force_cpu short-circuits ahead of the forced, auto-Intel and auto-no-HIP triggers. Cover it so it stays that way: the matrix runs [Linux, Windows, macOS] x [NVIDIA, AMD, Intel, CPU only] x [unset, vulkan, hip, rocm, cpu] with the legacy UNSLOTH_FORCE_VULKAN set as well, and asserts the published bundle survives every one. WSL presents as Linux to this resolver, so it rides the Linux row. Also assert the guard is not vacuous: the same host still takes Vulkan once the CPU pin is gone, so the matrix cannot pass on a resolver that had simply stopped routing to Vulkan.
|
Checked the one thing I wanted settled before this goes in: whether the new backend switch can override a deliberate CPU install.
Ran it rather than only reading it, over [Linux, Windows, macOS] x [NVIDIA, AMD, Intel, CPU only] x [unset, auto, cpu] x [unset, vulkan, hip, rocm, cpu], 240 combinations with the legacy Added that matrix as a test in 3e769cc, plus one asserting the guard is not vacuous: the same host does still take Vulkan once the CPU pin is gone, so the matrix cannot pass on a resolver that had simply stopped routing to Vulkan at all. 253 install tests pass, and 1260 in tests/studio/install. No change needed to the variable naming. The docstring at install_llama_prebuilt.py already explains why this module reads only its own variable, and the layering holds up. |
for more information, see https://pre-commit.ci
|
@codex review |
|
Codex Review: Didn't find any major issues. Keep them coming! 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". |
unslothai#7373 landed the opt-in Vulkan installer, which overlaps this branch's installer work, so install_llama_prebuilt.py takes main's backend selection wholesale: - UNSLOTH_LLAMA_BACKEND is the public selector, not UNSLOTH_LLAMA_CPP_BACKEND. The latter is a setup variable meaning auto/cpu that setup already warns about and ignores, so reading it here would opt a user into Vulkan behind that warning. main covers this with an explicit test, so this branch's parametrized selector test moves onto UNSLOTH_LLAMA_BACKEND and keeps its legacy UNSLOTH_FORCE_VULKAN alias cases. - _route_to_vulkan_prebuilt keeps main's third trigger (Windows AMD with no HIP-prebuilt gfx arch) and its --llama-backend opt-out. This branch's own contribution to that file is unchanged: _vulkan_only_attempts and _vulkan_only_release_plans still drop the generic CPU fallback so an explicit Vulkan install fails instead of silently landing on CPU. strict_vulkan now reads the resolved backend, and is deliberately not set for main's Windows-AMD auto-fallback, which is a rescue from a missing HIP arch and must keep the CPU plans it can still use.
#7507) test_shipping_code_names_an_encoding is red on main. #7373 added sync_marker_llama_backend, whose read_text/write_text pair does not name an encoding, so both fall back to locale.getencoding(): AssertionError: 2 text read/write call sites in shipping code let the operator's locale decide the encoding, so they crash or silently produce mojibake on Windows. Pass encoding = "utf-8": ['studio/install_llama_prebuilt.py:5656: write_text()', 'studio/install_llama_prebuilt.py:5647: read_text()'] Reproduced on a clean checkout of main at 7917c78: 1 failed, 7 passed. That guard landed in #7486 a few commits earlier, so the rule predates these call sites; nothing about the Vulkan work is wrong beyond the missing kwarg. The create path that writes the same file, 26 lines above at 5621, already passes encoding = "utf-8", so main is also internally inconsistent about one file: written as utf-8, read back under the operator locale. Scope, stated honestly: json.dumps defaults to ensure_ascii = True, so the marker this module writes is pure ASCII and round-trips under cp1252 as well as utf-8. The exposure is a marker produced or edited by something else. A decode failure on the read would not even surface, because UnicodeDecodeError subclasses ValueError and the surrounding except (OSError, ValueError) swallows it into the early return, leaving the backend silently unsynced. So this restores a green suite and makes the file self-consistent rather than fixing a live crash. Verified: tests/test_runtime_text_encoding.py 1 failed / 7 passed before, 8 passed after; tests/test_source_read_encoding.py still passes.
3 participants
Problem
On Windows AMD, the installer only ever picks the
win-hip-radeonprebuilt; Vulkan is auto-selected only for Intel/other GPUs (install_llama_prebuilt.py,direct_upstream_release_plan). Cards below the HIP prebuilt's gfx floor (e.g. RX 480, gfx803) are invisible to llama-server entirely.A user with an RX 9070 XT + RX 480 worked around this by manually copying the official
llama-bin-win-vulkan-x64.zipover the bundled build, which works (and Studio now understands it since #7356), but a Studio update clobbers the swap back to HIP.Fixes #7357.
Fix
UNSLOTH_LLAMA_BACKEND=vulkan(and legacyUNSLOTH_FORCE_VULKAN=1) plus--llama-backend vulkanroute the installer to the upstream Vulkan prebuilt on any host. The choice is recorded asllama_backend: "vulkan"inUNSLOTH_PREBUILT_INFO.jsonand re-asserted on Studio updates.gfx110X, etc.) and gfx1103 (Phoenix / 780M via the gfx110X bundle).--has-rocmwithout a resolved gfx arch does not force Vulkan.Verification
pytest studio/backend/tests/test_install_resolve_prebuilt.py -q -k "vulkan or llama_backend or route_to_vulkan" pytest studio/backend/tests/test_llama_cpp_update.py -q -k vulkan20 routing tests and the Vulkan update-preservation test pass with mocked host profiles.
Hardware validation not run here (no Windows AMD dual-GPU box). Suggested manual checks before merge:
llama-serversees the GPUUNSLOTH_LLAMA_BACKEND=vulkansurvives a Studio update