Skip to content

Conversation

abheesht17
Copy link
Collaborator

@abheesht17 abheesht17 commented Mar 2, 2023

@abheesht17 abheesht17 requested a review from mattdangerw March 2, 2023 19:39
@abheesht17
Copy link
Collaborator Author

Reverting back to the original since we can't set "trainable" for weights:

AttributeError: Exception encountered when calling layer "transformer_encoder_layer_0" (type WhisperEncoder).

in user code:

    File "/content/keras-nlp/keras_nlp/layers/transformer_encoder.py", line 180, in call  *
        self._build(inputs.shape)
    File "/content/keras-nlp/keras_nlp/models/whisper/whisper_encoder.py", line 41, in _build  *
        self._self_attention_layer._key_dense.bias.trainable = False

    AttributeError: can't set attribute


Call arguments received by layer "transformer_encoder_layer_0" (type WhisperEncoder):
  • inputs=tf.Tensor(shape=(None, None, 384), dtype=float32)
  • padding_mask=None
  • attention_mask=None

Copy link
Member

@mattdangerw mattdangerw left a comment

Choose a reason for hiding this comment

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

This looks great to me! Just some small comments, but let's add tests!

@abheesht17 abheesht17 requested a review from mattdangerw March 8, 2023 03:07
Copy link
Member

@mattdangerw mattdangerw left a comment

Choose a reason for hiding this comment

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

Looks great! Just a last round of comments I think.

@abheesht17 abheesht17 requested a review from mattdangerw March 9, 2023 19:41
Copy link
Member

@mattdangerw mattdangerw left a comment

Choose a reason for hiding this comment

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

Two nits. But let's merge after that!

# The number of mel-frequency filters. We hardcode this to 80:
# https://github.com/openai/whisper/blob/v20230124/whisper/audio.py#L101-L102.
# TODO: If needed, we can make it configurable.
num_mels = 80
Copy link
Member

Choose a reason for hiding this comment

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

Let's do NUM_MELS = 80 after the import block but before the class, that is approach we have used before.

Copy link
Member

@mattdangerw mattdangerw left a comment

Choose a reason for hiding this comment

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

Thanks! Excited for this.

@mattdangerw mattdangerw merged commit 007840b into keras-team:master Mar 10, 2023
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