-
Notifications
You must be signed in to change notification settings - Fork 27.5k
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
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! |
* copied statement for test files --------- Co-authored-by: ydshieh <[email protected]>
* copied statement for test files --------- Co-authored-by: ydshieh <[email protected]>
* copied statement for test files --------- Co-authored-by: ydshieh <[email protected]>
What does this PR do?
Copied from
for test files.Run
make fix-copies
will showI will need to update
test_tokenization_longformer.py
before merge.