Repository navigation
Conversation
|
Hi @bogdanr, thanks for your contribution! Per our contribution guidelines, the automated PR checker found the following issue(s) that need your attention:
Please note that maintainers reserve the right to make final decisions on PRs. If you believe there is a mistake, please comment below. |
vk::DeviceLostError escaped ggml_backend_vk_graph_compute and unwound through llama_decode, which is extern "C", so the process aborted. Catch it and fail the graph instead. The flag is sticky and lives on vk_device because a lost device invalidates every object created from it. Assisted-by: Claude Code
79097f4 to
3c6a09c
Compare
|
Updated: description now follows the template, and I closed the second PR (#27184) so this one stands alone. Force-pushed the branch with tidier comments and a proper commit message. |
|
This seems ok. What's the app-level behavior after the device is lost? Will llama-cli continue trying to execute and spam a bunch of error message, or will it cleanly stop? |
|
It doesn't continue trying. It produces an error and then releases the processing slot. I've updated the PR message with a better description. |
0cc4m
left a comment
There was a problem hiding this comment.
Get rid of most of the comments. AI tends to overcomment a lot. Only keep concise hand-written comments where code is unclear.
| try { | ||
| ggml_vk_synchronize(ctx); | ||
| } catch (const vk::SystemError &) { | ||
| ctx->device->device_lost = true; |
|
|
||
| ggml_vk_graph_cleanup(ctx); | ||
| } catch (const vk::DeviceLostError &) { | ||
| ctx->device->device_lost = true; |
|
|
||
| if (ctx->device->device_lost) { | ||
| GGML_LOG_ERROR("%s: device %s was lost by an earlier submission - recreate the backend to recover\n", | ||
| __func__, ctx->device->name.c_str()); |
There was a problem hiding this comment.
This text is unhelpful (AI-generated, I guess) and this log is not needed. It should log once when the device is lost, then stop.
| } catch (const vk::DeviceLostError &) { | ||
| // a lost device invalidates every object created from it, so fail all later calls too | ||
| ctx->device->device_lost = true; | ||
| GGML_LOG_ERROR("%s: device %s was lost - recreate the backend to recover\n", __func__, ctx->device->name.c_str()); |
There was a problem hiding this comment.
More AI-generated logging, keep it to the point and avoid suggestions.
|
Also set this to ready for review if it is, it was automatically set to draft because you hadn't used the PR template. |
Resolved against the post-ggml-org#28732 source split: the PR's pre-split copies of the device/queue structs are dropped in favour of ours, and device_lost is added to vk_device_struct in ggml-vulkan-types.h where it now lives. Assisted-by: Claude Opus 5
Overview
When a Vulkan device is lost mid-graph,
vulkan.hppthrowsvk::DeviceLostErrorout ofggml_backend_vk_graph_compute. Nothing catches it, so it unwinds throughllama_decode, which isextern "C", and the whole process aborts:The caller cannot defend against this. There is no status to inspect and nothing to catch - the application is gone.
This catches the error and returns
GGML_STATUS_FAILEDinstead, so a lost device fails the call rather than the process.A lost device invalidates every object created from it, so the flag is sticky and lives on
vk_device, not on the backend context: several contexts can share one device and none of them are usable afterwards. Once it is set, every latergraph_computereturnsGGML_STATUS_FAILEDimmediately until the backend is recreated.Two more places needed the same treatment for that to hold:
ggml_backend_vk_synchronizereturnsvoidand can see the same loss, so it records the flag and leaves the reporting to the nextgraph_compute.ggml_vk_cleanupruns from the backend destructor, where a throw aborts just as hard, so itsggml_vk_synchronizecall is wrapped too.No interface change, no other backend touched.
This is the pattern Metal already uses -
has_erroringgml-metal-context.m, with the same comment about the backend having to be recreated. This brings Vulkan in line with it.Additional information
Where I hit it: Intel Lunar Lake integrated graphics (
Intel(R) Graphics (LNL)), Mesa Vulkan,xekernel driver. One HTTP request is enough: prefill ~10,200 tokens of a 26B model with all layers resident. Kernel side, one line per abort:The trigger is the driver's job watchdog, not anything ggml does wrong. Raising
job_timeout_msfrom 5000 to 10000 and changing nothing else let the identical prompt finish (471.6 s, no abort). Anything that overruns the per-job deadline will do it, so this is not specific to my hardware - any driver with a watchdog can lose a device this way.It cannot be avoided from the caller side. Each of these was tried against the same reproducer:
op_offloadoff, all layers residentn_ubatch512 -> 128GGML_VK_MAX_NODES_PER_SUBMIT100 -> 25GGML_VK_DISABLE_COOPMAT=1,GGML_VK_DISABLE_COOPMAT2=1n_batch2048 -> 256, three runsThe last row is the telling one: the same setting both survives and dies, so no parameter reliably keeps a job under the deadline.
To be clear about what this does and does not do: it does not stop the device being lost. It stops the device loss taking the process with it. Recovery still means tearing the backend down and building a new one.
What the apps do after the device is lost. The failure comes back as
GGML_STATUS_FAILED-> decode returns-3, which both frontends already handle:llama-completion/llama-cli:common_prompt_batch_decodelogsfailed to evalonce and returns false, andmainreturns 1. Clean exit, one message.llama-server:ret < -1maps toCompute error., so every processing slot gets an error response and is released, andupdate_slotsbreaks out of the batch loop. The server stays up; a later request fails the same way, one log line each. It does not spin - the retry-with-half-batch path only runs for the KV-space case (ret == 1), never for a compute error.The sticky flag matters here: once set,
graph_computereturns before touching the device at all, so nothing is submitted to a dead queue in a loop. Metal logs on that path per call too, and I kept it the same.Related: #25664, #27076.
Two things I deliberately left out, happy to add either if you want them here rather than in a follow-up:
ggml_backend_vk_buffer_*fast once the device is lost. Bigger diff, and the flag is already in the right place to do it later.llama_encode/llama_decodein try/catch so no backend can unwind across the C boundary. That one is a separate concern from the Vulkan bug; I will open it after this lands.Builds clean with
GGML_VULKAN=ON. I will post the result of re-running the reproducer against a patched build - expected outcome is a failed decode and a live process instead of an abort.Requirements
Assisted-by:trailer.