Repository navigation
Pull 9 high-value upstream candle PRs into the fork (Tier 1) - #37
Merged
Merged
Conversation
…e#3617) A crafted `.pth` could drive the pickle reader's value stack/memo to multi-GiB heap (an N-flood, or BINGET replay of a memoised subtree; CWE-1325) or build an arbitrarily deep value whose recursive Drop overflows the stack (a Q-chain; CWE-674) -- both on the normal load path (read_pth_tensor_info -> Stack::read_loop) before any tensor data is touched. This is the pickle sibling of the GGUF DoS fixed in huggingface#3556/huggingface#3585. Add three always-on bounds, charged O(1) per opcode via a parallel depth/size metadata stack (a per-push deep walk would itself be an O(n^2) CPU-DoS): - PICKLE_MAX_WORKING_SET (512 MiB): every push and memo-clone deep size is charged to a cumulative floor; memo clones are charged before the clone, so an over-budget replay is rejected without allocating. - PICKLE_MAX_DEPTH (64, matching GGUF_MAX_VALUE_DEPTH): construction nesting depth is capped, so an over-deep value never forms and recursive Drop stays shallow. - PICKLE_MAX_PAYLOAD (64 MiB): a BINUNICODE length is checked before the backing buffer is allocated. Also reject LONG1 values wider than 8 bytes, fixing a pre-existing arithmetic-overflow panic (`<< (i * 8)` for n_bytes >= 9; CWE-190) that fuzzing this change surfaced; anamnesis's reference parser already handles LONG1 this way. Availability-only: reduce never invokes the callable. No public API change; legitimate state_dict files are unaffected. Five unit tests cover the four vectors plus a positive parse. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
`RotaryEmbedding::new` cast `inv_freq` and the `t` position vector
to `dtype` (typically bf16) *before* the matmul, then computed
`.sin()` / `.cos()` on the bf16 result. bf16 has a 7-bit mantissa
and cannot represent integer positions above ~256 exactly; e.g.
position 15962 rounds to neighbouring bf16 values 15936 or 15968
(16-step granularity at this magnitude). For low-frequency dims
where `inv_freq[0] = 1.0`, the angle then becomes the rounded
position, and `cos(15962) ≈ -0.547` vs `cos(15968) ≈ -0.882` —
neighbouring representable bf16 inputs produce essentially
uncorrelated cosines.
The result: at long-context positions the cos/sin tables become
noise rather than trigonometric values, and any model using this
RoPE diverges from `transformers`' reference output once positions
exceed ~10k tokens. The error is hidden by mean-pool + L2-norm
for short sequences (per-position errors average toward zero) but
surfaces at long context where the per-position error pattern
systematically bends the pooled vector.
The HuggingFace Python reference correctly forces fp32 for the
angle computation:
with torch.autocast(device_type=device_type, enabled=False):
freqs = (inv_freq_expanded.float() @ position_ids_expanded.float()).transpose(2, 3)
emb = torch.cat((freqs, freqs), dim=-1)
cos = emb.cos() * self.attention_scaling
sin = emb.sin() * self.attention_scaling
return cos.to(dtype=x.dtype), sin.to(dtype=x.dtype)
candle's own `qwen3_vl/text.rs::RotaryEmbedding::new` also keeps
inv_freq/t in fp32 and casts only the final `.cos()` / `.sin()`
outputs to dtype. This change applies the same pattern to qwen2.
The fix is minimal (drop the two `.to_dtype(dtype)?` calls before
the matmul; add them after the trig).
## Empirical evidence
Discovered via a six-step layer-by-layer bisection on Jina V4
(Qwen2.5-VL backbone via `trust_remote_code=True` vendored copy of
qwen2 logic, but the bug is in candle-transformers' qwen2.rs that
many downstream models build on). Using a 1000-sample stratified
cosine harness against `transformers` 4.x + `flash_attn 2.8.3`,
on cuda:0 bf16:
| Stratum | n | P95 before fix | P95 after fix |
|---|---|---:|---:|
| retrieval.query | 200 | 0.9996 | 0.9996 |
| retrieval.passage | 200 | 0.9970 | 0.9997 |
| text-matching | 200 | 0.9995 | 0.9995 |
| code | 200 | 0.9959 | 0.9990 |
| matryoshka | 100 | 0.9995 | 0.9996 |
| **long-context** (~16k tokens) | 100 | **0.7705** | **0.9999** |
Long-context min cosine: 0.7032 → 0.9994. Per-position max-abs
diff at post-RoPE Q (15962-token sample): 38.0 → 0.25.
## Other models with the same pattern
A grep of `candle-transformers/src/models/` for the
`.to_dtype(dtype)?` pre-matmul pattern returns a number of similar
RotaryEmbedding implementations (gemma.rs, olmo.rs, olmo2.rs,
chatglm.rs, phi3.rs, mixtral.rs, falcon.rs, recurrent_gemma.rs,
glm4.rs, …). I'm scoping this PR to qwen2.rs since that's the
model I have direct empirical evidence for, but maintainers may
want a coordinated follow-up fixing the same pattern across the
qwen-family adjacents.
## Note on M-RoPE
Qwen2.5-VL uses M-RoPE (multimodal rotary), not standard RoPE.
The interaction matters here only for text-only inputs: M-RoPE's
section split + per-grid position handling degenerates to standard
RoPE when all three position grids are identical. So the bug
above (which affects standard RoPE in candle's qwen2.rs) propagates
to any text-only Qwen2.5-VL forward that builds on this code path,
even though the immediate model isn't qwen2 itself.
`Config` for the Yi models declares `rope_theta: f64` and both `config_6b()` and `config_34b()` set it to `5_000_000.`, which is the RoPE base Yi was trained with. But `RotaryEmbedding::new` hardcodes `10000f32` when building `inv_freq`, so `cfg.rope_theta` is never read and the rotary frequencies are computed with the wrong base for every Yi inference. Every other model that carries a `rope_theta` config threads it into the same `inv_freq` loop (see mistral.rs, mixtral.rs, olmo.rs, stable_lm.rs, starcoder2.rs). Use `cfg.rope_theta` here too, matching those siblings.
disentangled_attention_bias initialized its score accumulator as F32 and XSoftmax::apply built its fill tensors as F32 regardless of the input dtype, so forward failed with a dtype mismatch for F16 models. Cast the scalars to the working dtype; the F32 path is unchanged. Signed-off-by: tobocop2 <5562156+tobocop2@users.noreply.github.com>
Phi-3-mini long-prompt degradation had two independent causes: 1. RoPE phase table was built in the model dtype (BF16/F16), which quantizes positions and phases on long axes. Compute in F32 and cast sin/cos afterward, matching mistral/llama. Apply the same RoPE fix to qwen2, gemma, and gemma2. 2. config.json sliding_window was ignored. Parse the field, apply it in the causal mask (mask key j when q_pos - j >= window), and keep a mask on single-token decode once KV length exceeds the window. Adds unit tests for Config serde, RoPE dtype, and window mask rules. Fixes huggingface#3698
- Exercise real prepare_decoder_attention_mask (prefill + decode offset) - Build tiny zero-weight model and run prefill/decode forward - Assert RoPE BF16 path matches F32 reference at high positions - Config serde: sliding_window present and absent - Collapse identical if-branches for clippy::if_same_then_else
…tized_qwen3_moe Both model types kept their per-layer KV cache fields private with no accessor beyond clear_kv_cache() (reset-only) -- neither exposed any way to extract the current cache state or restore a previously-saved one, so a caller couldn't persist and later resume attention state without reprocessing every token from scratch. Both new methods follow the same shape and pairing convention (Vec<Option<(Tensor, Tensor)>>, one entry per layer, errors rather than silently partial-restoring on a layer-count mismatch) so callers don't need per-architecture branching to handle either backend's state.
…antized_glm4 The fork's quantized_glm4 LayerWeights nests kv_cache under self_attn (AttentionWeights), unlike upstream where it sits directly on the layer. Route clear_kv_cache through layer.self_attn.kv_cache accordingly. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
The fork's toolchain (clippy 1.95) flags repeat().take() as manual_repeat_n; rewrite the three test-only occurrences with std::iter::repeat_n. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
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
9 participants
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.
Cherry-picks 9 high-value open PRs from
huggingface/candleinto the fork ahead of upstream review, prioritised by the Tachyon serving roadmap (loader security, correctness on served models, KV management, OpenAI-parity sampling). Original commits and authorship are preserved.Included (upstream PR → author)
Loader security
Correctness on served models
gelu_pytorch_tanh, not SiLU)cfg.rope_thetasliding_windowGeneration
no_repeat_ngramlogits utilityKV cache management
clear_kv_cachefor quantized gemma3/phi3/glm4/lfm2 (+ qwen3moe)Fork adaptations (2 extra commits)
clear_kv_cachethroughlayer.self_attn.kv_cacheinquantized_glm4(the fork nests the cache underself_attn, unlike upstream).repeat().take()in Bound the pickle VM's working set and nesting depth (fixes #3617) huggingface/candle#3628's tests asrepeat_n(fork toolchain is Rust 1.95, clippy stricter than the PR was written against).CI gate (candle's own, all green)
cargo fmt --all -- --checkcargo check --workspacecargo clippy --workspace --tests --examples --benches -- -D warningsDeliberately excluded
memotypes); Bound the pickle VM's working set and nesting depth (fixes #3617) huggingface/candle#3628 is broader and already bounds the memo-replay amplification DoS.Not addressed here
The Qwen3 "causal mask for batch>1" fix (competing PRs huggingface#3743/huggingface#3484/…) is left for a separate, deliberate single-PR pick.