refactor(seq2seq): improve Seq2Seq data collation and logging - #80
Merged
Conversation
- Replace tokenizer pad with DataCollatorForSeq2Seq in Seq2SeqTransformerDataModule - Modify evaluation logging to handle cases with non-tensor values in BaseTransformerModule - Ensure consistency in logging loss and perplexity during training in SimpleT5Model - Update LMScorer to correctly handle a mix of tensor and non-tensor values, supporting DistillationLoss [tests] - test: add tests for Seq2Seq collation and LMScorer changes - Add test to ensure correct padding behavior with ignore index in Seq2SeqTransformerDataModule - Add test to validate that LMScorer supports both tensor and DistillationLoss objects
…y checks - Move the default model initialization into the constructor based on `pretrained_model` and `model_config` - Delay the `super().__init__` call until after the model is potentially initialized within the class constructor [bert_squeeze] - fix: correct scorer label initialization - Initialize the `BaseSequenceClassificationScorer` with a list of label indices instead of `self.num_labels` [tests] - test: add tests for BaseSequenceClassificationTransformerModule default scorer - Ensure scorer is properly initialized with correct labels and distinct scorer objects for validation and test sets
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Description
Improves Seq2Seq data handling and logging robustness. Replaces manual tokenizer padding with
DataCollatorForSeq2Seqto properly handle label padding with ignore index (-100), preventing labels from being padded with the tokenizer's pad token. Refactors evaluation logging to support both tensor and non-tensor values (e.g.,DistillationLossobjects), and restructures model initialization order to ensure models are properly configured before calling parent constructors. Also fixes scorer initialization to use label lists instead of integers.Risk
Low risk - changes improve correctness and robustness. The data collation change ensures labels are properly masked during training. Model initialization order change ensures proper configuration but should be tested across different model types.
Tests
Comprehensive unit tests added:
test_collate_pads_labels_with_ignore_index: validates label padding with -100test_lm_scorer_supports_loss_onlyandtest_lm_scorer_supports_distillation_loss: validate LMScorer handles both tensor and DistillationLosstest_base_sequence_classification_transformer_module_default_scorer: validates correct scorer initialization