fix(updater): migrate gpu-vulkan slot TOMLs to gpu-rocm/cpu - #1934
fix(updater): migrate gpu-vulkan slot TOMLs to gpu-rocm/cpu#1934thinmintdev wants to merge 5 commits into
Conversation
PR #1923 retired the Vulkan LLM lane and made load_sync refuse a GPU slot without /dev/kfd, but deliberately left existing on-disk slot TOMLs unmigrated — any install with device="gpu-vulkan" slots refuses to load them after updating (#1924). Add relabel_stale_vulkan_slots as a sixth post-activation migration pass, following the same raw-TOML-surgery convention as retag_stale_slot_images/clear_stale_mtp_overrides: relabel device from gpu-vulkan to gpu-rocm when /dev/kfd is present, or to cpu (with a loud WARNING breadcrumb, since this is a real behavior change) when it is not. Only the device key is touched, and the pass is idempotent since a relabeled slot no longer matches the gpu-vulkan guard. Refs #1924 #1888
The migration's docstring used "legacy", which scripts/check_sunset.py's scar-marker regex flags — reword to "retired" (no behavior change).
![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: 5169a161a8
ℹ️ 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".
| holder = raw["slot"] | ||
| if holder is None: | ||
| continue | ||
| new_device = "gpu-rocm" if have_kfd else "cpu" |
There was a problem hiding this comment.
This host-wide choice rewrites every gpu-vulkan slot based solely on /dev/kfd, although Vulkan remains valid for other configurations. Intel/NVIDIA hosts have no KFD by design, so working Vulkan LLM slots are permanently downgraded to CPU; additionally, Vulkan-only provider slots such as Whisper.cpp, Kokoro, VibeVoice, and ComfyUI are changed to either CPU or unsupported ROCm depending on the probe. Filter by the affected AMD llama.cpp runtime/host instead of migrating all Vulkan slots.
Useful? React with 👍 / 👎.
| # operator flips MTP to Auto/Off in the drawer. | ||
|
|
||
| try: | ||
| relabel_stale_vulkan_slots(job_id=job_id) |
There was a problem hiding this comment.
In the checked Updater.commit() self-update flow, run_post_activation_migrations() executes before seam.activate() installs the target release into the shared venv. Consequently, an upgrade into the release containing this new call runs the old process's version of this function, which has no Vulkan migration; after activation, the new KFD guard is present but the legacy slot TOMLs remain unchanged. Invoke this newly shipped migration through the staged/activated code so it runs on the update that introduces it.
Useful? React with 👍 / 👎.
| from hal0.config.paths import slots_config_dir | ||
| from hal0.providers._gpu import kfd_present as _probe_kfd_present | ||
|
|
||
| have_kfd = _probe_kfd_present() if kfd_present is None else kfd_present |
There was a problem hiding this comment.
On the checked install.sh repair/upgrade path, this probe runs as root, while kfd_present() explicitly uses os.access() to determine whether the current process can read and write /dev/kfd; the service later loads slots as User=hal0. If KFD exists but its group mapping or permissions do not admit hal0—the LXC failure mode the probe is meant to catch—root can select gpu-rocm, then the restarted service fails the same KFD preflight and cannot load the slot. Run the usability probe with the service user's credentials rather than the installer's.
Useful? React with 👍 / 👎.
Codex review on #1934 (P1): the migration was relabeling ANY device = "gpu-vulkan" slot based solely on kfd_present(), but require_kfd_for_gpu_slot (added in #1923) only ever gates a gpu-vulkan slot on an AMD host (host_is_amd_gpu()). On Intel/NVIDIA hardware /dev/kfd never exists by design, so a working non-AMD Vulkan slot was never broken by #1923 — and the unscoped migration would have silently downgraded it to cpu on its next update, since kfd_present() is also (correctly) False there. Add the same amd_host gate require_kfd_for_gpu_slot uses: on a non-AMD host the migration is now a full no-op (no writes, no log lines). Refs #1924 #1888
|
Triaged the Codex review comments:
|
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: b86408439f
ℹ️ 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".
| if raw.get("device") == "gpu-vulkan": | ||
| holder = raw | ||
| elif isinstance(raw.get("slot"), dict) and raw["slot"].get("device") == "gpu-vulkan": | ||
| holder = raw["slot"] |
There was a problem hiding this comment.
When a legacy nested TOML also contains a duplicated top-level device = "gpu-vulkan"—a drift shape explicitly handled by slot_scalar_table()—this branch selects the root dictionary. If both copies are Vulkan, only the ignored root value is relabeled; _flatten_slot_toml() treats [slot] as authoritative, so the next load still sees gpu-vulkan and the slot remains broken despite the migration logging success. Select the holder through slot_scalar_table(raw), which gives any [slot] table precedence.
Useful? React with 👍 / 👎.
| holder["device"] = new_device | ||
| try: | ||
| write_toml_atomic(toml_path, raw) |
There was a problem hiding this comment.
When an update runs in the background while an operator or API request edits an affected slot, this migration reads and replaces the TOML without the coarse slot_write_lock() used by the normal slot writers. A PATCH that lands after this read but before write_toml_atomic() is overwritten by the stale raw dictionary, silently losing the concurrent configuration change. Hold slot_write_lock() across the read-through-write operation and use the shared slot writer seam.
Useful? React with 👍 / 👎.
| if raw.get("device") == "gpu-vulkan": | ||
| holder = raw | ||
| elif isinstance(raw.get("slot"), dict) and raw["slot"].get("device") == "gpu-vulkan": | ||
| holder = raw["slot"] |
There was a problem hiding this comment.
On older supported slot TOMLs that contain backend = "vulkan" but no device, this guard never selects a holder, even though SlotConfig._promote_backend_to_device() explicitly treats that shape as gpu-vulkan. The manager's raw slot load then reaches the container preflight without a device, so the KFD guard also skips it and an AMD host without KFD can continue launching the invalid Vulkan lane after this migration. Normalize the authoritative scalar table's legacy backend value and persist the corresponding device during this pass.
Useful? React with 👍 / 👎.
| holder["device"] = new_device | ||
| try: | ||
| write_toml_atomic(toml_path, raw) |
There was a problem hiding this comment.
When an affected Vulkan slot is active during an installer upgrade—or once the self-update invocation is corrected—this write changes only its TOML. The checked post-swap and installer unit-rerender paths deliberately do not restart slot services, so the existing container keeps its old Vulkan devices and argv; on the CPU branch the warning even claims the slot no longer uses the GPU while the live process still does. For the #1888 failure mode, that process can continue returning invalid tokens until an unrelated reload or reboot, so changed active LLM slots need to be stopped/reloaded or surfaced as requiring immediate operator action.
Useful? React with 👍 / 👎.
Independent review — verdict: request changes (1 blocking, 3 non-blocking)Reviewed at What holds up
Revert-and-confirm-redSix mutations against the merge head, running
Baseline restored → 19 passed. Full suite 1. BLOCKING — the migration relabels non-llama.cpp
|
Changes-requested review on #1934 (blocking): the migration matched on the bare device = "gpu-vulkan" string, so it also rewrote gpu-vulkan slots belonging to non-llama runtimes (Kokoro TTS, whisper.cpp/Moonshine STT, ComfyUI) that legitimately run Vulkan images — capabilities/ catalog.py deliberately keeps gpu-vulkan for exactly those runtimes. On a kfd-present box this relabeled a working Vulkan slot to gpu-rocm, a label its image can't honor (gpu_visibility_env silently swaps GGML_VK_VISIBLE_DEVICES -> HIP_VISIBLE_DEVICES, dropping any gpu_index pin); on a kfd-absent box it demoted a working Vulkan STT/image-gen slot to cpu, stripping /dev/dri. Gate on hal0.providers.container._spec_provider_for — the same runtime-family discriminator load_sync itself uses — and skip a candidate slot entirely (no relabel, no warning, no log, on either kfd axis) unless it resolves to None (the default llama-server GPU provider). The kfd-absent non-llama case (require_kfd_for_gpu_slot itself over-firing for non-llama runtimes) is tracked separately as #1941 and is deliberately not addressed here. Also: - correct the docstring's false "comments survive" implication — write_toml_atomic reserializes via tomli_w and drops them, same as every other pass in this module (non-blocking review finding) - add the vulkan-slot pass to install.sh's post-activation-migrations transcript, which still only named four passes (non-blocking) - update the run_post_activation_migrations test stub to accept amd_host, matching the real signature (non-blocking) TDD: the seven new runtime-scope tests were confirmed RED against the pre-fix code (kokoro/comfyui/transcription slots were being relabeled) before this commit made them green. Refs #1924 #1888
|
You have reached your Codex usage limits for code reviews. You can see your limits in the Codex usage dashboard. |
|
Addressed all four findings from the changes-requested review, pushed in f515543. Blocking — runtime scopingFixed. The migration now resolves every Per your note, the kfd-absent non-llama case (i.e. New TDD coverage in Non-blocking
Verification
Not merging or enabling automerge — awaiting review. |
Re-review of
|
| Mutation | Result |
|---|---|
remove the _spec_provider_for block entirely (pre-fix behavior) |
7 failed / 15 passed |
keep the call, drop only the skip (if False:) |
7 failed / 15 passed |
invert the gate (if provider is None: — skip llama, relabel non-llama) |
18 failed / 4 passed |
| exception branch falls through to relabel instead of skipping | 22 passed ← see nit 1 |
Exactly 7 red on revert, matching the claim. Baseline restored → 22 passed; full env -u FORCE_COLOR HAL0_HOME=$(mktemp -d) uv run --extra dev pytest tests/updater tests/install -q → 538 passed.
Re-verified post-revision
- Device-key-only still holds, now proven by value rather than by reading: I round-tripped a slot carrying
runtime,port,n_gpu_layers,gpu_index,[model]and[server].env, deleteddevicefrom both sides, and asserted the parsed documents are equal. Onlydevicechanged. - Idempotency still holds and is now stronger than the suite asserts: the second run leaves the file byte-identical (not merely "device unchanged" / "no new log lines") — the
continueprecedes the write, so no rewrite occurs. - Non-AMD no-op unchanged: still returns before the directory is globbed.
Non-blocking follow-ups (2, 3, 4 from the first pass — all landed)
- Docstring comment claim: fixed, and better than asked — it now names
write_toml_atomic'stomli_wreserialization and states that comments are dropped, plus notes the same is true ofretag_stale_slot_images/clear_stale_mtp_overrides. - install.sh transcript: fixed in all three places (block comment,
infoline, printed summary), in the order the passes actually run. - Stub signature: fixed (
amd_hostadded). - PR body test-count claim: corrected (22 new + 4 edited).
Nits — not gating
- The exception branch is untested. Mutating it to relabel-instead-of-skip leaves all 22 tests green. It behaves correctly (I verified out-of-band), but nothing guards it against a future edit. A ~10-line test monkeypatching
hal0.providers.container._spec_provider_forto raise, asserting file-unchanged + theruntime_unresolvedevent, would close it. Same for the profile-family path (type=llm+ a non-llamaprofile), which is the discriminator's more interesting half and is currently only covered bytype-shaped slots. - Private cross-module import.
from hal0.providers.container import _spec_provider_forcouples the updater to a private name. Justified here (parity withload_syncis the whole point, and a public alias would be its own churn), but worth a one-line note at the import so a future rename doesn't silently change this migration's scope. - Docstring formatting artifact —
"…logs nothing\n new."inrelabel_stale_vulkan_slotswrapped mid-phrase. Cosmetic.
CI
All green on this head, including γ-suite (chromium) (11m58s pass — the #1926 flake did not recur).
No blocking objections. Approving.
…gate Approved-with-nits follow-up on #1934: the except branch of the _spec_provider_for gate (added for the runtime-scoping blocking fix) was untested — a mutation that turned its `continue` into an optimistic relabel would have passed all 22 existing tests. Add test_runtime_resolution_error_leaves_slot_untouched_and_logs, which monkeypatches _spec_provider_for to raise and asserts: the slot TOML is byte-identical after the run, the pass returns 0 (no crash), and updater.vulkan_migration_slot_runtime_unresolved is logged with the slot name and error. Confirmed this test fails when the except branch is mutated to fall through as llama.cpp instead of skipping (verified locally, reverted before commit — not left in the tree). Also add test_qwen3tts_profile_slot_survives_untouched, pinning the profile-family-shaped case (a slot dispatched to a non-llama runtime via its `profile` field rather than device/type) as a regression guard — reviewer manually verified this path but nothing pinned it. And a one-line comment on the _spec_provider_for import noting the parity-with-load_sync is intentional, so it isn't "cleaned up" into a public wrapper or re-derived locally. Refs #1924 #1888
|
Pushed the two nit tests + import comment in b7600d7.
Verification: Not merging — leaving for operator merge. |
|
You have reached your Codex usage limits for code reviews. You can see your limits in the Codex usage dashboard. |
1 participant
Summary
PR #1923 retired the Vulkan LLM lane (#1888) and made
container.py'sload_syncrefuse to load a GPU slot whosedeviceisgpu-vulkanorgpu-rocmwhen/dev/kfdis not visible. It deliberately relabeled only the installer's seed slot TOMLs (installer/etc-hal0/slots/*.toml) fromgpu-vulkantogpu-rocm, and explicitly left slot TOMLs already materialised on an existing install untouched — so any pre-#1923 install withdevice = "gpu-vulkan"slots refuses to load them after updating, until an operator manually relabels the TOML (raised as open question 3 in #1923's report, filed as #1924).This PR adds an updater migration that does that relabel automatically.
Design
relabel_stale_vulkan_slotsis a new sixth pass inrun_post_activation_migrations(src/hal0/updater/updater.py) — the single sequence bothUpdater.commit()(self-update) andinstall.sh's repair/upgrade-in-place path call, so both upgrade routes converge on the same on-disk state.gpu-rocm:/dev/kfdpresent and usable (hal0.providers._gpu.kfd_present(), added in fix(gpu): require /dev/kfd and retire the Vulkan LLM lane #1923) — matches PR fix(gpu): require /dev/kfd and retire the Vulkan LLM lane #1923's own seed relabel exactly.cpu:/dev/kfdnot present. This is a genuine operator-visible behavior change (the slot drops off the GPU and gets much slower), so it is logged as its own distinct WARNING-level breadcrumb (updater.slot_vulkan_relabeled_cpu_fallback, note prefixedBEHAVIOR CHANGE:) — never folded silently into the routine relabel log line (updater.slot_vulkan_relabeled_rocm).hal0.providers._gpu.host_is_amd_gpu()), exactly matchingrequire_kfd_for_gpu_slot's own scope. A non-AMD host (Intel iGPU, NVIDIA without CDI) has no/dev/kfdby design, was never gated by fix(gpu): require /dev/kfd and retire the Vulkan LLM lane #1923's guard, and is a full no-op here — no relabel, no log.device = "gpu-vulkan"is not exclusively an llama.cpp label —capabilities/catalog.pydeliberately keeps it for the non-llama runtimes (Kokoro TTS, whisper.cpp/Moonshine STT, ComfyUI), which run genuinely-Vulkan images. Every candidate slot is resolved throughhal0.providers.container._spec_provider_for— the same discriminatorload_syncitself uses — and is skipped ENTIRELY (no relabel, no warning, no log, on either kfd axis) unless it resolves toNone(the default llama-server GPU provider). The kfd-absent non-llama case (require_kfd_for_gpu_slotitself over-firing for non-llama runtimes) is a separate, already-filed bug tracked as gpu: #1923's /dev/kfd guard over-fires on non-llama Vulkan runtimes — kokoro/whisper.cpp/ComfyUI slots refused on kfd-less AMD boxes #1941 and is deliberately NOT addressed here.tomllibload /write_toml_atomicdump, same convention as the existingretag_stale_slot_images/clear_stale_mtp_overridespasses in this module. Only thedevicekey's value is ever changed (top-level or nested under[slot], matching the two TOML shapes those passes already handle); every other field, and every slot whosedeviceis not literally"gpu-vulkan"or that isn't llama.cpp-backed, is left untouched. Note this is narrow-scope-by-key, not byte-for-byte file preservation — like every other pass in this module, a touched file's comments are dropped on rewrite (write_toml_atomicreserializes the whole parsed document viatomli_w).devicereads"gpu-rocm"or"cpu"it no longer matches the"gpu-vulkan"guard, so a second run touches nothing and emits no new log lines (covered bytest_relabel_is_idempotent,test_relabel_is_idempotent_on_cpu_fallback,test_idempotent_second_run_logs_nothing_new).slot,old("gpu-vulkan"),new("gpu-rocm"or"cpu"), andjob_id, viastructloglog.warning— same shape as the other post-activation passes.This should ship in the same release as #1923 — otherwise any pre-#1923 install with
gpu-vulkanLLM slots hits the new load-time refusal with no automatic recovery path.Test plan
tests/updater/test_vulkan_slot_migration.py(new file, 22 tests) — TDD throughout, including the runtime-scope fix (RED first against the pre-fix code, confirmed the non-llama tests failed, then implementation). Covers: kfd-present → gpu-rocm relabel (flat + nested[slot]shape), kfd-absent → cpu relabel with a loud distinct breadcrumb, AMD-host scoping (non-AMD host is a full no-op, real-probe default), runtime scoping (Kokoro/ComfyUI/transcription-shaped slots survive completely untouched on both kfd axes, alongside a genuine llama.cpp slot that still relabels), narrow scope (other keys/slots untouched), unreadable-TOML is skipped not fatal, idempotent re-run (both branches, including "logs nothing new"), and the default (no override) probes the realkfd_present()/host_is_amd_gpu().tests/updater/test_post_activation_migrations.py(4 existing tests edited, stub signature updated foramd_host) to stub and assert the new sixth pass runs in order, alongside the others, and is skipped when the schema migration fails first.installer/install.sh's post-activation-migrations transcript/log updated to name the vulkan-slot pass alongside the other four.env -u FORCE_COLOR HAL0_HOME=$(mktemp -d) uv run --extra dev pytest tests/updater tests/install -q→ 538 passeduv run ruff format --check src tests→ all formattedmake lint→ all checks passedpython scripts/check_sunset.py→ scar markers at baselineRefs #1924 #1888. Related: #1941 (tracks
require_kfd_for_gpu_slotitself over-firing for non-llama Vulkan runtimes on a kfd-absent AMD host — out of scope for this migration).