Skip to content
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

Add part: lstm block #66

Open
wants to merge 7 commits into
base: main
Choose a base branch
from
Open
Changes from 2 commits
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
64 changes: 64 additions & 0 deletions i6_models/parts/lstm.py
Original file line number Diff line number Diff line change
@@ -0,0 +1,64 @@
__all__ = ["LstmBlockV1Config", "LstmBlockV1"]

from dataclasses import dataclass
import torch
from torch import nn
from typing import Dict, Union

from i6_models.config import ModelConfiguration


@dataclass
class LstmBlockV1Config(ModelConfiguration):
christophmluscher marked this conversation as resolved.
Show resolved Hide resolved
input_dim: int
hidden_dim: int
num_layers: int
bias: bool
dropout: float
bidirectional: bool
Copy link
Contributor

Choose a reason for hiding this comment

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

should we allow this? I feel like if we have bidirectional here, the BLSTM part becomes redundant, which is maybe okay, but might also cause two different branches that do the same, which I am not sure we want (if there are potential extensions later). We could maybe also just deprecate the BLSTM block?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Good point I ll just remove the flag. Is maybe a bit more readable having two classes?!

enforce_sorted: bool

@classmethod
def from_dict(cls, model_cfg_dict: Dict):
Copy link
Member

Choose a reason for hiding this comment

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

Same Q as in the other PR: why is this necessary now, and hasn't been for the other assemblies?

christophmluscher marked this conversation as resolved.
Show resolved Hide resolved
model_cfg_dict = model_cfg_dict.copy()
return cls(**model_cfg_dict)


class LstmBlockV1(nn.Module):
def __init__(self, model_cfg: Union[LstmBlockV1Config, Dict], **kwargs):
christophmluscher marked this conversation as resolved.
Show resolved Hide resolved
super().__init__()

self.cfg = LstmBlockV1Config.from_dict(model_cfg) if isinstance(model_cfg, Dict) else model_cfg
christophmluscher marked this conversation as resolved.
Show resolved Hide resolved

self.dropout = self.cfg.dropout
self.enforce_sorted = None
self.lstm_stack = nn.LSTM(
input_size=self.cfg.input_dim,
hidden_size=self.cfg.hidden_dim,
num_layers=self.cfg.num_layers,
bias=self.cfg.bias,
dropout=self.dropout,
batch_first=True,
bidirectional=self.cfg.bidirectional,
)

def forward(self, x: torch.Tensor, seq_len: torch.Tensor) -> (torch.Tensor, torch.Tensor):
christophmluscher marked this conversation as resolved.
Show resolved Hide resolved
if not torch.jit.is_scripting() and not torch.jit.is_tracing():
Copy link
Member

Choose a reason for hiding this comment

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

Why only when not scripting? Don't you want that seq_len is always on CPU?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I followed the example in the blstm part.

        if not torch.jit.is_scripting() and not torch.jit.is_tracing():
            # during graph mode we have to assume all Tensors are on the correct device,
            # otherwise move lengths to the CPU if they are on GPU
            if seq_len.get_device() >= 0:
                seq_len = seq_len.cpu()

I did not copy the comment over... I did not yet get to look why this is necessary
@JackTemaki you implemented the BLSTM IIRC. You remember why this was done in this way?

Copy link
Contributor

Choose a reason for hiding this comment

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

The question is, is this still relevant? This was something I added at some point, but if this is not needed for ONNX export this should be removed until there is actually a reason for it.

if seq_len.get_device() >= 0:
seq_len = seq_len.cpu()
Comment on lines +83 to +84
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
if seq_len.get_device() >= 0:
seq_len = seq_len.cpu()
seq_len = seq_len.cpu()


lstm_packed_in = nn.utils.rnn.pack_padded_sequence(
input=x,
lengths=seq_len,
enforce_sorted=self.enforce_sorted,
batch_first=True,
)

lstm_out, _ = self.lstm_stack(lstm_packed_in)
lstm_out, _ = nn.utils.rnn.pad_packed_sequence(
Copy link
Contributor

Choose a reason for hiding this comment

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

Just out of curiosity: why does black force the new lines here but not for the blstm? Shouldnt it be the same line length?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

well this is dependent if you manually set the last commata. if set it will force new lines

lstm_out,
padding_value=0.0,
batch_first=True,
)

return lstm_out, seq_len
Loading