Skip to content
Open
Changes from 1 commit
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
45 changes: 32 additions & 13 deletions i6_models/assemblies/transformer/transformer_decoder_v1.py
Original file line number Diff line number Diff line change
Expand Up @@ -20,7 +20,7 @@
import torch.nn.functional as F

from dataclasses import dataclass, field
from typing import List, Optional, Tuple, TypedDict, Union
from typing import List, Optional, Tuple, TypedDict, Union, NotRequired

from i6_models.config import ModelConfiguration
from i6_models.parts.conformer import (
Expand Down Expand Up @@ -141,6 +141,8 @@ class TransformerDecoderV1Config(ModelConfiguration):
logits_bias: Whether to add a bias to the output logits.
Usually False is a good choice.
share_embedding: Whether to share the input and output embedding.
use_positional_encoding: use a sinus positional encoding on the initial input
do_output_embedding_matmul: apply the final model output x output embedding matmul
"""

block_cfg: TransformerDecoderBlockV1Config
Expand All @@ -150,13 +152,15 @@ class TransformerDecoderV1Config(ModelConfiguration):
num_output: int
logits_bias: bool
share_embedding: bool
use_positional_encoding: bool = True

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

I wonder if, instead of being a flag, this should be a configurable module instead, which you simply replace with a noop if you don't want any positional encoding. This would allow using other positional encoding schemes other than sinusoidal as well.

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Yes, agree, better would be to have this more dynamic.

ConformerMHSARelPosV1._sinusoidal_pe should maybe be moved to a separate function, and then you would have positional_encoding=absolute_sinusoidal_positional_encoding as default, and None is also allowed.

do_output_embedding_matmul: bool = True

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Perhaps

Suggested change
do_output_embedding_matmul: bool = True
embed_outputs_to_vocab_dim: bool = True

is clearer naming-wise?

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

I don't think it's cleaner. But I also don't like the original name. But I'm also not sure whether I like the logic at all (see my separate comment on this, why to have the out_logits at all if it is not used).

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

If this is False, and not cfg.share_embedding, the out_logits are not used at all. Does it make sense to even have them then?



class TransformerDecoderV1State(TypedDict):
"""Recurrent state of the transformer decoder."""

block_state: List[TransformerDecoderBlockV1State]
pos: Tensor
pos: NotRequired[Tensor]


class TransformerDecoderV1(nn.Module, ModuleWithState[TransformerDecoderV1State]):
Expand Down Expand Up @@ -190,13 +194,20 @@ def __init__(self, cfg: TransformerDecoderV1Config):
else:
self.out_logits = nn.Linear(self.model_dim, cfg.num_output, bias=cfg.logits_bias)

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

I just realize, this sharing is weird. I would always set self.out_logits. If sharing, you can just do self.out_logits.weights = self.input_embedding.weight. That would simplify the other code.

Also, self.out_logits should always be set (be None if not used). But with my suggestion, you don't need to care about this.

And then you would also allow to have logits_bias=True with share_embedding=True.


self.use_positional_encoding = cfg.use_positional_encoding
self.do_output_embedding_matmul = cfg.do_output_embedding_matmul

def get_initial_state(self) -> TransformerDecoderV1State:
""":return: initial decoder state"""
return {
state: TransformerDecoderV1State = {
"block_state": [block.get_initial_state() for block in self.module_list],
"pos": torch.tensor(0, dtype=torch.int32),
}

if self.use_positional_encoding:
state["pos"] = torch.tensor(0, dtype=torch.int32)

return state

def transform_encoder_output(
self,
encoder_output: Tensor,
Expand Down Expand Up @@ -229,10 +240,13 @@ def forward(
- `s = get_initial_state()`.
"""
x = self.input_embedding(labels) * self.input_embedding_scale
sinus_pe = ConformerMHSARelPosV1._sinusoidal_pe(
torch.arange(labels.shape[-1], device=labels.device) + state["pos"], self.model_dim
)
x = x + sinus_pe.unsqueeze(0)

if self.use_positional_encoding:
sinus_pe = ConformerMHSARelPosV1._sinusoidal_pe(
torch.arange(labels.shape[-1], device=labels.device) + state["pos"], self.model_dim
)
x = x + sinus_pe.unsqueeze(0)

x = self.input_dropout(x)

output = x
Expand All @@ -243,11 +257,16 @@ def forward(
new_state: TransformerDecoderV1State = {
**state,
"block_state": new_block_states,
"pos": state["pos"] + labels_lens.max(),
}

if self.use_positional_encoding:
new_state["pos"] = state["pos"] + labels_lens.max()

output = self.out_norm(output)
output_logits = (
F.linear(output, self.input_embedding.weight, None) if self.share_embedding else self.out_logits(output)
)
return output_logits, new_state

if self.do_output_embedding_matmul:
output = (
F.linear(output, self.input_embedding.weight, None) if self.share_embedding else self.out_logits(output)
)

return output, new_state