Skip to content

Adds TRANSFORMERS_TEST_DEVICE#25506

Merged
sgugger merged 5 commits intohuggingface:mainfrom
vvvm23:transformers-test-device
Aug 17, 2023
Merged

Adds TRANSFORMERS_TEST_DEVICE#25506
sgugger merged 5 commits intohuggingface:mainfrom
vvvm23:transformers-test-device

Conversation

@vvvm23
Copy link
Contributor

@vvvm23 vvvm23 commented Aug 14, 2023

Adds support for environment variable TRANSFORMERS_TEST_DEVICE to set device in use for running the test suite. This pattern is already in use in diffusers.

What does this PR do?

Adds support for environment variable TRANSFORMERS_TEST_DEVICE to set device in use for running the test suite. This is a pattern already in use in diffusers which would be useful to have in transformers.

Additionally, I would like to propose removing the check on available backends, as is found in the diffusers version. I included it here to match diffusers, but it would be useful to remove it (for example, if testing new backends and the like). Let me know if that is okay and I will amend the PR.

Before submitting

  • This PR fixes a typo or improves the docs (you can dismiss the other checks if that's the case).
  • Did you read the contributor guideline,
    Pull Request section?
  • Was this discussed/approved via a Github issue or the forum? Please add a link
    to it if that's the case.
  • Did you make sure to update the documentation with your changes? Here are the
    documentation guidelines, and
    here are tips on formatting docstrings.
  • Did you write any new necessary tests?

Who can review?

@sgugger, git blame says you! 🤗

Mirrors the same API in the diffusers library. Useful in transformers
too.
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 your PR! Instead of doing a test on backends which may or may not be available, it would be better to just try to do torch.device with the device found, to see if it errors or not.

@vvvm23
Copy link
Contributor Author

vvvm23 commented Aug 14, 2023

Hi thanks, it is a good suggestion to simply try creating the device. Perhaps diffusers can be updated to do the same?

Doing this approach makes the list of backends obsolete, so I removed. I just let it throw an unhandled error as I feel the message is informative enough, but let me know if you want that to be caught.

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 iterating! One last nit and we should be good to merge!

@vvvm23
Copy link
Contributor Author

vvvm23 commented Aug 16, 2023

Updated the error message, let me know if it works for you~

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 a lot! Can you just run make style on your branch to fix the quality issue?

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.

Could you also add some documentation in testing.md to not make it hidden?

@vvvm23
Copy link
Contributor Author

vvvm23 commented Aug 17, 2023

Thanks a lot! Can you just run make style on your branch to fix the quality issue?

Woops, I ran this but forgot to commit the changes.

Could you also add some documentation in testing.md to not make it hidden?

Will do 👍 where do you think the best place is this for this in the file?

I should say, to all the suggested changes here, should these also be mirrored in diffusers? My initial PR was basically a direct copy from there.

@ArthurZucker
Copy link
Collaborator

You can probably add it to docs/source/en/testing.md.
Not sure about diffusers but documenting a new env variable should always be good 😉

@HuggingFaceDocBuilderDev

The docs for this PR live here. All of your documentation changes will be reflected on that endpoint.

@vvvm23
Copy link
Contributor Author

vvvm23 commented Aug 17, 2023

@ArthurZucker added a small section following the "To GPU or not to GPU" section – in my head this fits well. Let me know how you feel about the language there 🤗 I ran make style also which I believe also formats the docs.

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.

Thanks! Just waiting for the CIs

@sgugger sgugger merged commit 1791ef8 into huggingface:main Aug 17, 2023
@vvvm23 vvvm23 deleted the transformers-test-device branch August 17, 2023 12:27
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