Skip to content

Add Qwen Moe #2163

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

Open
wants to merge 23 commits into
base: master
Choose a base branch
from
Open

Add Qwen Moe #2163

wants to merge 23 commits into from

Conversation

heyyanshuman
Copy link
Collaborator

@heyyanshuman heyyanshuman commented Mar 23, 2025

This PR adds Qwen Mixture-of-expert model to Keras Hub.

Huggingface Reference : link

@heyyanshuman heyyanshuman self-assigned this Mar 29, 2025
@heyyanshuman heyyanshuman marked this pull request as ready for review March 29, 2025 05:06
@mattdangerw mattdangerw removed the request for review from divyashreepathihalli March 31, 2025 16:41
@divyashreepathihalli divyashreepathihalli added the kokoro:force-run Runs Tests on GPU label Mar 31, 2025
@kokoro-team kokoro-team removed the kokoro:force-run Runs Tests on GPU label Mar 31, 2025
Copy link
Collaborator

@divyashreepathihalli divyashreepathihalli left a comment

Choose a reason for hiding this comment

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

just reviewed the MOE part of the code.

@@ -79,7 +79,7 @@ def build(self, decoder_sequence_shape):
self.hidden_dim = decoder_sequence_shape[-1]

# Self attention layer.
self._self_attention_layer = QwenAttention(
self._self_attention_layer = QwenMoeAttention(
Copy link
Collaborator

Choose a reason for hiding this comment

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

I see you have forked this file in qwen_moe folder - why is this being edited?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

looks like this slipped in in find & replace, fixed it.


for expert_idx in range(self.num_experts):
expert_layer = self.experts[expert_idx]
idx, top_x = ops.where(expert_mask[expert_idx])
Copy link
Collaborator

Choose a reason for hiding this comment

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

will the ops.where return different output shape here on different forward passes? - if so that would not work with JAX XLA

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I don't think that this should be the case. Why do you think so?

Copy link
Collaborator

@abheesht17 abheesht17 Apr 2, 2025

Choose a reason for hiding this comment

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

ops.where won't work on JAX at all, if you don't provide x and y.

ops.where calls jnp.where. Check the note here:

Because the size of the output of nonzero is data-dependent, the function is not
compatible with JIT and other transformations. The JAX version adds the optional
size argument which must be specified statically for jnp.nonzero to be used within
JAX’s transformations.

You can, however, consider passing the size argument. That might make it work.

)
expert_mask = ops.transpose(expert_mask, axes=[2, 1, 0])

for expert_idx in range(self.num_experts):
Copy link
Collaborator

@divyashreepathihalli divyashreepathihalli Mar 31, 2025

Choose a reason for hiding this comment

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

The for loop here to go over the experts are inefficient for XLA compilation. This implementation would need to updated - I had tried out a dummy MOE implementation in JAX here - https://colab.sandbox.google.com/drive/1r0rscZK_2bNpDmFLC1POEEQoKcqWQYlQ
in order to bring this to KearsHub we are missing ragged_dot op.

Copy link
Collaborator

Choose a reason for hiding this comment

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

I have prototyped the implementation - will add this op soon

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

When can we expect this to be available as a part of keras op?

@heyyanshuman
Copy link
Collaborator Author

@divyashreepathihalli How should we accomodate aux_loss for CausalLM task here model here?

We are specifying Sparse Categorical CrossEntropy Loss here:

if optimizer == "auto":
optimizer = keras.optimizers.Adam(2e-5)
if loss == "auto":
loss = keras.losses.SparseCategoricalCrossentropy(from_logits=True)
if weighted_metrics == "auto":
weighted_metrics = [keras.metrics.SparseCategoricalAccuracy()]
super().compile(
optimizer=optimizer,
loss=loss,
weighted_metrics=weighted_metrics,
**kwargs,

Copy link
Collaborator

@divyashreepathihalli divyashreepathihalli left a comment

Choose a reason for hiding this comment

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

Thanks for the updates @heyyanshuman!
I left some comments on the PR regarding tf ops
please add tests for the layers, backbones and tasks
I am curious to know if model.fit works, do you have a demo colab for inference and FT? - looking for the aux loss implementation

)
self._query_dense.build(inputs_shape)

self._key_dense = keras.layers.EinsumDense(
Copy link
Collaborator

@divyashreepathihalli divyashreepathihalli Apr 10, 2025

Choose a reason for hiding this comment

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

you might want to rename this to to match other KH models here - value_dense and query_dense
this will allow enabling LoRA on this Model -

return ["query_dense", "value_dense", "query", "value"]

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I don't have access to this document :(

Copy link
Collaborator

Choose a reason for hiding this comment

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

sorry! wrong copy pasta error! updated the link -

return ["query_dense", "value_dense", "query", "value"]

@keras_hub_export(
"keras_hub.models.QwenMoeCausalLM",
)
class QwenMoeCausalLM(CausalLM):
Copy link
Collaborator

Choose a reason for hiding this comment

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

add docstring and example to show model.fit and generate

@divyashreepathihalli divyashreepathihalli added the kokoro:force-run Runs Tests on GPU label Apr 14, 2025
@kokoro-team kokoro-team removed the kokoro:force-run Runs Tests on GPU label Apr 14, 2025
Copy link
Collaborator

@divyashreepathihalli divyashreepathihalli left a comment

Choose a reason for hiding this comment

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

Thanks @heyyanshuman - can you add a demo colab for inference and fit? and also aprovide colab/screenshot for numerics verification?

)
self._query_dense.build(inputs_shape)

self._key_dense = keras.layers.EinsumDense(
Copy link
Collaborator

Choose a reason for hiding this comment

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

sorry! wrong copy pasta error! updated the link -

return ["query_dense", "value_dense", "query", "value"]

return False
if running_on_gpu():
# GPU never supports softcap in the fused op.
if self.logit_soft_cap is not None:
Copy link
Collaborator

Choose a reason for hiding this comment

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

does Qwen MOE use logit_soft_cap?

@heyyanshuman
Copy link
Collaborator Author

Output matching ss:

image

@divyashreepathihalli divyashreepathihalli added the kokoro:force-run Runs Tests on GPU label Apr 15, 2025
@kokoro-team kokoro-team removed the kokoro:force-run Runs Tests on GPU label Apr 15, 2025
gate_output = self._feedforward_gate_dense(x)

# Note that we run the activation function in full 32-bit
# precision since this is what `torch.nn.functional.silu`
Copy link
Collaborator

Choose a reason for hiding this comment

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

is this torch specific? Or did you mean this is what the original implementation is doing?



class QwenMoeExperts(keras.layers.Layer):
"""Batched feed‑forward experts à‑la Llama‑4 (pure keras.ops)."""
Copy link
Collaborator

@divyashreepathihalli divyashreepathihalli Apr 15, 2025

Choose a reason for hiding this comment

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

NIT : a-la - lets keep this simple english - inspired by Llama 4 or whatever



class QwenSparseMoeBlock(keras.layers.Layer):
"""Qwen‑2 sparse block rewritten in Llama‑4 batched style."""
Copy link
Collaborator

Choose a reason for hiding this comment

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

NIT: rewritten - inspired by Llama 4 implementation

shared_expert_intermediate_dim,
num_experts,
top_k,
norm_topk_prob,
Copy link
Collaborator

Choose a reason for hiding this comment

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

NIT: topk->top_k

norm_topk_prob,
kernel_initializer="glorot_uniform",
layer_norm_epsilon=1e-5,
router_aux_loss_coef=0.01,
Copy link
Collaborator

Choose a reason for hiding this comment

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

expand coef-> coefficient

self.intermediate_dim_shared = shared_expert_intermediate_dim
self.num_experts = num_experts
self.top_k = top_k
self.norm_topk_prob = norm_topk_prob
Copy link
Collaborator

Choose a reason for hiding this comment

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

topk->top_k

shared_expert_intermediate_dim,
num_experts,
top_k,
norm_topk_prob,
Copy link
Collaborator

Choose a reason for hiding this comment

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

topk->top_k - here and everywhere

self.intermediate_dim = intermediate_dim
self.num_query_heads = num_query_heads
self.num_key_value_heads = num_key_value_heads

Copy link
Collaborator

Choose a reason for hiding this comment

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

remove unnecessary new lines



class QwenMoeBackboneTest(TestCase):
def setUp(self):
Copy link
Collaborator

Choose a reason for hiding this comment

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

add a flash attention mock test like -

def test_flash_attention_call(self):

return out, router_logits


class QwenMoeTransformerDecoder(keras.layers.Layer):
Copy link
Collaborator

Choose a reason for hiding this comment

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

add a self.run_layer_test for the decoder

return x


class QwenMoeExperts(keras.layers.Layer):
Copy link
Collaborator

Choose a reason for hiding this comment

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

also add a self.run_layer_test for this layer as well

Copy link
Collaborator

@divyashreepathihalli divyashreepathihalli left a comment

Choose a reason for hiding this comment

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

Left a few more NIT comments!! Looking great overall!
Once we have the comments addressed and inference and fit demo - it is ready for merge.

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.

4 participants