KEMBAR78
Refactor all require decorators to use skipUnless when possible by muellerzr · Pull Request #16999 · huggingface/transformers · GitHub
Skip to content

Conversation

@muellerzr
Copy link
Contributor

What does this PR do?

I was refactoring the Accelerate tests today and I noticed we use if .. else.. as a conditional for skipping tests based on imports. Unittest has skipUnless and skipIf, letting us simplify those decorators to be one line.

E.g.:

    if not _run_slow_tests:
        return unittest.skip("test is slow")(test_case)
    else:
        return test_case

Can be:

    return unittest.skipUnless(_run_slow_tests, "test is slow")(test_case)

(Adding you as a reviewer for this Sylvain, unsure who else should be added)

@muellerzr muellerzr added Tests Related to tests cleanup labels Apr 28, 2022
@muellerzr muellerzr requested a review from sgugger April 28, 2022 18:36
@muellerzr muellerzr changed the title Update all require decorators to use skipUnless when possible Refactor all require decorators to use skipUnless when possible Apr 28, 2022
@HuggingFaceDocBuilderDev
Copy link

HuggingFaceDocBuilderDev commented Apr 28, 2022

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

Copy link
Collaborator

@sgugger sgugger left a comment

Choose a reason for hiding this comment

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

Thanks for the simplification!
Let's also wait for @LysandreJik approval.

@muellerzr muellerzr requested a review from LysandreJik April 28, 2022 19:57
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.

LGTM, pinging @ydshieh for knowledge.

@muellerzr muellerzr requested a review from ydshieh April 29, 2022 11:59
@ydshieh
Copy link
Collaborator

ydshieh commented Apr 29, 2022

Thank you 🚀 for this PR, and also for pinning me so that I can learn!

@muellerzr muellerzr merged commit 57e6464 into main Apr 29, 2022
@muellerzr muellerzr deleted the muellerzr-refactor-test-decorators branch April 29, 2022 12:55
stevhliu pushed a commit to stevhliu/transformers that referenced this pull request May 3, 2022
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

cleanup Tests Related to tests

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants