[ConstantLengthDataset] Fix packed dataset issue
#452
Merged
Conversation
This file contains 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
|
The documentation is not available anymore as the PR was closed or merged. |
lvwerra
reviewed
Jun 21, 2023
docs/source/sft_trainer.mdx
Outdated
| @@ -98,6 +98,8 @@ trainer = SFTTrainer( | |||
| trainer.train() | |||
| ``` | |||
|
|
|||
| Note that if you use a packed dataset and if you pass `max_steps` in the training arguments you will probably train your models for more than few epochs. | |||
There was a problem hiding this comment.
That's not really true, right? It could also be shorter - depends on the dataset and max_steps you choose :)
| @@ -423,6 +423,65 @@ def test_data_collator_completion_lm(self): | |||
| result_text = self.tokenizer.decode(batch["input_ids"][0, last_pad_idx + 1 :]) | |||
| self.assertTrue(result_text == "I have not been masked correctly.") | |||
|
|
|||
| def test_sft_trainer_infinite_with_model(self): | |||
There was a problem hiding this comment.
What do you think about double checking here for both cases if we trained as many steps as we expect?
Co-authored-by: Leandro von Werra <lvwerra@users.noreply.github.com>
…nto fix-packed-dataset
lvwerra
approved these changes
Jun 21, 2023
Closed
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
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.
What does this PR do?
Fixes #450
As reported in that issue, there is an inconsistent behaviour when using
packing=Trueand the number of iterations showed in the logs of training. In fact, the inconsistency comes from the fact that in the packed case, sequences are packed all together on the fly untilmax_seq_lengthis reached, leading to reducing the number of total expected samples.The fix is to force-set the
infiniteargument toTrueif a user explicity decides to train a model withmax_stepsstrategy but properly warn them that the argument has been overriden and warn the user when the dataset is moved to the next iteration.In the case of epochs training strategy, we should force-set infinite argument to False, otherwise the training will run forever.
Added also nice CI tests to make sure that this behavior will stay consistent in future commits. Added also a line in the documentation.
cc @lvwerra @vwxyzjn