Replaces calls to `.cuda` with `.to(torch_device)` in tests by vvvm23 · Pull Request #25571 · huggingface/transformers

vvvm23 · 2023-08-17T15:05:04Z

torch.Tensor.cuda() is a pre-0.4 solution to changing a tensor's device. It is recommended to prefer .to(...) for greater flexibility and error handling. Furthermore, this makes it more consistent with other tests (that tend to use .to(torch_device)) and ensures the correct device backend is used (if torch_device is neither cpu or cuda).

This could be the case if TRANSFORMERS_TEST_DEVICE is not cpu or cuda. See #25506.

By default, I don't think this PR should change any test behaviour, but let me know if this is misguided.

What does this PR do?

Replaces calls to torch.Tensor.cuda() with .to(torch_device) equivalents. This not only ensures consistency between different tests and their management of device, but also makes tests more flexible with regard to custom or less common PyTorch backends.

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?

This affects multiple tests an doesn't target any specific modality. However, they are all PyTorch models. @sgugger, hope you don't mind me tagging you again 🙂

`torch.Tensor.cuda()` is a pre-0.4 solution to changing a tensor's device. It is recommended to prefer `.to(...)` for greater flexibility and error handling. Furthermore, this makes it more consistent with other tests (that tend to use `.to(torch_device)`) and ensures the correct device backend is used (if `torch_device` is neither `cpu` or `cuda`).

ArthurZucker

If cuda was specified as the device, we should use cuda and not torch_device since these tests are usually meant to be ran on GPU were results can vary.

vvvm23 · 2023-08-17T15:37:04Z

Isn't this the case for a lot of other tests? They use the decorator @require_torch_gpu to skip the test if torch_device != cuda, so the tests will still only be run on GPU as they are meant to be.

ArthurZucker

Mostly talking about jukebox which does not have the require_gpu if I am not mistaken. You can just revert jukebox changes not really important.

Otherwise this does not seem to increase readability. If you can make the snippets fit in two lines would better!

ArthurZucker · 2023-08-17T15:41:06Z

tests/models/bloom/test_modeling_bloom.py

        greedy_output = model.generate(
-            input_ids["input_ids"].cuda(), attention_mask=input_ids["attention_mask"], max_length=50, do_sample=False
+            input_ids["input_ids"].to(torch_device),
+            attention_mask=input_ids["attention_mask"],
+            max_length=50,
+            do_sample=False,
        )


Can fit in two lines

Sadly not without causing the CI to fail when checking style 😓 Splitting into four lines was a direct result of calling make style

You can still do something like:

Suggested change

greedy_output = model.generate(

input_ids["input_ids"].cuda(), attention_mask=input_ids["attention_mask"], max_length=50, do_sample=False

input_ids["input_ids"].to(torch_device),

attention_mask=input_ids["attention_mask"],

max_length=50,

do_sample=False,

)

input_id, attention_mask = input_ids["input_ids"].to(torch_device), input_ids["attention_mask"]

greedy_output = model.generate(input_ids, attention_mask=attention_mask, max_length=50, do_sample=False)

vvvm23 · 2023-08-17T15:47:42Z

I added torch_device into jukebox as I didn't see why it shouldn't be there if it is in basically every other test. If there is some extra behaviour or meaning I am missing, it would be helpful to know. In any case, the modifications to Jukebox are limited to two tests: one of which is skipped entirely anyway and the other is test_fp16_slow_sampling which I noticed does not have the require_torch_gpu decorator – only the slow decorator. I'll add one if you think it is worth it here 🙂

ArthurZucker

Just on last nit on the formatting.

ArthurZucker · 2023-08-18T06:11:54Z

tests/models/bloom/test_modeling_bloom.py

        greedy_output = model.generate(
-            input_ids["input_ids"].cuda(), attention_mask=input_ids["attention_mask"], max_length=50, do_sample=False
+            input_ids["input_ids"].to(torch_device),
+            attention_mask=input_ids["attention_mask"],
+            max_length=50,
+            do_sample=False,
        )


You can still do something like:

Suggested change

greedy_output = model.generate(

input_ids["input_ids"].cuda(), attention_mask=input_ids["attention_mask"], max_length=50, do_sample=False

input_ids["input_ids"].to(torch_device),

attention_mask=input_ids["attention_mask"],

max_length=50,

do_sample=False,

)

input_id, attention_mask = input_ids["input_ids"].to(torch_device), input_ids["attention_mask"]

greedy_output = model.generate(input_ids, attention_mask=attention_mask, max_length=50, do_sample=False)

tests/models/jukebox/test_modeling_jukebox.py

vvvm23 · 2023-08-18T08:39:32Z

Nice suggestions, I misunderstood what you meant initially by splitting into two lines. Hope it is all good now~

tests/models/bloom/test_modeling_bloom.py

HuggingFaceDocBuilderDev · 2023-08-18T09:21:23Z

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

tests/models/bloom/test_modeling_bloom.py

Co-authored-by: Arthur <48595927+ArthurZucker@users.noreply.github.com>

vvvm23 · 2023-08-18T10:36:22Z

This should be good now 👍

ArthurZucker

Thanks

ArthurZucker reviewed Aug 17, 2023

View reviewed changes

ArthurZucker reviewed Aug 18, 2023

View reviewed changes

addressing review comments

e345174

ArthurZucker reviewed Aug 18, 2023

View reviewed changes

tests/models/bloom/test_modeling_bloom.py Outdated Show resolved Hide resolved

vvvm23 added 2 commits August 18, 2023 10:10

more formatting changes in Bloom test

f9c1f7e

make style

150b50b

ArthurZucker reviewed Aug 18, 2023

View reviewed changes

tests/models/bloom/test_modeling_bloom.py Outdated Show resolved Hide resolved

vvvm23 and others added 2 commits August 18, 2023 10:48

Update tests/models/bloom/test_modeling_bloom.py

28f9bb7

Co-authored-by: Arthur <48595927+ArthurZucker@users.noreply.github.com>

fixes style failures

e82952e

ArthurZucker approved these changes Aug 18, 2023

View reviewed changes

ArthurZucker merged commit 9d7afd2 into huggingface:main Aug 18, 2023

vvvm23 deleted the testing-remove-dotcuda branch August 18, 2023 10:41

ydshieh mentioned this pull request Aug 24, 2023

Fix failing test_batch_generation for bloom #25718

Merged

Conversation

vvvm23 commented Aug 17, 2023

What does this PR do?

Before submitting

Who can review?

Uh oh!

ArthurZucker left a comment

Choose a reason for hiding this comment

Uh oh!

vvvm23 commented Aug 17, 2023

Uh oh!

ArthurZucker left a comment

Choose a reason for hiding this comment

Uh oh!

ArthurZucker Aug 17, 2023

Choose a reason for hiding this comment

Uh oh!

vvvm23 Aug 17, 2023

Choose a reason for hiding this comment

Uh oh!

ArthurZucker Aug 18, 2023

Choose a reason for hiding this comment

Uh oh!

vvvm23 commented Aug 17, 2023

Uh oh!

ArthurZucker left a comment

Choose a reason for hiding this comment

Uh oh!

ArthurZucker Aug 18, 2023

Choose a reason for hiding this comment

Uh oh!

Uh oh!

vvvm23 commented Aug 18, 2023

Uh oh!

Uh oh!

HuggingFaceDocBuilderDev commented Aug 18, 2023

Uh oh!

Uh oh!

vvvm23 commented Aug 18, 2023

Uh oh!

ArthurZucker left a comment

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants