Studio: route AMD integrated GPUs to the Vulkan llama.cpp prebuilt - #10381
Conversation
On gfx1150/gfx1151 Vulkan is measurably the better backend. The Strix Halo CI run has Vulkan ahead of ROCm on both axes, prefill 417.0 against 339.3 t/s and decode 32.37 against 29.88, each gap wider than the within-arm spread. Every ROCm defect reported against these parts is a managed-memory fault, up to a hard k_set_rows HSA fault on Linux, that Vulkan cannot reach because it never reads GGML_CUDA_ENABLE_UNIFIED_MEMORY. Detection now installs the Vulkan prebuilt on a host whose every AMD gfx is in VULKAN_PREFERRED_GFX_TARGETS, keeping every guard the existing no-HIP fallback uses and adding one of its own: an AMD Vulkan ICD must be registered, so a box that would enumerate zero Vulkan devices keeps ROCm. The choice persists as "auto", so the recorded arch survives and a later switch back lands on the right build. An explicit backend still wins. Only the integrated parts. Discrete RDNA2/3/4 trades prefill for decode in public numbers and we have measured none of it, and ROCm ships no Vulkan ICD for CDNA, so a headless MI100 routed here would fall to CPU. Existing automatic installs migrate through the update banner rather than the hidden backend-switch path. The status now reports the backend an automatic selection would install when it differs from the one on disk, and only when the release is already current: an update at a newer release re-detects anyway. The banner offers it as an update and shows the backend pair in place of two equal tags. A deliberate backend choice is never offered a migration. Four things that had to land with the flip: - setup.ps1 lists windows-vulkan in every expected-kind branch, so a future repair of that guard cannot delete a working Vulkan install. - An OpenAI auto-switch GPU pin now records which index space it is in. ROCm pins physical ids and Vulkan compact ggml ordinals, so a pin replayed across the change addressed different cards; it is now dropped instead. - Tensor spill planning needs no change: on a Vulkan integrated host the planner declines because a shared GPU stays in the child's device list, and on ROCm it reaches plan_placement and returns "unified memory host, spilling frees no device memory". Same outcome, different log line. - The whisper re-pair needs no change: a migration carries a backend request, so the whisper phase already plans a repair pairing and degrades softly.
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. |
for more information, see https://pre-commit.ci
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
This comment was marked as resolved.
This comment was marked as resolved.
Sorry, something went wrong.
Path.home() raises rather than returning a default when it cannot resolve a home directory, which is the documented behaviour for a Windows service account with no USERPROFILE, HOMEDRIVE or HOMEPATH. Evaluating it at module scope put that in the import of install_llama_prebuilt.py itself, so the failure would have taken install and update down on every platform, over a directory only the Vulkan probe reads, and the traceback would not have mentioned Vulkan at all. Found by importing the module with Path.home() forced to raise. The test does the same, and fails against the module-level tuple.
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: 604bddd011
ℹ️ 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".
| targets = list(dict.fromkeys([*_host_rocm_gfx_targets(host), active])) | ||
| if not all(target in VULKAN_PREFERRED_GFX_TARGETS for target in targets): |
There was a problem hiding this comment.
On an Adrenalin-only Windows host without a usable hipinfo/amd-smi probe, the checked setup.ps1 path forwards only the selected $script:ROCmGfxArch via --rocm-gfx; _apply_host_overrides() therefore constructs rocm_gfx_targets with only that one value. On a mixed gfx1150/gfx1151 APU plus discrete-AMD machine where the APU is selected, this all(...) check succeeds despite the unseen discrete card and routes the whole host to Vulkan, contradicting the intended guard that any discrete AMD GPU keeps ROCm. Do not enable the preference unless the physical AMD inventory is known to be complete.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Not taking this one. _apply_host_overrides deliberately yields [gfx] on a probe that saw nothing, and its comment says why: the driver-only Windows host is precisely the case the forward exists for, so requiring a complete inventory would disable the route on the hosts it is for. The failure it prevents is also not a correctness one: an unseen discrete AMD card would get the Vulkan bundle, which runs it, rather than the ROCm bundle it would prefer. The guards that do matter here are kept, and a physical NVIDIA card or a HIP device mask still declines outright.
Two defeat the route outright on Windows, three affect the migration offer. The Radeon/Adrenalin driver registers amd-vulkan64.json in System32, not the AMDVLK amdvlk64.json the needle list knew, so the ordinary Windows gfx1150 and gfx1151 host answered False and the Windows half of this route could never fire. The basename now folds "-" to "_" before matching, which covers both spellings without a second needle. The Vulkan loader reads the registry value name as the manifest path and its DWORD data as an enable flag: zero loads it, anything else it skips (Vulkan-Loader LoaderDriverInterface.md, "If the value is 0, then the loader will attempt to load the file"). The probe appended every value regardless, so a stale or deliberately disabled AMD registration could route a working ROCm host onto a bundle that enumerates no device and falls back to CPU. Non-DWORD registrations are ignored for the same reason. get_update_status(force_refresh=True) refreshed release freshness but called the migration probe with the default, so an explicit recheck kept answering from a resolve that predated a driver or GPU change for the rest of the 24h TTL. environment_backend_override treats "auto" as a recognized value, so a bare suppression on "an override exists" withheld the offer from a host that can take it. "auto" asks for the detection the migration re-applies, and the job runs the installer with --llama-backend auto. Only a concrete selection owns the backend now. A migration is switch-shaped but update-behaved: it reinstalls at the same release on another backend. The whisper phase took the repair-only branch, which returns no phase for a self-contained install, so a pending whisper release update was dropped on the round the migration was taken. It now takes the chained plan, which both catches whisper up and re-pairs it, and falls back to the repair plan when there is nothing to catch up on. Each fix has a regression test that fails against the previous code; the repair branch keeps a negative control that passes on both.
for more information, see https://pre-commit.ci
This comment was marked as resolved.
This comment was marked as resolved.
Sorry, something went wrong.
…a name Four review fixes, all of them a value read out of the wrong place. The loader override is honoured ahead of the search directories, so a host that sets one is judged on it alone. A stale entry naming a removed driver was taken as proof of an AMD ICD on its basename, which is the failure the registry branch already refuses when it skips a disabled key: what cannot be loaded is not a driver, and routing on it lands the install on a bundle that enumerates nothing. Only manifests that are there count, and an override of nothing but stale paths falls through to the directories the loader would then search itself. A pin and the index space it is written in are one value. The one-time backfill fills field by field, so a server row holding physical ids and a browser offering Vulkan ordinals produced the ids of one space labelled with the other -- a row that looks stored and names a different GPU. The fill now skips the group whole when any part of it is already held. The banner asked the two tags whether the release had moved, and they cannot answer: installed_tag is deliberately the normalized base tag while latest_tag is the full release identity, so on a fork install they differ at the very release the machine is running. That is exactly where a backend migration is offered, so it announced a version update that does not exist and opened a changelog between a tag and itself. update_available is the field that says the release moved. And the migration resolver re-probes from scratch. Windows setup infers the arch from the GPU name when hipinfo and amd-smi are absent and forwards it, so without the replay such a host sees no ROCm GPU, resolves auto to CPU, and an install that is working on ROCm reads as drifted -- offering, in the update banner, a migration that replaces GPU inference with CPU. The arch the marker recorded is now replayed to the resolver the way the install itself already replays it, in the REMEMBERED spelling, so a live probe that does see a GPU still wins.
for more information, see https://pre-commit.ci
This comment was marked as resolved.
This comment was marked as resolved.
Sorry, something went wrong.
|
Checked the route in studio/install_llama_prebuilt.py and it stays behind the existing bail-outs plus the ICD registry check, with the choice still persisted as auto so the remembered gfx survives; the banner migration path and the earlier review points look addressed, so I will get this reviewed. |
This comment was marked as resolved.
This comment was marked as resolved.
Sorry, something went wrong.
The status path recovers the architecture from the installed bundle when the marker never recorded one, which is how the migration is offered at all on a Windows host whose probes name no gfx. The apply then read the bare marker field, so its re-resolve saw no arch, resolved auto back onto ROCm, and refused the migration the banner was still showing as already_selected.
|
@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: 4fd355ce10
ℹ️ 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".
| _install_dir_for(binary), | ||
| force_refresh = force_refresh, | ||
| published_repo = repo, | ||
| rocm_gfx = (marker or {}).get("rocm_gfx"), |
There was a problem hiding this comment.
For a managed ROCm install whose marker lacks rocm_gfx but whose asset name contains it (the Windows driver-only case _remembered_rocm_gfx() explicitly supports), this passes no architecture to the resolver. Because that subprocess cannot rediscover the GPU, Settings can report Automatic as CPU and ROCm as unavailable even while the update-status path correctly resolves the new Vulkan preference; applying Automatic can therefore install a backend different from the one the picker advertised. Pass _remembered_rocm_gfx(marker) here as well so all backend-status options use the same recovered host profile.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Right, and it is the same inconsistency as the apply path one round earlier, in the third of the three places that resolve a host profile. get_backend_status passed the bare marker field while _pending_backend_migration goes through _remembered_rocm_gfx, so on a Windows AMD box whose probes name no gfx the picker resolved Automatic to CPU and marked ROCm unavailable while the banner was offering the Vulkan migration -- two views of one machine, and applying Automatic from the picker would install neither. Fixed in a0a0e22, with test_the_picker_describes_the_same_host_the_update_check_does asserting the picker resolves Automatic to vulkan and keeps ROCm available for a marker with no rocm_gfx beside a gfx1151 bundle; on the pre-fix code it fails with assert 'cpu' == 'vulkan'.
Settings resolved its options without the architecture the installed bundle names, so an AMD host whose probes report none read as CPU-only there while the update check, which recovers it, was offering the Vulkan preference. Applying Automatic from the picker could then install a backend it never advertised.
|
@codex review |
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: a0a0e2206f
ℹ️ 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".
| for directory in _vulkan_icd_search_dirs(): | ||
| try: | ||
| paths.extend(str(entry) for entry in sorted(directory.glob("*.json"))) |
There was a problem hiding this comment.
On a gfx1150/gfx1151 host with VK_LOADER_DRIVERS_SELECT or VK_LOADER_DRIVERS_DISABLE configured to exclude the AMD manifest, this scan still counts that manifest as usable even though the Vulkan loader will not load it. The automatic route can consequently replace working ROCm acceleration with a Vulkan build that has no AMD device and may run on CPU; apply the loader's manifest filters before using the discovered paths as evidence for the preference.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Correct, and fixed in b8cda58. The scan honours VK_LOADER_DRIVERS_DISABLE and VK_LOADER_DRIVERS_SELECT before treating a manifest as evidence of a device: disable is read first and a select list answers alone when set, matching the loader's own order and its four basename globs, case-insensitively. The filters apply to a VK_DRIVER_FILES force list too, so that path is covered as well. Two tests, both failing on the pre-fix code: a filtered-out radeon manifest no longer answers for the driver, and a select list naming another vendor excludes it on its own.
…ulkan VK_LOADER_DRIVERS_DISABLE and VK_LOADER_DRIVERS_SELECT exclude a registered driver from ever being loaded, and they apply to every driver the loader knows, a VK_DRIVER_FILES force list included. The ICD scan counted a filtered-out AMD manifest as evidence of a device, so an integrated host with the AMD driver filtered out would have swapped working ROCm for a Vulkan build with no device.
|
@codex review |
for more information, see https://pre-commit.ci
|
Codex Review: Didn't find any major issues. Keep it up! 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". |
What this looks like for a ROCm userTwo isolated Studio installs, base
Before, nothing is drawn at all: the release is current, so
The numbers behind the picture, read from each photographed server rather than counted by eye:
Both sides sit at the same release with no version update on offer, so the pair isolates the backend offer rather than an ordinary update. How the host was arranged, since it mattersThe machine this ran on has eight NVIDIA GPUs, so the AMD host is simulated. A Checked before any screenshot was taken: the resolver alone answers One cosmetic difference to ignore: the chat greeting is chosen at random per load, so the two halves show different text there. |
The migration banner's sub-line only said no restart was needed, which is true of every update and says nothing about why to take this one. On gfx1151 the measured gap is prefill +21.9% and decode +7.3% in Vulkan's favour, so name it. Gated on the migration offer and on the rocm -> vulkan pair specifically: a version update, and a backend the user picked by hand, keep the plain line, and a migration between any other pair would not be describing those two backends.
|
You have reached your Codex usage limits for security reviews. Please try again later. |
|
Refreshed the banner evidence at the current head ( Same construction: BEFORE is this PR's merge base
The full window, otherwise identical apart from the randomised greeting:
What the photographed servers themselves report, read from
Both sides sit at the current release with no version update on offer, which is the state this has to work in: The note is scoped to the migration. It renders under the same condition that titles the banner "New llama.cpp backend", and only for a ROCm to Vulkan move, so an ordinary version update and a backend picked by hand in Settings both keep the plain "No restart needed after update". Disclosure, since it is not obvious from the picture: this box is 8x NVIDIA, so both sides run under the same shim that hides On the number, so it is on the record next to the screenshot: the gfx1151 CI runner measured Vulkan ahead of ROCm by 21.9% on prefill (417.0 against 339.3 t/s) and 7.3% on decode (32.4 against 29.9 t/s), |
1 participant
On gfx1150 and gfx1151 Vulkan is measurably the better llama.cpp backend, so detection now installs the Vulkan prebuilt there instead of ROCm, and an existing automatic ROCm install is offered the change through the ordinary update banner.
Why
Measured on a Strix Halo CI runner (Radeon 8060S, 128 GB box with a 64 GiB carve-out),
llama-benchon Qwen3.8-Flash-Next UD-IQ1_S, arms alternated inside one job, 3 cycles:The gap is wider than the within-arm spread on both axes.
Separately, every ROCm defect reported against these parts is a managed-memory fault. On a Linux gfx1151 runner with
GGML_CUDA_ENABLE_UNIFIED_MEMORYset,llama-serverfaults ink_set_rowson the first decode on bothb10715-mixandb10798-mix, and the same binaries are clean with the variable absent or=0, as is Vulkan. Vulkan never reads that variable, which is the whole of the "Vulkan works, ROCm does not" pattern in these reports.What changes
_should_prefer_vulkan_for_amd_igpuis a third reason inside the existing_route_to_vulkan_prebuilt, beside the two the no-HIP fallback already had, so it inherits every guard that path uses: an unknown arch bails, a physical NVIDIA card bails, aHIP_VISIBLE_DEVICESmask bails, and every physical gfx is judged rather than just the active one. It adds two of its own: membership inVULKAN_PREFERRED_GFX_TARGETS, and an AMD Vulkan ICD being registered with the loader.The ICD check is what makes this safe rather than merely narrow.
_run_vulkan_probecannot be used here: it runs a helper against an installed llama.cpp, and this decision is made inside the installer before that build exists. The router reads the loader's own manifest registry instead,HKLM\SOFTWARE\Khronos\Vulkan\Drivers(and WOW6432Node) on Windows,VK_DRIVER_FILES/VK_ICD_FILENAMESthen the four standardicd.ddirectories on Linux, matching the manifest basename against the names AMD's drivers actually register. A host that would enumerate zero Vulkan devices keeps ROCm.The choice persists as
auto, notvulkan, sorocm_gfxsurvives in the marker, the updater keeps forwarding the remembered arch, and the auto-Vulkan CPU crash fallback stays armed. An explicit--llama-backendorUNSLOTH_LLAMA_CPP_BACKENDstill wins.Only the integrated parts. Discrete RDNA2/3/4 trades prefill for decode in public numbers and none of it is measured here. CDNA is the reason this is an allowlist rather than "all AMD": ROCm ships no Vulkan ICD, so a headless MI100 routed this way would enumerate zero Vulkan devices and fall to CPU, which is a dead backend rather than a slower one. Widening is a one-line constant change once there is a measurement behind it.
Migrating existing installs
Through the update banner, not the backend-switch path, which the banner deliberately hides.
A plain update at a newer release already re-detects the backend, because the installer is run with no
--llama-backendwhen no request is recorded. So the migration is only needed, and is only offered, when the release on disk is already current._pending_backend_migrationreports the backend a re-applied automatic selection would install when it differs from the one on disk, the status carriesbackend_migration_available/from_backend/to_backend, and the banner presents it as an update, showing the backend pair in place of two identical tags.A deliberate backend choice is never offered a migration: a concrete recorded request is applied by definition, so re-selecting ROCm ends this permanently with no new marker state.
Four things that had to land alongside the flip
setup.ps1listswindows-vulkanin every$expectedKindsbranch. The guard is inert today because nothing writesinstall_kind, but a future repair of it would otherwise delete a working Vulkan install on every setup run.physical, so no existing settings file changes._planned_tensor_spilldeclines because a shared-memory GPU stays in the child's device list; on ROCm it proceeds andplan_placementreturns "unified memory host, spilling frees no device memory". Same outcome either side of the flip, different log line.Testing
1225 pass and 2 skip across the twelve backend suites this touches:
test_install_resolve_prebuilt,test_llama_backend_switch,test_llama_cpp_update,test_combined_update,test_openai_auto_switch,test_llama_backend_marker,test_llama_backend_selection,test_llama_cpp_freshness,test_llama_route,test_setup_llama_cpp_backend,test_update_flow_messages,test_llama_cpp_vulkan_probe. 29 frontend cases pass undernode --experimental-strip-types --test.New coverage includes the route firing on both platforms, declining without an AMD Vulkan driver, declining beside a discrete card or a physical NVIDIA card, declining under a HIP device mask, an explicit backend opting out, a forced Vulkan selection on an integrated host persisting as automatic, the ICD probe reading the loader overrides first and judging the manifest name rather than the directory, a drifted automatic install being offered as an update, a deliberate choice never being offered one, and a pin written in the other index space being rejected.
Two notes on what is not covered. There is no hardware verification of the flip itself, so the namespace, no-ICD, rollback and idempotency arms are unit tests rather than a Strix Halo run. And the +22.9% is one model on one integrated GPU; it is not quoted for any part that has not been measured.