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

CI: avoid human error, automatically infer generative models #33212

Draft
wants to merge 12 commits into
base: main
Choose a base branch
from

Conversation

gante
Copy link
Member

@gante gante commented Aug 30, 2024

What does this PR do?

This PR:

  1. Replaces manual definition of generative models to test with generate with an automatic definition -- if model_class.can_generate(), then it runs tests from GenerationTesterMixin. No more human mistakes, which happened frequently in the past months 🐛 🔫
  2. Now that we run generation tests for all models that can generate, there are a few (old) bad apples. Explicitly skip them i.e. overwrite the automated all_generative_model_classes and explain why certain classes are being skipped. (bad apples detected with py.test tests/models/ -k test_greedy_generate -vv) 💔
  3. Moves tests that call generate from the generic test mixin to GenerationTesterMixin. This means a) we can have a better overview of what's being tested with generate; and b) model architectures without generative capabilities will have fewer skips 🎯

In a follow-up PR:

  1. We need to manually define the model's main input name (e.g., pixel_values) in the model tester. Make it use model.main_input_name instead, to avoid human error Done ✅
  2. Despite the changes in this PR, generate tests will only run if GenerationTesterMixin is inherited. We can easily forget to add the mixin, resulting in a false positive. Add an automated check: if any of the model classes can generate, then GenerationTesterMixin must be inherited in the tester

@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.

@gante gante changed the title CI: automatically infer generative models CI: avoid human error, automatically infer generative models Aug 30, 2024
@zucchini-nlp
Copy link
Member

Super cool! If VLMs start failing on generation tests, that's okay. I have a local draft for that, which needs to be reviewed after a few other PRs are merged

@gante gante force-pushed the run_all_generate_tests_all_times branch from 489a2d6 to 1e8e794 Compare September 20, 2024 16:45
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.

3 participants