Skip to content

Conversation

ydshieh
Copy link
Collaborator

@ydshieh ydshieh commented Apr 11, 2022

What does this PR do?

class ModelTesterMixin has attribute test_torchscript as well as method named def test_torchscript.

In a few model specific test files, we have test_torchscript = True defined, for example,

T5ModelTest:

test_torchscript = True

DistilBertModelTest:

This actually makes the test_torchscript being a boolean value instead of a method, therefore test_torchscript is not run for these places. See the last section for a dummy example.

Fix

Although this could be fixed just by removing test_torchscript = True, it would be a better idea to rename the method def test_torchscript to something else like def test_torchscript_simple.

Dummy example (to reproduce the issue)

class DummyCommonTest:

    test_me = True

    def test_me(self):
        a = 3
        print(a)

class DummyModelTest1(DummyCommonTest):

    pass

class DummyModelTest2(DummyCommonTest):

    test_me = True

dummy_test = DummyCommonTest()
print(dummy_test.test_me)
dummy_test.test_me()

dummy_model_test_1 = DummyModelTest1()
# A method
print(dummy_model_test_1.test_me)
# can be called
dummy_model_test_1.test_me()

dummy_model_test_2 = DummyModelTest2()
# A boolean
print(dummy_model_test_2.test_me)
# can't be called: (TypeError: 'bool' object is not callable)
dummy_model_test_2.test_me()

@HuggingFaceDocBuilderDev
Copy link

HuggingFaceDocBuilderDev commented Apr 11, 2022

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

@ydshieh ydshieh changed the title rename Rename the method test_torchscript Apr 11, 2022
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.

Great catch, LGTM!

@ydshieh ydshieh removed the request for review from patrickvonplaten April 11, 2022 16:21
@ydshieh ydshieh merged commit 2109afa into huggingface:main Apr 11, 2022
@ydshieh ydshieh deleted the fix_model_tester_mixin_attr branch April 11, 2022 16:21
elusenji pushed a commit to elusenji/transformers that referenced this pull request Jun 12, 2022
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