Skip to content

refactor olmoe#101

Merged
Jintao-Huang merged 2 commits into
modelscope:mainfrom
Jintao-Huang:refactor_olmoe
May 29, 2026
Merged

refactor olmoe#101
Jintao-Huang merged 2 commits into
modelscope:mainfrom
Jintao-Huang:refactor_olmoe

Conversation

@Jintao-Huang

Copy link
Copy Markdown
Collaborator

No description provided.

@gemini-code-assist gemini-code-assist Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Code Review

This pull request refactors the OLMoE model implementation, simplifying OLMoESelfAttention, OLMoEBridge, and OLMoELoader by inheriting from and leveraging base classes. It updates the QK layernorm logic to apply RMSNorm over the full Q/K channels and correctly handle tensor parallel (TP) replication. The review feedback suggests adding defensive None checks for self.q_norm and self.k_norm when configuring their parameters and applying them to the query/key tensors to prevent potential runtime errors if QK layernorm is disabled.

Comment on lines +51 to +54
for norm in (self.q_norm, self.k_norm):
for p in norm.parameters():
p.sequence_parallel = False
p.average_gradients_across_tp_domain = True

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

medium

If submodules.q_layernorm or submodules.k_layernorm is None (for example, if QK layernorm is disabled or not configured in a custom setup), self.q_norm or self.k_norm will be initialized as None. Accessing .parameters() on a None object will raise an AttributeError. Adding a defensive check to ensure the norm is not None before iterating over its parameters prevents potential runtime crashes.

Suggested change
for norm in (self.q_norm, self.k_norm):
for p in norm.parameters():
p.sequence_parallel = False
p.average_gradients_across_tp_domain = True
for norm in (self.q_norm, self.k_norm):
if norm is not None:
for p in norm.parameters():
p.sequence_parallel = False
p.average_gradients_across_tp_domain = True

Comment on lines +64 to +65
query = self.q_norm(query)
key = self.k_norm(key)

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

medium

Similarly, if self.q_norm or self.k_norm is None, calling them directly will raise a TypeError. Adding defensive None checks before applying the norms ensures robustness.

Suggested change
query = self.q_norm(query)
key = self.k_norm(key)
if self.q_norm is not None:
query = self.q_norm(query)
if self.k_norm is not None:
key = self.k_norm(key)

@Jintao-Huang
Jintao-Huang merged commit 18ffcc1 into modelscope:main May 29, 2026
1 check passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants