Repository navigation
Make longrope compileable - #49555
Conversation
`longrope_frequency_update` picks between the short and long scaling
factors based on the sequence length. `seq_len` is derived from
`position_ids` and is therefore always a tensor, so the check is
data-dependent branching and fullgraph compilation fails on the very
first forward pass.
Unlike the other dynamic RoPE types, the choice genuinely depends on the
sequence length and cannot be resolved at initialization. Compute both
sets of frequencies once and select between them with `torch.where`,
which is traceable and keeps the selected values identical to before.
This also fixes two latent bugs in the same block. The cache lookup used
`f"{layer_type}_long_inv_freq"` instead of the prefix, so for a
non-nested RoPE it looked up `"None_long_inv_freq"`, which never exists
and the long factors were recomputed on every forward pass. And
`long_inv_freq` was never moved to `device`, unlike `original_inv_freq`.
`openbmb/MiniCPM3-4B` is blocked earlier: `yarn_apply_mscale` reads
`rope_parameters["factor"]` unconditionally, but `factor` is optional
and a longrope config does not necessarily carry it, so building the
model raises `KeyError`. Move the read inside the branch that needs it.
That helper is shared through `modular`, so the models deriving from it
are regenerated.
Phi3Config copies `original_max_position_embeddings` (default 4096) into `rope_parameters`, so seq_len=16 never exceeded it and the long factors were never exercised.
On CPU, `.to(device)` returns the same nn.Buffer, so setattr goes through Module.__setattr__ and reads `.persistent`, which dynamo cannot trace. The buffer already follows model.to(), so the reassignment is not needed.
|
LGTM but it's a change to the core RoPE code so cc core maintainers before merge! @ArthurZucker @Cyrilvallez @vasqu |
|
cc @zucchini-nlp if you could take a first look here |
| # selecting branchlessly keeps this traceable, `seq_len` is a tensor | ||
| inv_freq = torch.where(seq_len > original_max_position_embeddings, long_inv_freq, original_inv_freq) | ||
| self.register_buffer(f"{prefix}inv_freq", inv_freq, persistent=False) |
There was a problem hiding this comment.
from what i see longrope changes the freq of all positions when seq_len goes beyond config. Doesnt look equivalent with current code if we just keep original freqs as is
There was a problem hiding this comment.
It still does. seq_len is max(position_ids) + 1, a scalar, so torch.where picks the whole long or short set for every position. It is the same if/else as before, just without the Python branch. I ran main and this PR on a tiny Phi3 (short, then long, then short again): inv_freq and cos/sin are bitwise identical, and in the long call even position 1 uses the long frequencies. I also reworded the comment so this is clearer.
Script
import hashlib
import torch
from transformers import Phi3Config
from transformers.models.phi3.modeling_phi3 import Phi3RotaryEmbedding
torch.manual_seed(0)
head_dim, ompe = 16, 8
cfg = Phi3Config(
hidden_size=64,
num_attention_heads=4,
max_position_embeddings=64,
original_max_position_embeddings=ompe,
rope_parameters={
"rope_type": "longrope",
"rope_theta": 10000.0,
"short_factor": [1.0 + 0.1 * i for i in range(head_dim // 2)],
"long_factor": [2.0 + 0.3 * i for i in range(head_dim // 2)],
"original_max_position_embeddings": ompe,
},
)
rope = Phi3RotaryEmbedding(cfg)
x = torch.randn(1, 1, 32, head_dim)
def run(seq_len):
cos, sin = rope(x[:, :, :seq_len], torch.arange(seq_len)[None])
return rope.inv_freq.clone(), cos, sin
def h(t):
return hashlib.md5(t.contiguous().numpy().tobytes()).hexdigest()[:10]
short_freq, cos_s, _ = run(ompe)
long_freq, cos_l, sin_l = run(2 * ompe)
back_freq, _, _ = run(ompe)
print("inv_freq short", h(short_freq), "long", h(long_freq), "back", h(back_freq))
print("cos/sin long ", h(cos_l), h(sin_l))
print("pos1 cos short vs long equal:", torch.equal(cos_s[0, 1], cos_l[0, 1]))
ref = torch.outer(torch.arange(2 * ompe).float(), long_freq)
ref = torch.cat([ref, ref], -1).cos() * rope.attention_scaling
print("long call: all positions use long freqs:", torch.allclose(cos_l[0], ref, atol=1e-6))Same output on main and this PR:
inv_freq short ac8ef83cd5 long 6a1a3cdc3e back ac8ef83cd5
cos/sin long 569afb9d1e 159ce6875c
pos1 cos short vs long equal: False
long call: all positions use long freqs: True
There was a problem hiding this comment.
ah right right, i imaged that was a mask over positions greater/smaller 😄 Thanks for adding hte comment
| def dynamic_frequency_update(self, position_ids, device, layer_type=None): | ||
| """ |
There was a problem hiding this comment.
do you want to try and make dynamic rope compilable? 😄
There was a problem hiding this comment.
Happy to look at it, but I'd rather do it in a separate PR. Dynamic rope keeps state: it grows the cached length and resets it when the input gets short again. So it needs more care than longrope, and I'd like to keep this PR small since it touches core RoPE code. We can discuss the approach there.
There was a problem hiding this comment.
Wait, it might be possible with little changes. Let me try, I will give you feedback once I figured it out.
ArthurZucker
left a comment
There was a problem hiding this comment.
Very nice contribution ty!
I'd just make sure that small inputs are not affected w.o compile (perf wise)? but I'm sure it should be fine!
| scaling_factor = rope_parameters["factor"] | ||
| if mscale_all_dim: | ||
| mscale = yarn_get_mscale(scaling_factor, mscale_all_dim) | ||
| # `factor` is optional for some rope types, only read it when it is needed |
There was a problem hiding this comment.
we don't need the comment everywhere!
There was a problem hiding this comment.
isnt that modular dragging it around 😆 oh yeah, maybe not always
There was a problem hiding this comment.
Removed. It came from modular_deepseek_v2.py, so I dropped it there and regenerated the 13 modeling files.
| inv_freq, self.attention_scaling = rope_init_fn( | ||
| self.config, | ||
| device, | ||
| seq_len=max_seq_len_cached, | ||
| layer_type=layer_type, | ||
| ) |
There was a problem hiding this comment.
only thing is that we recompute freqs each time at reset or smaller seq length, can you confirm the perfs pls
There was a problem hiding this comment.
It does slow down eager: ~155 → ~430 µs per rotary_emb call, +0.1–1.2 ms/token in generate. I reverted the dynamic part (743408a) to keep this PR to longrope, and will look into dynamic separately.
A100, torch 2.14, bf16, eager, two alternating rounds (round 1 / round 2), dynamic RoPE forced with factor=4.0, max_position_embeddings=1024:
| dynamic RoPE | rotary_emb µs/call, main → PR |
generate ms/token, main → PR |
|---|---|---|
| Qwen2.5-0.5B, 32-token prompt | 154 / 158 → 443 / 423 | 16.45 / 16.70 → 17.66 / 16.92 |
| Qwen2.5-0.5B, 1536-token prompt | 153 / 158 → 470 / 420 | 16.58 / 16.95 → 17.66 / 17.06 |
| Llama-3.1-8B, 32-token prompt | 160 / 170 → 431 / 429 | 22.22 / 22.71 → 22.82 / 22.83 |
| Llama-3.1-8B, 1536-token prompt | 159 / 174 → 430 / 429 | 22.60 / 23.48 → 23.77 / 23.59 |
| def test_longrope_compiled_matches_eager(self, seq_len): | ||
| config = self.model_tester.get_config() | ||
| config.original_max_position_embeddings = 8 | ||
| config.max_position_embeddings = 32 | ||
| rotary_dim = config.hidden_size // config.num_attention_heads // 2 | ||
| config.rope_parameters = { | ||
| "rope_type": "longrope", | ||
| "rope_theta": 10000.0, | ||
| "short_factor": [1.0] * rotary_dim, | ||
| "long_factor": [2.0] * rotary_dim, | ||
| "original_max_position_embeddings": 8, | ||
| "factor": 4.0, | ||
| } | ||
| model = Phi3ForCausalLM(config).to(torch_device).eval() | ||
| input_ids = torch.arange(1, seq_len + 1, device=torch_device).unsqueeze(0) | ||
|
|
||
| with torch.no_grad(): | ||
| expected = model(input_ids, use_cache=False).logits |
There was a problem hiding this comment.
dynamic matcthes eager in compile test missing 😄
If you wish, feel free to split dynamic into separate PR - just dont want to block you from getting longrope merged. With dynamic we'll need to check perfs, looks sus from the code to re-compute freq all the time
There was a problem hiding this comment.
It was test_dynamic_rope_compiled_matches_eager in tests/utils/test_modeling_rope_utils.py, now reverted together with the dynamic change.
Not slower in eager: short inputs are on par, long inputs are faster since main recomputed the long freqs on every forward (the cache key bug fixed here). Same setup, Phi-3.5-mini:
Script
import statistics
import sys
import time
import torch
from transformers import AutoConfig, AutoModelForCausalLM
name = sys.argv[1]
cfg = AutoConfig.from_pretrained(name)
if "--dynamic" in sys.argv:
cfg.rope_parameters = {"rope_type": "dynamic", "rope_theta": cfg.rope_parameters["rope_theta"], "factor": 4.0}
cfg.max_position_embeddings = 1024
model = AutoModelForCausalLM.from_pretrained(name, config=cfg, dtype=torch.bfloat16, device_map="cuda").eval()
rope = model.config.rope_parameters
orig = rope.get("original_max_position_embeddings", model.config.max_position_embeddings)
rot = model.model.rotary_emb
x = torch.zeros(1, 1, 8, dtype=torch.bfloat16, device="cuda")
g = torch.Generator().manual_seed(0)
NEW = 128
short_len, long_len = 32, orig + 512
def bench_rotary(pos, n=2000, rounds=7):
pid = torch.tensor([[pos]], device="cuda")
res = []
for _ in range(rounds):
for _ in range(50):
rot(x, pid)
torch.cuda.synchronize()
t = time.perf_counter()
for _ in range(n):
rot(x, pid)
torch.cuda.synchronize()
res.append((time.perf_counter() - t) / n * 1e6)
return statistics.median(res)
def bench_generate(ids, reps):
times = []
for i in range(reps + 2):
torch.cuda.synchronize()
t = time.perf_counter()
model.generate(ids, max_new_tokens=NEW, min_new_tokens=NEW, do_sample=False)
torch.cuda.synchronize()
if i >= 2:
times.append((time.perf_counter() - t) / NEW * 1e3)
return statistics.median(times)
with torch.no_grad():
print(f"rotary_emb, short: {bench_rotary(short_len - 1):.1f} us/call")
print(f"rotary_emb, long : {bench_rotary(long_len - 1):.1f} us/call")
short = torch.randint(100, model.config.vocab_size - 100, (1, short_len), generator=g).cuda()
long = torch.randint(100, model.config.vocab_size - 100, (1, long_len), generator=g).cuda()
print(f"generate, short : {bench_generate(short, 10):.2f} ms/token")
print(f"generate, long : {bench_generate(long, 6):.2f} ms/token") |
zucchini-nlp
left a comment
There was a problem hiding this comment.
Thanks lets merge and do dynamic in follow-up then
|
The docs for this PR live here. All of your documentation changes will be reflected on that endpoint. The docs are available until 30 days after the last update. |
|
The test is failing on main and hopefully fixed soon, I will try to merge tmrw if it's not urgent. Otherwise ping Anton/Arthur to force it |
|
This closes #31339 |
|
[For maintainers] Suggested jobs to run (before merge) run-slow: axk1, axk2, bailing_hybrid, deepseek_v2, deepseek_v3, deepseek_v32, glm4_moe_lite, glm_moe_dsa, hy_v4, longcat_flash, minicpm3, mistral4, phi3, youtu |
CI recapDashboard: View test results in Grafana |
Fixes #31339 (edit from @qgallouedec)
longrope_frequency_updatebranches on a tensor (if seq_len > original_max_position_embeddings). Sofullgraph=Truefails on every longrope model, and the default static-cache auto-compile graph-breaks on every step. This PR picks the long or short frequency set withtorch.whereinstead.It also fixes two longrope bugs:
UnboundLocalErrorwith per-layer-typerope_parameters.yarn_apply_mscalereadrope_parameters["factor"]unconditionally, soopenbmb/MiniCPM3-4B(nofactor) failed to load withKeyError.A phi3 test covers both branches under
fullgraph=Trueand fails on main. Eager logits are bitwise identical to main. No new test failures.A100,
torch==2.14.0+cu130, bf16, 128 new tokens, tok/s before → after:fullgraph=TrueReproduction
Before:
After: both models generate and print
tokens identical to eager: True.Benchmark
python bench.py MODEL_ID {eager,compiled,fullgraph}Logits check
Run before and after, then compare the saved tensors with
torch.equal.