-
Notifications
You must be signed in to change notification settings - Fork 418
feat(trainer): push post-meta buffer reinit down to owning layers #3378
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
base: main
Are you sure you want to change the base?
Changes from 4 commits
b9ae380
52ecfab
df5ec2f
22054c8
d5b1d4d
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 |
|---|---|---|
|
|
@@ -82,6 +82,20 @@ def forward( | |
| sin = emb.sin() * attention_scaling | ||
| return cos.to(dtype=x.dtype), sin.to(dtype=x.dtype) | ||
|
|
||
| def init_buffers_post_meta(self) -> None: | ||
|
Collaborator
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. I'm honestly not a big fan of having this model specific code here - would try to think of a way to make this owned by the rope layer itself as well
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. @S1ro1 this is a method on
Collaborator
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. Oh i see, i thought it's the parent model, then all good |
||
| for layer_type in self.layer_types: | ||
| rope_init_fn = self.compute_default_rope_parameters | ||
| if self.rope_type[layer_type] != "default": | ||
| rope_init_fn = ROPE_INIT_FUNCTIONS[self.rope_type[layer_type]] | ||
| inv_freq, attention_scaling = rope_init_fn( | ||
| self.config, | ||
| getattr(self, f"{layer_type}_inv_freq").device, | ||
| layer_type=layer_type, | ||
| ) | ||
| getattr(self, f"{layer_type}_inv_freq").copy_(inv_freq) | ||
| getattr(self, f"{layer_type}_original_inv_freq").copy_(inv_freq) | ||
| setattr(self, f"{layer_type}_attention_scaling", attention_scaling) | ||
|
|
||
|
|
||
| def _laguna_attention_config(config: LagunaConfig, num_heads: int) -> AttentionConfig: | ||
| return AttentionConfig( | ||
|
|
@@ -384,27 +398,6 @@ def forward( | |
| temperature=temperature, | ||
| ) | ||
|
|
||
| def init_buffers_post_meta(self) -> None: | ||
| rotary_emb = self.model.rotary_emb | ||
| for layer_type in rotary_emb.layer_types: | ||
| rope_init_fn = rotary_emb.compute_default_rope_parameters | ||
| if rotary_emb.rope_type[layer_type] != "default": | ||
| rope_init_fn = ROPE_INIT_FUNCTIONS[rotary_emb.rope_type[layer_type]] | ||
| inv_freq, attention_scaling = rope_init_fn( | ||
| rotary_emb.config, | ||
| getattr(rotary_emb, f"{layer_type}_inv_freq").device, | ||
| layer_type=layer_type, | ||
| ) | ||
| getattr(rotary_emb, f"{layer_type}_inv_freq").copy_(inv_freq) | ||
| getattr(rotary_emb, f"{layer_type}_original_inv_freq").copy_(inv_freq) | ||
| setattr(rotary_emb, f"{layer_type}_attention_scaling", attention_scaling) | ||
|
|
||
| for module in self.modules(): | ||
| if isinstance(module, MoE) and module.tokens_per_expert.device.type != "meta": | ||
| module.tokens_per_expert.zero_() | ||
| if module.expert_bias is not None: | ||
| module.expert_bias.zero_() | ||
|
|
||
|
|
||
| __all__ = [ | ||
| "LagunaForCausalLM", | ||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -12,7 +12,7 @@ | |
| from transformers.models.qwen3_5.modeling_qwen3_5 import ( | ||
| Qwen3_5PreTrainedModel as HFQwen3_5PreTrainedModel, | ||
| ) | ||
| from transformers.models.qwen3_5.modeling_qwen3_5 import Qwen3_5VisionModel | ||
| from transformers.models.qwen3_5.modeling_qwen3_5 import Qwen3_5VisionModel, Qwen3_5VisionRotaryEmbedding | ||
| from transformers.processing_utils import Unpack | ||
| from transformers.utils import TransformersKwargs | ||
|
|
||
|
|
@@ -32,6 +32,19 @@ | |
| from prime_rl.utils.sequence import get_cu_seqlens_from_seq_lens | ||
|
|
||
|
|
||
| def _init_vision_rope_buffers_post_meta(self: Qwen3_5VisionRotaryEmbedding) -> None: | ||
| inv_freq = 1.0 / ( | ||
| self.theta ** (torch.arange(0, self.dim, 2, dtype=torch.float32, device=self.inv_freq.device) / self.dim) | ||
| ) | ||
| self.inv_freq.copy_(inv_freq) | ||
|
|
||
|
|
||
| # Qwen3_5VisionRotaryEmbedding is upstream transformers code; Qwen3_5VisionModel.__init__ | ||
| # hardcodes its construction, so we can't subclass-and-inject like elsewhere. Attach the | ||
| # buffer-init hook to the class directly instead. | ||
| Qwen3_5VisionRotaryEmbedding.init_buffers_post_meta = _init_vision_rope_buffers_post_meta | ||
|
Collaborator
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. We can probably vendor the rotary embeddings ourselves, would do it anyway given the aim to remove transformers so we can do it now instead of this ugly patch
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. Agreed, but didn't want to mix concerns here |
||
|
|
||
|
|
||
| class Qwen3_5GatedFlashAttention(Qwen3_5MoeGatedFlashAttention): | ||
| pass | ||
|
|
||
|
|
@@ -452,26 +465,6 @@ def forward( | |
| temperature=temperature, | ||
| ) | ||
|
|
||
| def init_buffers_post_meta(self): | ||
| if self._is_vlm: | ||
| lm_rope = self.model.language_model.rotary_emb | ||
| else: | ||
| lm_rope = self.model.rotary_emb | ||
|
|
||
| if hasattr(lm_rope, "rope_init_fn"): | ||
| inv_freq, lm_rope.attention_scaling = lm_rope.rope_init_fn(lm_rope.config, lm_rope.inv_freq.device) | ||
| lm_rope.inv_freq.copy_(inv_freq) | ||
|
|
||
| if self._is_vlm: | ||
| vis_rope = self.model.visual.rotary_pos_emb | ||
| if hasattr(vis_rope, "inv_freq"): | ||
| dim = vis_rope.inv_freq.shape[0] | ||
| inv_freq = 1.0 / ( | ||
| 10000.0 | ||
| ** (torch.arange(0, dim * 2, 2, dtype=torch.float32, device=vis_rope.inv_freq.device) / (dim * 2)) | ||
| ) | ||
| vis_rope.inv_freq.copy_(inv_freq) | ||
|
|
||
|
|
||
| __all__ = [ | ||
| "Qwen3_5ForCausalLM", | ||
|
|
||
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.
let's make this PostMetaBufferInitModule, I hate this smell of ai code where it does private classes
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.
Lol, I'll change it, but pretty sure I asked for private classes and fns in these spots since they're not things I'd expect users to need to use or know about
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.
makes sense, just my aversion, can keep if you think so
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.
already changed, no strong feelings either way