Repository: ludwig-ai/ludwig
Pull request: Enable end-to-end training for text features using ParallelCNN
This PR allows end to end training for text input.

- Added output shapes to ParallelCNN.

Problem statement:
Test and remove any regressions in modeling performance due to PyTorch migration
Test performance regressions on key datasets

- [x] ATIS
- [x] MNIST
- [x] Titanic

These are tests that are triggered periodically that train lightweight models on standard datasets and confirm that there aren't any performance regressions.

Hints from issue discussion:

Commit message: Added output shapes to ParallelCNN

Patch to review:
diff --git a/ludwig/encoders/sequence_encoders.py b/ludwig/encoders/sequence_encoders.py
index 44018eb9ebc..237e300260d 100644
--- a/ludwig/encoders/sequence_encoders.py
+++ b/ludwig/encoders/sequence_encoders.py
@@ -520,10 +520,6 @@ def __init__(
                 default_dropout=dropout,
             )
 
-    @property
-    def input_shape(self) -> torch.Size:
-        return torch.Size([self.max_sequence_length])
-
     def forward(self, inputs, training=None, mask=None):
         """
             :param inputs: The input sequence fed into the encoder.
@@ -534,9 +530,7 @@ def forward(self, inputs, training=None, mask=None):
         """
         # ================ Embeddings ================
         if self.should_embed:
-            embedded_sequence = self.embed_sequence(
-                inputs, training=training, mask=mask
-            )
+            embedded_sequence = self.embed_sequence(inputs)
         else:
             embedded_sequence = inputs
             while len(embedded_sequence.shape) < 3:
@@ -565,6 +559,16 @@ def forward(self, inputs, training=None, mask=None):
 
         return {'encoder_output': hidden}
 
+    @property
+    def input_shape(self) -> torch.Size:
+        return torch.Size([self.max_sequence_length])
+
+    @property
+    def output_shape(self) -> torch.Size:
+        if self.reduce_output is not None:
+            return self.fc_stack.output_shape
+        else:
+            return self.parallel_conv1d.output_shape
 
 @register(name='stacked_cnn')
 class StackedCNN(SequenceEncoder):
diff --git a/ludwig/modules/embedding_modules.py b/ludwig/modules/embedding_modules.py
index a2c31098316..fee0155b65e 100644
--- a/ludwig/modules/embedding_modules.py
+++ b/ludwig/modules/embedding_modules.py
@@ -422,7 +422,8 @@ def __init__(
         else:
             self.dropout = None
 
-    def forward(self, inputs, training=None, mask=None):
+    def forward(self, inputs: torch.Tensor):
+        inputs = inputs.int()
         embedded = self.embeddings(inputs)
         if self.dropout:
             embedded = self.dropout(embedded)


Code review comments:
diff_hunk: @@ -422,7 +422,8 @@ def __init__(
         else:
             self.dropout = None
 
-    def forward(self, inputs, training=None, mask=None):
+    def forward(self, inputs: torch.Tensor):
+        inputs = inputs.int()
path: ludwig/modules/embedding_modules.py
text: @user1:
I guess the intent here is to make sure that all inputs are integers, which is a good intent, although I'd rather not have this cast. The rationale is that is someone passes a float or boolean tensor, this will result in an error, and it's probably better to raise an error in that case than silently doing a cast that may hide something wrong in the logic. People should not pass float or boolean tensors to begin with. So maybe the intent is better captured by an assert?

@author:
@user1 makes sense generally. However, what should the appropriate place for casting input types be? E.g. here, the input being passed in was a `Float` tensor while the Embedding module only support `Int` or `Long` tensors.

@user1:
The tensors are coming from here:
https://github.com/ludwig-ai/ludwig/blob/tf-to-torch/ludwig/encoders/sequence_encoders.py#L537-L539

Which in turn are coming from here:
https://github.com/ludwig-ai/ludwig/blob/tf-to-torch/ludwig/features/text_feature.py#L267-L280

So there's already an assert and already a cast to int32 (which should probably look into because the input could be int64, but that's another story). So not sure how it was possible you were getting floats, it would probably be worth putting a breakpoint there and going back in the trace to figure it out as is is likely a bug somewhere I can't crrently see it.

@author:
Removing the int casting ended up not causing a failure - I believe the bug that I was running into might have been fixed by a rebase.

Added a test to check the data type of the input tensor instead!