Skip to content

Conversation

ylacombe
Copy link
Contributor

What does this PR do?

Fix some Bark failing tests where the logits processors' tensors were misplaced on CPU!
It was introduced by #29804 but not flagged because of the Bark custom generation!

Slow tests are now all green on a A100!

cc @ydshieh and @amyeroberts

@HuggingFaceDocBuilderDev

The docs for this PR live here. All of your documentation changes will be reflected on that endpoint. The docs are available until 30 days after the last update.

Copy link
Contributor

@amyeroberts amyeroberts 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 working on a fix!

I'm a bit confused by this solution - neither SuppressTokensLogitsProcessor nor BarkEosPrioritizerLogitsProcessor take device as an init kwarg My bad - I was looking at an older commit

Copy link
Contributor

@amyeroberts amyeroberts 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 fixing!

Tbh, this pattern of passing the device here I think is a bit odd (why do we take a tensor, turn it into a list, then turn it back into a tensor on the device? but that's more of a generate question)

@amyeroberts
Copy link
Contributor

It was introduced by #29804 but not flagged because of the Bark custom generation!

@ylacombe Does this mean that Bark's generation isn't properly tested?

@ylacombe
Copy link
Contributor Author

@ylacombe Does this mean that Bark's generation isn't properly tested?

It's not at the moment, we've discussed the reasons why in this thread. It might make sense to revisit this decision once the standard generation method is refactored, WDYT ?

@ydshieh
Copy link
Collaborator

ydshieh commented Jun 14, 2024

Thank you @ylacombe !

@amyeroberts There is still BarkModelIntegrationTests and it catches the bug :-) which leads to this PR.

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.

💯

@ylacombe ylacombe merged commit 7ae4fc2 into huggingface:main Jun 17, 2024
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.

4 participants