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

Codemod Unittest assertions to bare asserts #1301

Merged
merged 4 commits into from
Feb 1, 2024

Conversation

akx
Copy link
Contributor

@akx akx commented Jan 31, 2024

This PR runs https://github.com/akx/codemod-unittest-to-pytest-asserts to rewrite Unittest self.assert... assertions as plain assertions that can be introspected by Pytest's assert rewriting.

@HuggingFaceDocBuilderDev

The docs for this PR live here. All of your documentation changes will be reflected on that endpoint. The docs are available until 30 days after the last update.

Copy link
Contributor

@younesbelkada younesbelkada left a comment

Choose a reason for hiding this comment

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

Hi @akx
Thanks for your contribution !
few tests are failing with:

FAILED tests/test_core.py::CoreTester::test_masked_whiten - TypeError: type Tensor doesn't define __round__ method

I think adding .item() on those would fix the tests - could you please have a look? 🙏

@akx
Copy link
Contributor Author

akx commented Feb 1, 2024

@younesbelkada Sure thing. Pushed a commit that hand-rewrites the assert round(...) style tests to an assert abs(...) < ... anyway.

Copy link
Contributor

@younesbelkada younesbelkada left a comment

Choose a reason for hiding this comment

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

Sorry for the additional iteration @akx ! Could you also fix

FAILED tests/test_reward_trainer.py::RewardTrainerTester::test_reward_trainer_margin

? 🙏 Thanks !

@akx akx force-pushed the unittest-asserts branch 2 times, most recently from 2c1d460 to 4021cc9 Compare February 1, 2024 09:16
@akx akx marked this pull request as draft February 1, 2024 09:18
@akx akx force-pushed the unittest-asserts branch from 4021cc9 to 809f9f7 Compare February 1, 2024 09:21
@akx akx force-pushed the unittest-asserts branch from 809f9f7 to e823124 Compare February 1, 2024 17:39
@akx akx marked this pull request as ready for review February 1, 2024 17:39
@akx
Copy link
Contributor Author

akx commented Feb 1, 2024

Should be good to go again, I gave the failing test a spin locally (which took some editing because the tests don't run OOTB on mps devices, but there we go). 👍

@younesbelkada
Copy link
Contributor

Thanks very much @akx - just re-ran the CI

Copy link
Contributor

@younesbelkada younesbelkada left a comment

Choose a reason for hiding this comment

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

Huge work ! Thanks !

@younesbelkada younesbelkada merged commit ae8431b into huggingface:main Feb 1, 2024
9 checks passed
lapp0 pushed a commit to lapp0/trl that referenced this pull request May 10, 2024
* Remove stray commas from test data

* Codemod Unittest assertions to bare asserts

* Make `assertAlmostEqual` tests more idiomatic

* DRY some test strings
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.

3 participants