-
Notifications
You must be signed in to change notification settings - Fork 28.3k
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
Clean-up generation tests after moving methods to private #29582
Conversation
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. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thank you for the cleanup! 🧼
@zucchini-nlp a more descriptive PR name would be nice :D Musicgen seems to need some retouches, looking at the failing CI. @amyeroberts see the PR header in #29437 for more context on why we are deprecating calls to the internal decoding methods |
Fixed MusicGen. All passed except for two tests for multigpu, but those are failing in |
There was a problem hiding this 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 this tidy up - great to see so much code killed.
Only request before merge is that we still have a max length backwards compatibility test
@@ -2455,220 +2037,6 @@ def test_diverse_beam_search(self): | |||
], | |||
) | |||
|
|||
def test_max_length_backward_compat_greedy(self): |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We should still have at least one test_max_length_backward_compat
test which calls model.generate
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I removed this tests, because we can only check it if the (now private) generate methods are called directly. When calling from the model.generate
, the max_length
get defined by default from the generation config.
We have a test test_default_max_length_warning
, which check max_length warnings when calling directly generate
@zucchini-nlp suggestion: revert docsting changes, as they will cause CI to fail ( #29718 fixes the CI error) :) |
51102e9
to
be52438
Compare
* clean-up tests * refine comments * fix musicgen tests * make style * remove slow decorator from a test * more clean-up * fix other failing tests
What does this PR do?
Folllowing #29437 , this PR removes tests that call "to be private" generation methods. Instead we check each generation method by passing its arguments to the
generate
method.All tests in generation, including slow are passing.
Before submitting
Pull Request section?
to it if that's the case.
documentation guidelines, and
here are tips on formatting docstrings.
Who can review?
Anyone in the community is free to review the PR once the tests have passed. Feel free to tag
members/contributors who may be interested in your PR.
@gante