Skip to content

Conversation

Bond099
Copy link
Contributor

@Bond099 Bond099 commented Mar 18, 2025

This PR adds the StableLM-3B 4E1T model to Keras Hub. However, numerical matching with the Hugging Face implementation is still in progress.

@Bond099
Copy link
Contributor Author

Bond099 commented Mar 22, 2025

@divyashreepathihalli Here is a comparison of numerics with Hugging Face in Colab. The results match with an absolute tolerance of 1e-3, but they do not match when using 1e-5. Could you please take a look and suggest some improvements or explanations for this discrepancy?

@divyashreepathihalli divyashreepathihalli added the kokoro:force-run Runs Tests on GPU label Mar 24, 2025
@kokoro-team kokoro-team removed the kokoro:force-run Runs Tests on GPU label Mar 24, 2025
@sachinprasadhs sachinprasadhs added the WIP Pull requests which are work in progress and not ready yet for review. label Apr 11, 2025
@divyashreepathihalli
Copy link
Collaborator

The numerics is good enough!

@Bond099 Bond099 marked this pull request as ready for review May 15, 2025 09:58
Copy link
Collaborator

@abheesht17 abheesht17 left a comment

Choose a reason for hiding this comment

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

Looks good overall, but let's check the numerics and the generate output.

@mattdangerw
Copy link
Member

@Bond099 let's sync this with the latest changes and make sure to run our format script. I'm not exactly sure why non of our CI is running, but I don't think it ran.

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

abheesht17 commented Jun 16, 2025

Let's clean up the PR. Can we fix the following minor things?

  • Pull in master.
  • I still don't see tie_weights = False in the PR.
  • Run formatting, etc.
  • Let's wait for all the tests to run.

@abheesht17
Copy link
Collaborator

Looks like there are conflicts. Please pull in master and resolve conflicts

@abheesht17 abheesht17 added the kokoro:force-run Runs Tests on GPU label Jul 3, 2025
@kokoro-team kokoro-team removed the kokoro:force-run Runs Tests on GPU label Jul 3, 2025
@@ -3,7 +3,6 @@
from keras import random
Copy link
Collaborator

Choose a reason for hiding this comment

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

I'd actually want sampler.py to be untouched, because all models use this. We'd want to make this change only for Stable LM. The changes will be less intrusive that way.

Is there a way you can pass the correct mask right here:

, https://github.com/keras-team/keras-hub/blob/master/keras_hub/src/samplers/sampler.py#L79?

If you do pass the correct mask, this should evaluate to the correct bool: https://github.com/keras-team/keras-hub/blob/master/keras_hub/src/samplers/sampler.py#L100.

Take a look if this works.

@divyashreepathihalli
Copy link
Collaborator

/gemini review

Copy link

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

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 adds support for the StableLM-3B-4E1T model. I've identified a critical bug in the StableLMTransformerDecoder's caching logic that would cause failures during training, and a few medium-to-high severity issues in the StableLMBackbone's documentation and the checkpoint conversion script.

Comment on lines +163 to +185
x, self_attention_cache = self.self_attention_layer(
hidden_states=x,
attention_mask=self_attention_mask,
cache=self_attention_cache,
cache_update_index=self_attention_cache_update_index,
)
x = self.self_attention_dropout(x, training=training)
x = x + residual

residual = x

# Feedforward block
x = self.feedforward_layernorm(x)
gate_output = self.feedforward_gate_dense(x)
gate_output = self.activation(gate_output)
intermediate_output = self.feedforward_intermediate_dense(x)
x = self.feedforward_output_dense(
ops.multiply(intermediate_output, gate_output)
)
decoder_output = x + residual
if self_attention_cache is not None:
return decoder_output, self_attention_cache
return decoder_output

Choose a reason for hiding this comment

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

critical

The call method's handling of the attention cache has a bug. When call is invoked without a cache (i.e., self_attention_cache is initially None), self.self_attention_layer returns a tuple of (attention_output, attention_output). This makes the local self_attention_cache a tensor, causing the if condition to be true and the method to incorrectly return a tuple, breaking the StableLMBackbone functional model which expects a single tensor output. Use a different variable name for the cache returned by the attention layer to fix this.

        attention_output, next_attention_cache = self.self_attention_layer(
            hidden_states=x,
            attention_mask=self_attention_mask,
            cache=self_attention_cache,
            cache_update_index=self_attention_cache_update_index,
        )
        x = self.self_attention_dropout(attention_output, training=training)
        x = x + residual

        residual = x

        # Feedforward block
        x = self.feedforward_layernorm(x)
        gate_output = self.feedforward_gate_dense(x)
        gate_output = self.activation(gate_output)
        intermediate_output = self.feedforward_intermediate_dense(x)
        x = self.feedforward_output_dense(
            ops.multiply(intermediate_output, gate_output)
        )
        decoder_output = x + residual

        if self_attention_cache is not None:
            return decoder_output, next_attention_cache
        return decoder_output

cfg["max_sequence_length"] = pt_cfg["max_position_embeddings"]
cfg["layer_norm_epsilon"] = pt_cfg["layer_norm_eps"]
cfg["rope_max_wavelength"] = pt_cfg["rope_theta"]
cfg["partial_rotary_factor"] = pt_cfg["partial_rotary_factor"]

Choose a reason for hiding this comment

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

high

The Hugging Face config parameter partial_rotary_factor should be mapped to rotary_percentage to ensure the Keras model is configured correctly.

Suggested change
cfg["partial_rotary_factor"] = pt_cfg["partial_rotary_factor"]
cfg["rotary_percentage"] = pt_cfg["partial_rotary_factor"]

