Skip to content

Guard against all weights in a super-block being zero #3010

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
merged 2 commits into from
Sep 5, 2023

Conversation

ikawrakow
Copy link
Contributor

@cebtenzzre

Does this resolve the problem in #2982.

In order to get the assertion observed in #2982 all weights in a block of 256 must be zero. I see that I have a guard against this for all k_quants except Q6_K

@ikawrakow
Copy link
Contributor Author

ikawrakow commented Sep 4, 2023

Looking deeper into the assert observed in #2982, this will most likely not fix it. The nearest_int() function is called with NaN, while this change guards against all weights in a block of 256 being zero. If that would happen, than nearest_int() would be invoked with Inf, not NaN. The way the code is written, I can only see two possibilities to get a NaN while quantizing to Q6_K:

  • There are already NaN's present in the fp16 model being quantized
  • The weights are not all zero, bur are extremely small (std::numeric_limits<float>::min() or close to that). Small enough, so that, when computing 63 / max on this line in make_qx_quants(), the result overflows and iscale becomes -Inf. The check in nearest_int() does not trigger when the argument is -Inf and we get a non-zero integer value back (-4194304) With this, sumlx and suml2 computed further below both become +/-Inf, and we get a scale that is NaN via scale = sumlx/suml2 = Inf/Inf = NaN.

If this analysis is correct, the "bug" observed in #2982 is more a model problem rather than anything else. But just in case, I now added a check against extremely small model weights.

@cebtenzzre
Copy link
Collaborator

cebtenzzre commented Sep 5, 2023

I'm doing a little more investigation with gdb and rr right now. I haven't tried any of the potential fixes yet.

  • all x values in this block are zero:
>>> print *(float (*)[256])x
$34 = {[0] = 0 <repeats 256 times>}
  • each call to make_qx_quants returns at line 90 (!amax case)
  • make_qx_quants returns scale=0 for each of the 16 sub-blocks
  • iscale = -128.f/max_scale and max_scale is zero
  • when nearest_int(iscale*scales[ib]) is called, iscale is -inf and scales[ib] is 0, and -inf * 0 = NaN.

So, maybe the first commit from this PR is the right fix.

edit: 79f1757 is sufficient. 2d5f5d7 doesn't seem to hurt.

@ikawrakow ikawrakow requested review from howard0su and ggerganov and removed request for howard0su September 5, 2023 06:22
@ikawrakow ikawrakow merged commit d59bd97 into master Sep 5, 2023
@ikawrakow ikawrakow deleted the ik/issue_2982 branch September 5, 2023 07:55
Comment on lines +1091 to +1092
y[i].d = ggml_fp32_to_fp16(0.f);
continue;
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@ikawrakow

Don't we need to increase x here?

            y[i].d = ggml_fp32_to_fp16(0.f);
            x += QK_K;
            continue;

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes, we do. Great catch!

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You will fix it?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yup, will push a fix in minute directly to master

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants