models: fix GDN normalization - #156
Conversation
Codex Review SummaryThis comment shows the latest Codex review activity on this pull request.
ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
Codex reacts with 👀 while any review is running, comments if it has suggestions, and reacts with 👍 once all reviews finish with no findings. |
63defbe to
f011d80
Compare
![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: f011d805c7
ℹ️ 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".
| // flash-linear-attention's l2norm, x * rsqrt(sum(x*x) + eps), with eps INSIDE the root. | ||
| // ggml_l2_norm is x / max(sqrt(sum(x*x)), eps), i.e. torch.nn.functional.normalize: right for | ||
| // rwkv7-base, which passes normalize's own 1e-12, wrong here. transformers hit the same thing | ||
| // and fixed it in huggingface/transformers#40842. | ||
| // rms_norm already carries eps inside the root, so this needs no new ggml op. |
There was a problem hiding this comment.
Condense this five-line, mid-sentence-wrapped comment to the essential one or two lines. The repository explicitly requires concise code comments and forbids hard-wrapping them, so this block adds avoidable maintenance noise to a globally included header.
AGENTS.md reference: AGENTS.md:L74-L77
Useful? React with 👍 / 👎.
f011d80 to
83a424d
Compare
danielhanchen
changed the title
danielhanchen
changed the title
The GDN q/k normalization is defined by flash-linear-attention as
l2norm(x) = x * rsqrt(sum(x*x) + eps)
with eps inside the root. Every GDN call site in the tree uses ggml_l2_norm
instead, which is x / max(sqrt(sum(x*x)), eps), i.e.
torch.nn.functional.normalize - its CUDA kernel cites that page.
The clamp never engages at these magnitudes, so in practice llama.cpp
normalizes with no epsilon at all where the reference has one inside the
root.
transformers made the same substitution when it first added Qwen3-Next and
corrected it three days later in huggingface/transformers#40842, 'Fix the
misalignment between the l2norm in GDN of Qwen3-Next and the implementation
in the FLA library'. vLLM and SGLang vendor FLA rather than reimplementing
it, so neither ever had the clamp.
eps keeps coming from the checkpoint, exactly as every call site already
passed it. The references hardcode 1e-6 for this norm; that is a separate
question and the two agree on every GDN checkpoint in the wild.
ggml_l2_norm itself is correct and unchanged, as is rwkv7-base, its original
caller, which passes normalize's own default eps of 1e-12.
No new ggml op: rms_norm already carries eps inside the root, so
rms_norm(x, eps/n) * (1/sqrt(n)) is exactly x * rsqrt(sum(x*x) + eps).
83a424d to
c03dc68
Compare
1 participant
Fixes GDN q/k normalization to use
x * rsqrt(sum(x^2) + eps)instead ofx / max(sqrt(sum(x^2)), eps), matching the official Qwen FlashQLA repo, FLA, Transformers, vLLM, and SGLang.Affects qwen35, qwen35moe, qwen3next, qwen4exp, kimi-linear, kimi-k3, and bailingmoe3.
ggml_l2_normand RWKV7 are unchanged.