Comment on lines +45 to +46
tie_word_embeddings: bool, optional. Whether to tie input and output
embeddings. Defaults to False.

Choose a reason for hiding this comment

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

medium

The docstring for __init__ includes the tie_word_embeddings argument, but it is not present in the method's signature. Either add the argument to the __init__ method, or remove it from the docstring.

Comment on lines +130 to +132
keras_model.get_layer(
f"transformer_layer_{layer_index}"
)._self_attention_layer._query_dense.set_weights(weights)

Choose a reason for hiding this comment

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

medium

The script accesses layer attributes using names with a leading underscore (e.g., _self_attention_layer, _query_dense). Use the public attribute names for setting weights instead.

Suggested change
keras_model.get_layer(
f"transformer_layer_{layer_index}"
)._self_attention_layer._query_dense.set_weights(weights)
keras_model.get_layer(
f"transformer_layer_{layer_index}"
).self_attention_layer.query_dense.set_weights(weights)

@sachinprasadhs sachinprasadhs moved this to In Progress in KerasHub Jul 16, 2025
@divyashreepathihalli
Copy link
Collaborator

/gemini review

Copy link

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

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 adds the StableLM-3B 4E1T model, which is a valuable contribution. The implementation largely follows the repository's style guide, including the modular structure and naming conventions. However, there are a few critical omissions and issues that need to be addressed. Most importantly, the model presets file (stablelm_presets.py) is missing, which is required by the style guide [^1] and is essential for loading pre-trained models. The PR also lacks the required validation Colab notebooks [^2]. Additionally, I've found a critical bug in the CausalLM implementation due to the use of a removed utility function, and a few other issues in the backbone and checkpoint conversion script that I've detailed in the specific comments.

Comment on lines +137 to +139
end_locations = any_equal(
token_ids, stop_token_ids, ops.logical_not(padding_mask)
)

Choose a reason for hiding this comment

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

critical

The any_equal utility function has been removed in this PR (as seen in the changes to sampler.py), but it's still being used here. This will cause a runtime error.

You should replace this with equivalent logic to check for stop tokens. Here's a suggested implementation that achieves the same result.

Suggested change
end_locations = any_equal(
token_ids, stop_token_ids, ops.logical_not(padding_mask)
)
stop_token_ids_tensor = ops.convert_to_tensor(
stop_token_ids, dtype=token_ids.dtype
)
is_stop_token = ops.any(
token_ids[..., None] == stop_token_ids_tensor, axis=-1
)
end_locations = is_stop_token & ops.logical_not(padding_mask)

cfg["max_sequence_length"] = pt_cfg["max_position_embeddings"]
cfg["layer_norm_epsilon"] = pt_cfg["layer_norm_eps"]
cfg["rope_max_wavelength"] = pt_cfg["rope_theta"]
cfg["partial_rotary_factor"] = pt_cfg["partial_rotary_factor"]

Choose a reason for hiding this comment

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

high

The configuration key partial_rotary_factor is extracted from the Hugging Face config, but the StableLMBackbone expects rotary_percentage. This will cause a TypeError when initializing the backbone because of an unexpected keyword argument. You should use the correct key rotary_percentage.

Suggested change
cfg["partial_rotary_factor"] = pt_cfg["partial_rotary_factor"]
cfg["rotary_percentage"] = pt_cfg["partial_rotary_factor"]

self.token_embedding = ReversibleEmbedding(
input_dim=vocabulary_size,
output_dim=hidden_dim,
tie_weights=False,

Choose a reason for hiding this comment

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

medium

The tie_word_embeddings argument is documented in the Args section of the docstring but is not included in the __init__ method's signature. This causes a discrepancy between the documentation and the implementation, and prevents users from configuring this behavior. 1

To fix this, you should:

  1. Add tie_word_embeddings=False to the __init__ signature.
  2. Store it as self.tie_word_embeddings.
  3. Pass tie_weights=self.tie_word_embeddings to the ReversibleEmbedding layer.
  4. Include it in get_config().
Suggested change
tie_weights=False,
tie_weights=tie_word_embeddings,

Style Guide References

Footnotes

  1. Docstrings should accurately document all parameters. The tie_word_embeddings parameter is in the docstring but not in the function signature, making it unusable. (link)

if model_parallel_dim_name not in device_mesh.axis_names:
raise ValueError(
f"{model_parallel_dim_name} is not found in the "
f"device_mesh.axis_names. {device_mesh.axis_name=}"

Choose a reason for hiding this comment

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

medium

There's a typo in the f-string. It should be device_mesh.axis_names instead of device_mesh.axis_name to correctly display the available axis names in the error message.

Suggested change
f"device_mesh.axis_names. {device_mesh.axis_name=}"
f"device_mesh.axis_names. {device_mesh.axis_names=}"

if data_parallel_dim_name not in device_mesh.axis_names:
raise ValueError(
f"{data_parallel_dim_name} is not found in the "
f"device_mesh.axis_names. {device_mesh.axis_name=}"

Choose a reason for hiding this comment

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

medium

There's a typo in the f-string. It should be device_mesh.axis_names instead of device_mesh.axis_name to correctly display the available axis names in the error message.

Suggested change
f"device_mesh.axis_names. {device_mesh.axis_name=}"
f"device_mesh.axis_names. {device_mesh.axis_names=}"

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
WIP Pull requests which are work in progress and not ready yet for review.
Projects
Status: In Progress
Development

Successfully merging this pull request may close these issues.

6 participants