Skip to content

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

@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
@ydshieh ydshieh deleted the copied_for_test_files branch October 11, 2023 12:12
@LysandreJik
Copy link
Member

Ok!

@ydshieh ydshieh mentioned this pull request Nov 8, 2023
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.

5 participants