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

Copied from for test files #26713

Merged
merged 13 commits into from
Oct 11, 2023
Merged

Copied from for test files #26713

merged 13 commits into from
Oct 11, 2023

Conversation

ydshieh
Copy link
Collaborator

@ydshieh ydshieh commented Oct 10, 2023

What does this PR do?

Copied from for test files.

Run make fix-copies will show

python utils/check_copies.py --fix_and_overwrite
Detected changes, rewriting tests/models\longformer\test_tokenization_longformer.py.

I will need to update test_tokenization_longformer.py before merge.

@ydshieh ydshieh requested a review from ArthurZucker October 10, 2023 09:11
@ydshieh ydshieh removed the request for review from ArthurZucker October 10, 2023 09:15
@ydshieh ydshieh marked this pull request as draft October 10, 2023 09:15
@ydshieh ydshieh requested a review from ArthurZucker October 10, 2023 09:21
@ydshieh ydshieh marked this pull request as ready for review October 10, 2023 09:21
@ydshieh
Copy link
Collaborator Author

ydshieh commented Oct 10, 2023

well, I need to check why

ci/circleci: tests_repo_utils

fails.

@ydshieh ydshieh marked this pull request as draft October 10, 2023 09:30
Copy link
Collaborator

@ArthurZucker ArthurZucker left a 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

utils/check_copies.py Outdated Show resolved Hide resolved
utils/check_copies.py Outdated Show resolved Hide resolved
@HuggingFaceDocBuilderDev
Copy link

HuggingFaceDocBuilderDev commented Oct 10, 2023

The documentation is not available anymore as the PR was closed or merged.

Comment on lines +152 to +157
# 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"
Copy link
Collaborator Author

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"

@ydshieh
Copy link
Collaborator Author

ydshieh commented Oct 10, 2023

Will have to perform some fixes about copied statements before merge.

@ydshieh ydshieh marked this pull request as ready for review October 11, 2023 09:40
Copy link
Collaborator

@ArthurZucker ArthurZucker left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

💘

Copy link
Member

@LysandreJik LysandreJik left a 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)

Comment on lines -30 to 31
# Copied from transformers.tests.roberta.test_modeling_roberta.py with Roberta->Longformer
@require_tokenizers
class LongformerTokenizationTest(TokenizerTesterMixin, unittest.TestCase):
Copy link
Member

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:
Copy link
Member

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

@ydshieh
Copy link
Collaborator Author

ydshieh commented Oct 11, 2023

If there is a single place, say __init__, being a non-copy, then so far we can't put # Copied from at the class level. We can rework the script a bit to have # Ignore copied in a method, but would be better to do this in a follow up PR.

@ydshieh ydshieh merged commit 5334796 into main Oct 11, 2023
3 checks passed
@ydshieh ydshieh deleted the copied_for_test_files branch October 11, 2023 12:12
@LysandreJik
Copy link
Member

Ok!

helboukkouri pushed a commit to helboukkouri/transformers that referenced this pull request Oct 16, 2023
* copied statement for test files

---------

Co-authored-by: ydshieh <[email protected]>
@ydshieh ydshieh mentioned this pull request Nov 8, 2023
blbadger pushed a commit to blbadger/transformers that referenced this pull request Nov 8, 2023
* copied statement for test files

---------

Co-authored-by: ydshieh <[email protected]>
EduardoPach pushed a commit to EduardoPach/transformers that referenced this pull request Nov 18, 2023
* copied statement for test files

---------

Co-authored-by: ydshieh <[email protected]>
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