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.

Sign up for GitHub

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

Jump to bottom

[tests] fix "Tester object has no attribute '_testMethodName'" #34910

Merged
merged 16 commits into from
Dec 13, 2024
Merged

[tests] fix "Tester object has no attribute '_testMethodName'" #34910

merged 16 commits into from
Dec 13, 2024

Conversation

faaany
Copy link
Contributor

@faaany faaany commented Nov 25, 2024 •
edited
Loading

What does this PR do?

I am not sure whether this is related to my test environment, but I got a lot of these kinds of failures when running tests as shown below:

============================================================= ERRORS ==============================================================
__________________________ ERROR collecting tests/models/pix2struct/test_image_processing_pix2struct.py ___________________________
/usr/lib/python3.10/unittest/case.py:461: in __hash__
    return hash((type(self), self._testMethodName))
E   AttributeError: 'Pix2StructImageProcessingTester' object has no attribute '_testMethodName'
_____________________________ ERROR collecting tests/models/pixtral/test_image_processing_pixtral.py ______________________________
/usr/lib/python3.10/unittest/case.py:461: in __hash__
    return hash((type(self), self._testMethodName))
E   AttributeError: 'PixtralImageProcessingTester' object has no attribute '_testMethodName'
__________________________ ERROR collecting tests/models/pop2piano/test_feature_extraction_pop2piano.py ___________________________
/usr/lib/python3.10/unittest/case.py:461: in __hash__
    return hash((type(self), self._testMethodName))
E   AttributeError: 'Pop2PianoFeatureExtractionTester' object has no attribute '_testMethodName'
======================================================== warnings summary =========================================================

After the fix, tests run through. Could you take a look? Thanks a lot!

@amyeroberts @ydshieh

@Rocketknight1
Copy link
Member

I'm not sure we've observed this on our own test runners, but cc @ydshieh anyway

@ydshieh
Copy link
Collaborator

ydshieh commented Nov 26, 2024

Hi @faaany . Could you provide the command that you used to launch the test? Thank you

@ydshieh
Copy link
Collaborator

ydshieh commented Nov 26, 2024

But IMO, those classes like SegformerImageProcessingTester should not be a subclass of unittest.Testcase. They are not the classes to be tested but just helpers for the corresponding classes like SegformerImageProcessingTest.

Don't know why they are like that 😅

@faaany
Copy link
Contributor Author

faaany commented Nov 27, 2024

Hi @faaany . Could you provide the command that you used to launch the test? Thank you

Below is my command example. Actually nothing special, I used to run with this command before and it worked. Could you help run this command on your machine and see how it goes?

python -m pytest tests/models/s*

@ydshieh
Copy link
Collaborator

ydshieh commented Nov 27, 2024

Sure. But I guess you mean python -m pytest tests/models/*?
BTW, in our CI workflow, each model is run in a separate (GitHub Action) job, rather than launched in the same process (like you do here), so there might have some difference.

@ydshieh
Copy link
Collaborator

ydshieh commented Nov 27, 2024

Hmm, not able to reproduce it in our CI env. But as mentioned, IMO, the changes should be remove the unittest.TestCase part in class ASTFeatureExtractionTester(unittest.TestCase): and similar place.

@faaany Would you be up to do this change? After that, we can check again if there is still some issues (running in your env.) and see what extra would be needed to do.

@faaany
Copy link
Contributor Author

faaany commented Nov 29, 2024

Hmm, not able to reproduce it in our CI env. But as mentioned, IMO, the changes should be remove the unittest.TestCase part in class ASTFeatureExtractionTester(unittest.TestCase): and similar place.

@faaany Would you be up to do this change? After that, we can check again if there is still some issues (running in your env.) and see what extra would be needed to do.

Sure, good ideas! I will try this out.

@faaany
Copy link
Contributor Author

faaany commented Nov 29, 2024

@ydshieh yes, after removing unittest.TestCase, it works in my env

@faaany
Copy link
Contributor Author

faaany commented Nov 29, 2024

done. it works.

ydshieh
ydshieh approved these changes Nov 29, 2024
View reviewed changes
Copy link
Collaborator

@ydshieh ydshieh left a comment

Choose a reason for hiding this comment

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

Glad it works in you env now, XPU must be happy this Friday!

Thanks again.

@ydshieh
Copy link
Collaborator

ydshieh commented Nov 29, 2024

@Rocketknight1 Would you like to take final look and maybe give us a ✅ 🙏 ?

TL;DR: the Tester class is not the class to be tested, they are just some helpers.

@faaany
Copy link
Contributor Author

faaany commented Dec 3, 2024

Hi @Rocketknight1, could you take a look? Thanks a lot!

Rocketknight1
Rocketknight1 approved these changes Dec 3, 2024
View reviewed changes
Copy link
Member

@Rocketknight1 Rocketknight1 left a comment

Choose a reason for hiding this comment

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

This looks clean to me, but it's big enough that I should get a final review from a core maintainer! cc @ArthurZucker @LysandreJik

Quick summary for them: Several "Tester" classes were subclasses of unittest.TestCase, but didn't actually have tests - they were just helpers for the actual test classes. This PR removes that subclassing, which fixes some issues.

@faaany
Copy link
Contributor Author

faaany commented Dec 9, 2024

I think the failed tests are unrelated to my changes. Could you help retrigger the CI, when it passes?

ArthurZucker
ArthurZucker approved these changes Dec 13, 2024
View reviewed changes
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.

LGTM thanks

@ydshieh ydshieh merged commit bdd4201 into huggingface:main Dec 13, 2024
20 of 22 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@ydshieh ydshieh ydshieh approved these changes

@ArthurZucker ArthurZucker ArthurZucker approved these changes

@Rocketknight1 Rocketknight1 Rocketknight1 approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@faaany @Rocketknight1 @ydshieh @ArthurZucker