Repository navigation
Make longrope compileable #49555
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
Make longrope compileable #49555
Changes from all commits
dcda292
ed7aa84
cfa265b
aa04f1c
ce640f6
f431d9b
4221422
0ed7ffb
5cddc50
743408a
7a24437
7164fed
b4bcd58
97ba00b
f43a0b7
f9eb338
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -132,6 +132,28 @@ def test_longrope_generation_crossing_boundary(self, sliding_window): | |
| expected = model(prefix, use_cache=False).logits[:, -1] | ||
| torch.testing.assert_close(logits, expected, rtol=1e-4, atol=1e-4) | ||
|
|
||
| @parameterized.expand([(8,), (16,)]) | ||
| 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 | ||
|
Comment on lines
+136
to
+153
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 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
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. It was |
||
| compiled = torch.compile(model, fullgraph=True)(input_ids, use_cache=False).logits | ||
| torch.testing.assert_close(compiled, expected, rtol=1e-4, atol=1e-4) | ||
|
|
||
|
|
||
| @slow | ||
| @require_torch | ||
|
|
||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
do you want to try and make dynamic rope compilable? 😄
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Wait, it might be possible with little changes. Let me try, I will give you feedback once I figured it out.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done in 0ed7ffb