Skip to content

Conversation

CISC
Copy link
Collaborator

@CISC CISC commented Aug 17, 2025

Force patch_embd weights to F16 or F32 to avoid broken GGUFs (f.ex. when using --outtype bf16) as IM2COL op requires F16 or F32. Only use F16 if forced by user or guessed, F32 being the safest choice and the tensor is not that large anyway.

@CISC CISC requested a review from ngxson August 17, 2025 09:26
@github-actions github-actions bot added the python python script changes label Aug 17, 2025
@CISC CISC requested a review from ngxson August 17, 2025 10:23
@CISC CISC changed the title convert : force patch_embd weights to F32 to avoid broken GGUFs convert : force patch_embd weights to F16 or F32 to avoid broken GGUFs Aug 17, 2025
@ngxson
Copy link
Collaborator

ngxson commented Aug 17, 2025

I mean we can extend this list:

gguf.MODEL_TENSOR.FFN_GATE_INP,
gguf.MODEL_TENSOR.POS_EMBD,
gguf.MODEL_TENSOR.TOKEN_TYPES,
gguf.MODEL_TENSOR.SSM_CONV1D,
gguf.MODEL_TENSOR.SHORTCONV_CONV,
gguf.MODEL_TENSOR.TIME_MIX_FIRST,
gguf.MODEL_TENSOR.TIME_MIX_W1,
gguf.MODEL_TENSOR.TIME_MIX_W2,
gguf.MODEL_TENSOR.TIME_MIX_DECAY_W1,
gguf.MODEL_TENSOR.TIME_MIX_DECAY_W2,
gguf.MODEL_TENSOR.TIME_MIX_LERP_FUSED,
gguf.MODEL_TENSOR.POSNET_NORM1,
gguf.MODEL_TENSOR.POSNET_NORM2,
gguf.MODEL_TENSOR.V_ENC_EMBD_POS,
gguf.MODEL_TENSOR.A_ENC_EMBD_POS,
gguf.MODEL_TENSOR.ALTUP_CORRECT_COEF,
gguf.MODEL_TENSOR.ALTUP_PREDICT_COEF,

But not 100% sure if it works though

@CISC
Copy link
Collaborator Author

CISC commented Aug 17, 2025

I mean we can extend this list:
But not 100% sure if it works though

Sure, I understood, I just wasn't sure it did either. :)

Edit: It works, but patch_embd doesn't have to be F32.

Anyway, since it's only a fallback for tensor_force_quant, and several Mmproj models already did this I think the solution I just committed is better.

@ngxson
Copy link
Collaborator

ngxson commented Aug 17, 2025

IM2COL op requires F16 or F32

On second thought, this is a bit strange. IM2COL takes 2 inputs:

  1. The input tensor to be unfolded (usually the input image)
  2. The shape to be unfolded (usually be the shape of the kernel, or patch_embd in this case)

So logically say, the type of IM2COL should only depend on the input tensor. patch_embd is only there to provide the shape, so it should not be important which type it is.

Looking at the CUDA kernel, the data of src0 is never used, only its shape is:

const int64_t KH = is_2D ? src0->ne[1] : 1;
const int64_t KW = src0->ne[0];

So I'm wondering if the problem could be due to another reason.

@CISC
Copy link
Collaborator Author

CISC commented Aug 17, 2025

So I'm wondering if the problem could be due to another reason.

It's because dst is promoted to BF16.

@ngxson
Copy link
Collaborator

ngxson commented Aug 17, 2025

Anyway, since it's only a fallback for tensor_force_quant, and several Mmproj models already did this I think the solution I just committed is better.

It's because dst is promoted to BF16.

Hmm ok in this case I think the current approach is safer.

Still a bit strange that im2col's output type is the same as kernel dtype. I expected it to be the same as input dtype. But will be quite risky to change it now, as it will be a breaking change.

@CISC CISC merged commit 4d19698 into master Aug 17, 2025
9 checks passed
@CISC CISC deleted the cisc/fix-convert-mmproj-patch-embd branch August 17, 2025 12:47
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

python python script changes

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants