- 
                Notifications
    You must be signed in to change notification settings 
- Fork 31k
          Copied from for test files
          #26713
        
          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
  
    Copied from for test files
  
  #26713
              
            Conversation
| well, I need to check why fails. | 
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.
Looks good to me in terms of changes
| The documentation is not available anymore as the PR was closed or merged. | 
| # Detail: the `Copied from` statement is originally designed to work with the last part of `TRANSFORMERS_PATH`, | ||
| # (which is `transformers`). The same should be applied for `MODEL_TEST_PATH`. However, its last part is `models` | ||
| # (to only check and search in it) which is a bit confusing. So we keep the copied statement staring with | ||
| # `tests.models.` and change it to `tests` here. | ||
| if base_path == MODEL_TEST_PATH: | ||
| base_path = "tests" | 
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.
This is to make # Copied from tests.models. work (instead of # Copied from models.) and still keep MODEL_TEST_PATH = "tests/models"
| Will have to perform some fixes about copied statements before merge. | 
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.
💘
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.
The changes to the check_copies.py file look good to me. I'm curious if we couldn't rework slightly our test files so that close-to-exact copies can be copied as entire files rather than individual methods. I think this would make contributions quite simpler
(if not, it's ok as long as users call add-new-model-like)
|  | ||
| # Copied from transformers.tests.roberta.test_modeling_roberta.py with Roberta->Longformer | ||
| @require_tokenizers | ||
| class LongformerTokenizationTest(TokenizerTesterMixin, unittest.TestCase): | 
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.
Would be simpler for contributors to have the full class copied instead of adding manual copied from statements to so many tests. It wasn't possible in this case?
|  | ||
| # Copied from transformers.tests.mistral.test_modelling_mistral.MistralModelTest with Llama->Mistral | ||
| class MistralModelTester: | 
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.
Here too it would be really great to keep the class-wide comment
| If there is a single place, say  | 
| Ok! | 
What does this PR do?
Copied fromfor test files.Run
make fix-copieswill showI will need to update
test_tokenization_longformer.pybefore merge.