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

Handle potentially long sequences with DataCollatorForCompletionOnlyLM #644

Merged
merged 4 commits into from
Aug 18, 2023

Conversation

tannonk
Copy link
Contributor

@tannonk tannonk commented Aug 14, 2023

This PR resolves #643 and provides a patch in DataCollatorForCompletionOnlyLM for handling long sequences which may or may not contain a valid response_template or a valid instruction_template.

For problematic instances where no response_template or instruction_template is found, we set the labels to the ignore_idx. As a result, problematic instances are ignored in the loss computation, but still allow for training to continue.

Copy link
Member

@lvwerra lvwerra left a comment

Choose a reason for hiding this comment

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

Looks good to me, thanks for fixing! Just a small nit then we can merge.

tests/test_data_collator_completion_only.py Outdated Show resolved Hide resolved
@HuggingFaceDocBuilderDev
Copy link

HuggingFaceDocBuilderDev commented Aug 15, 2023

The documentation is not available anymore as the PR was closed or merged.

@lvwerra
Copy link
Member

lvwerra commented Aug 15, 2023

Also could you run the code quality tests with make precommit? That should fix the CI.

@tannonk
Copy link
Contributor Author

tannonk commented Aug 15, 2023

Hey @lvwerra, thanks for the review. The requested changes have been made and make precommit passes on my end.

@lvwerra
Copy link
Member

lvwerra commented Aug 15, 2023

Looks like the tests are not passing, yet.

Copy link
Contributor

@younesbelkada younesbelkada left a comment

Choose a reason for hiding this comment

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

Hi @tannonk
Thanks a lot for the PR!
In my opinion, currently raising a RuntimeError really helps for understanding whether things worked correctly or not for users, I am afraid the warning is not a strong enough indicator to users. Maybe it is better to keep it and add a flag allow_ignore_not_matched for advanced users and keep the previous behaviour untouched, what do you think? cc @lvwerra - if that's not a good idea I am happy to merge the PR as it is as well

Copy link
Contributor

@younesbelkada younesbelkada left a comment

Choose a reason for hiding this comment

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

Discussed with @lvwerra , indeed if the sequence is too long the training would completely break and this could happen in the middle of a training, which is too odd. Let's merge it! Thanks again @tannonk for your work on this!

@younesbelkada younesbelkada merged commit 029f961 into huggingface:main Aug 18, 2023
kushal-tri pushed a commit to kushalarora/trl that referenced this pull request Sep 19, 2023
huggingface#644)

* avoid RuntimeError on long sequences

* add unittests and format

* remove dependency on external repo

* bug fix in DataCollatorForCompletionOnlyLM
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Exception handling in DataCollatorForCompletionOnlyLM
4 participants