Skip to content

Conversation

@gante
Copy link
Contributor

@gante gante commented Feb 13, 2023

What does this PR do?

(see title)

@gante gante changed the title Smaller TFHubertModelTest Fix TF CTC tests Feb 13, 2023
default_batch_size = self.model_tester.batch_size
self.model_tester.batch_size = 2
super().test_dataset_conversion()
super().test_keras_fit()
Copy link
Contributor Author

Choose a reason for hiding this comment

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

ninja copy paste hehe

self.assertIsNotNone(model)

# We override here as passing a full batch of 13 samples results in OOM errors for CTC
def test_dataset_conversion(self):
Copy link
Contributor Author

@gante gante Feb 13, 2023

Choose a reason for hiding this comment

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

This one, in TFHubertModelTest, was the issue. Since it did not have the lower batch size treatment as in the other TF CTC tests, I'm assuming this is the fix.

@gante gante marked this pull request as ready for review February 13, 2023 16:04
@gante gante requested review from sgugger and ydshieh February 13, 2023 16:04
@HuggingFaceDocBuilderDev
Copy link

HuggingFaceDocBuilderDev commented Feb 13, 2023

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

Copy link
Collaborator

@ydshieh ydshieh left a comment

Choose a reason for hiding this comment

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

OK for me. But would like @amyeroberts to take a look (even afterward) to check if any merged commit cause this issue. These tests run with tiny models, usually don't have such memory issue. And we didn't have this failure previously (right?).

@gante
Copy link
Contributor Author

gante commented Feb 13, 2023

@ydshieh before @amyeroberts work in #21502 these tests had no overwrite, correct. However, see the note in Amy's PR -- the labels were not correctly handled, which meant that test_dataset_conversion was probably terminating early here, which would explain the ausence of crashes prior to the PR :)

@susnato susnato mentioned this pull request Feb 13, 2023
5 tasks
Copy link
Collaborator

@sgugger sgugger left a comment

Choose a reason for hiding this comment

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

Thanks for the fix!

@gante gante merged commit 56b03c9 into huggingface:main Feb 13, 2023
@gante gante deleted the hubert_test_size branch February 13, 2023 21:23
@amyeroberts
Copy link
Contributor

Thanks for the fix @gante !

@ydshieh Yes, Joao's correct. There's three different cases where the state of the tests changed:

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