GGUF kernels: raise on unsupported quant type instead of returning uninitialized memory - #138
Open
vcruz305 wants to merge 1 commit into
Open
GGUF kernels: raise on unsupported quant type instead of returning uninitialized memory#138vcruz305 wants to merge 1 commit into
vcruz305 wants to merge 1 commit into
Conversation
None of the five switch (type) blocks in gguf_kernel.cu had a default: case, and the output tensor is allocated with torch::empty, so an unsupported quant type returned uninitialized memory rather than raising. ggml_moe_get_block_size fell through to return 0, which then divides or sizes a launch by zero downstream. The gap is reachable: ggml_mul_mat_vec_a8 and ggml_moe_a8_vec handle 19 types while ggml_mul_mat_a8 and ggml_moe_a8 handle 10, because the I-quants have no MMQ kernel. Any caller routing an I-quant to the MMQ path gets silent garbage, and the only symptom is a model that loads, runs at full speed, and produces fluent nonsense. Each default: now raises via TORCH_CHECK naming the function and the type families that entry point supports. ggml_dequantize also null-checks the ggml_get_to_cuda function pointer, which returns nullptr for an unknown type and was being called unconditionally. No kernel math changed. A comment above the first guard notes these kernels are vendored from sgl-kernel so a future re-vendor does not drop them.
|
Failure exists on any backend where an unsupported quant path falls through, +1 |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Splitting this out of #131 because it stands on its own and is quick to review. It is a correctness fix with no feature attached.
None of the five
switch (type)blocks ingguf_kernel.cuhas adefault:case, and the output tensor is allocated withtorch::empty. So passing a quant type the kernel does not implement returns uninitialized memory instead of raising:ggml_moe_get_block_sizehas the same shape and falls through toreturn 0, which then divides or sizes a launch by zero downstream.The gap is real rather than theoretical.
ggml_mul_mat_vec_a8andggml_moe_a8_vechandle 19 types, whileggml_mul_mat_a8andggml_moe_a8handle 10: the I-quants have no MMQ kernel. So any caller that routes an I-quant to the MMQ path today gets silent garbage, and the only symptom is a model that loads fine, runs at full speed and emits fluent nonsense. I lost a fair amount of time to exactly that before adding these guards.Each
default:now raises throughTORCH_CHECKand names the function plus which type families that entry point actually supports. I also null-check theggml_get_to_cudafunction pointer insideggml_dequantize, since it returnsnullptrfor an unknown type (dequantize.cuh) and the result was being called unconditionally.No kernel math is touched. The only behaviour change is that an unsupported type now fails loudly at the call instead of producing wrong numbers. There is a comment above the first guard noting these kernels are vendored from sgl-kernel and that the guards are a local addition, so a future re-vendor does not quietly drop them.
Found while working on #131 (GGUF quant types and qwen35moe), but it is independent of that branch.