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

enable multiple eval datasets #1052

Merged

Conversation

peter-sk
Copy link
Contributor

@peter-sk peter-sk commented Dec 2, 2023

The standard Trainer class from the transformers library (and the documentation of SFTTrainer) allow for multiple validation datasets to be passed as a dictionary from dataset name to dataset.

This does not work however in current SFTTrainer code. This PR fixes this.

@peter-sk
Copy link
Contributor Author

peter-sk commented Dec 3, 2023

@younesbelkada
Does this make sense?

@pop-srw
Copy link

pop-srw commented Dec 4, 2023

I have the same problem. When multiple eval datasets were pass as dict, It cause an error in dataset.map

tokenized_dataset = dataset.map(

I think this PR is really useful.

@peter-sk
Copy link
Contributor Author

peter-sk commented Dec 4, 2023

I have the same problem. When multiple eval datasets were pass as dict, It cause an error in dataset.map

tokenized_dataset = dataset.map(

I think this PR is really useful.

Yes, exactly this. And this error occurs naturally both when using packing and when not.

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 but I'll let @younesbelkada have a look as well!

@lvwerra
Copy link
Member

lvwerra commented Dec 4, 2023

If you could add a test for this that would be awesome!

@HuggingFaceDocBuilderDev

The docs for this PR live here. All of your documentation changes will be reflected on that endpoint.

@peter-sk
Copy link
Contributor Author

peter-sk commented Dec 4, 2023

@lvwerra
Sure, I can add a test. Are there any guidelines for this? I have tried it for transformers, but not for trl. Any hints are appreciated!

@peter-sk
Copy link
Contributor Author

peter-sk commented Dec 5, 2023

@lvwerra
I added a test.

@peter-sk
Copy link
Contributor Author

peter-sk commented Dec 6, 2023

@lvwerra @younesbelkada
Just a careful push :-)

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.

Looks great to me, thanks for this contribution!

@younesbelkada
Copy link
Contributor

@peter-sk the tests are taking forever, I think that something went wrong with the test you designed, can you please have a quick look?

@peter-sk
Copy link
Contributor Author

peter-sk commented Dec 6, 2023

@younesbelkada
I guess the training dataset was getting iterated (for hundreds of thousands of iterations).
Now, I am using twice the evaluation dataset. This test works fine in my machine.
I also downsized the test.

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.

Thanks again!

@younesbelkada younesbelkada merged commit 5a23354 into huggingface:main Dec 6, 2023
9 checks passed
lapp0 pushed a commit to lapp0/trl that referenced this pull request May 10, 2024
* enable multiple eval datasets

* added test

* try to avoid infinite computation

* make sure eval set is not infinite

* downsizing the test
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.

5 participants