Cast on host instead of IPU when using `precision=16` #13880

hmellor · 2022-07-27T13:27:23Z

What does this PR do?

Removes the cast to half from the forward pass and adds it to the host side pre-processing.

In overloading batch_to_device it also prevents data.to(device), which is not currently necessary for IPU.

Fixes #13828

Does your PR introduce any breaking changes? If yes, please list them.

Before submitting

Was this discussed/approved via a GitHub issue? (not for typos and docs)
Did you read the contributor guideline, Pull Request section?
Did you make sure your PR does only one thing, instead of bundling different changes together?
Did you make sure to update the documentation with your changes? (if necessary)
Did you write any new necessary tests? (not for typos and docs)
Did you verify new and existing tests pass locally with your changes?
Did you list all the breaking changes introduced by this pull request?
Did you update the CHANGELOG? (not for typos, docs, test updates, or minor internal changes/refactors)

PR review

Anyone in the community is welcome to review the PR.
Before you start reviewing, make sure you have read the review guidelines. In short, see the following bullet-list:

Is this pull request ready for review? (if not, please submit in draft mode)
Check that all items from Before submitting are resolved
Make sure the title is self-explanatory and the description concisely explains the PR
Add labels and milestones (and optionally projects) to the PR so it can be classified

Did you have fun?

Make sure you had fun coding 🙃

for more information, see https://pre-commit.ci

src/pytorch_lightning/strategies/ipu.py

for more information, see https://pre-commit.ci

src/pytorch_lightning/strategies/ipu.py

rohitgr7 · 2022-07-28T09:24:25Z

src/pytorch_lightning/strategies/ipu.py

+        # We don't call `super().batch_to_device` because `data.to(device)` is not
+        # currently necessary for IPUs. The movement of data from host<->IPU is
+        # currently handled by PopTorch.


if that's the case, we need to do a follow-up for users to let them know that some of the hooks won't work with IPUs, like transfer_batch_to_device, ...

Is this something you could do? I don't know the codebase well enough to know all the implications.

codecov · 2022-07-28T09:43:39Z

Codecov Report

Merging #13880 (b9ca3d9) into master (b37e466) will increase coverage by 26%.
The diff coverage is 8%.

❗ Current head b9ca3d9 differs from pull request most recent head daf9949. Consider uploading reports for the commit daf9949 to get more accurate results

@@            Coverage Diff            @@
##           master   #13880     +/-   ##
=========================================
+ Coverage      49%      75%    +26%     
=========================================
  Files         332      332             
  Lines       26010    26810    +800     
=========================================
+ Hits        12728    20078   +7350     
+ Misses      13282     6732   -6550

for more information, see https://pre-commit.ci

awaelchli

Thank you for working on this!

src/pytorch_lightning/strategies/ipu.py

tests/tests_pytorch/deprecated_api/test_remove_2-0.py

tests/tests_pytorch/accelerators/test_ipu.py

src/pytorch_lightning/CHANGELOG.md

tests/tests_pytorch/deprecated_api/test_remove_2-0.py

rohitgr7

LGTM

rohitgr7 · 2022-07-28T18:06:39Z

src/pytorch_lightning/strategies/ipu.py

+        # This override is necessary because the cast must occur before the data
+        # is moved to the device to prevent wasteful host->device copies.
+        if self.precision_plugin.precision in (PrecisionType.MIXED, PrecisionType.HALF):
+            batch = apply_to_collection(batch, (FloatTensor, torch.cuda.FloatTensor), function=Tensor.half)


Suggested change

batch = apply_to_collection(batch, (FloatTensor, torch.cuda.FloatTensor), function=Tensor.half)

batch = apply_to_collection(batch, Tensor, function=Tensor.half)

if we need to move all kinds of tensors for IPUs

Is this the change that caused #13983?

that's part of it, but before e.g. DoubleTensors, BfloatTensors etc. wouldn't have been cast as well.

tests/tests_pytorch/deprecated_api/test_remove_1-8.py

src/pytorch_lightning/strategies/ipu.py

Co-authored-by: Rohit Gupta <[email protected]>

Move half cast to occur on host instead of in fwd

c6a47dd

hmellor requested review from tchaton, awaelchli, justusschock, kaushikb11, williamFalcon, Borda, carmocca and rohitgr7 as code owners July 27, 2022 13:27

github-actions bot added the pl Generic label for PyTorch Lightning package label Jul 27, 2022

hmellor and others added 2 commits July 27, 2022 14:28

Update changelog

435d03f

[pre-commit.ci] auto fixes from pre-commit.com hooks

8379f01

for more information, see https://pre-commit.ci

rohitgr7 reviewed Jul 27, 2022

View reviewed changes

src/pytorch_lightning/strategies/ipu.py Outdated Show resolved Hide resolved

rohitgr7 reviewed Jul 27, 2022

View reviewed changes

awaelchli added strategy: ipu (removed) bug Something isn't working labels Jul 27, 2022

hmellor and others added 4 commits July 28, 2022 09:47

batch_to -> to_half, now local to batch_to_device

423d63a

[pre-commit.ci] auto fixes from pre-commit.com hooks

28016af

for more information, see https://pre-commit.ci

Add comments to overloaded function

2cd738d

Remove LightningIPUModule

b9ca3d9

rohitgr7 reviewed Jul 28, 2022

View reviewed changes

hmellor and others added 3 commits July 28, 2022 11:13

Validate precision in IPU tests

daf9949

Deprecate LightningIPUModel

300056e

[pre-commit.ci] auto fixes from pre-commit.com hooks

ee6a4cf

for more information, see https://pre-commit.ci

awaelchli approved these changes Jul 28, 2022

View reviewed changes

src/pytorch_lightning/strategies/ipu.py Outdated Show resolved Hide resolved

tests/tests_pytorch/deprecated_api/test_remove_2-0.py Outdated Show resolved Hide resolved

tests/tests_pytorch/accelerators/test_ipu.py Outdated Show resolved Hide resolved

carmocca reviewed Jul 28, 2022

View reviewed changes

src/pytorch_lightning/CHANGELOG.md Outdated Show resolved Hide resolved

tests/tests_pytorch/deprecated_api/test_remove_2-0.py Outdated Show resolved Hide resolved

carmocca added this to the pl:1.6.x milestone Jul 28, 2022

hmellor added 2 commits July 28, 2022 18:22

Update deprecation timeline

de82e00

Moved changelog entry to fixed

bb45874

Minor fixes

5ffbec0

carmocca approved these changes Jul 28, 2022

View reviewed changes

carmocca requested a review from rohitgr7 July 28, 2022 17:28

mergify bot added the ready PRs ready to be merged label Jul 28, 2022

carmocca added 2 commits July 28, 2022 19:30

Refactor

a5b1883

Merge branch 'master' into cast-before-move

ebe8c2b

carmocca enabled auto-merge (squash) July 28, 2022 17:31

rohitgr7 approved these changes Jul 28, 2022

View reviewed changes

Update test

9f303a4

auto-merge was automatically disabled July 28, 2022 18:42
Head branch was pushed to by a user without write access

hmellor and others added 3 commits July 28, 2022 19:43

Update src/pytorch_lightning/strategies/ipu.py

a9f12e9

Co-authored-by: Rohit Gupta <[email protected]>

Update tests/tests_pytorch/deprecated_api/test_remove_1-8.py

af485fa

Co-authored-by: Rohit Gupta <[email protected]>

Update src/pytorch_lightning/strategies/ipu.py

87d1426

Co-authored-by: Rohit Gupta <[email protected]>

rohitgr7 enabled auto-merge (squash) July 28, 2022 18:46

rohitgr7 merged commit 07b39c2 into Lightning-AI:master Jul 28, 2022

carmocca mentioned this pull request Jul 28, 2022

Warn IPU users that batch transfering hooks will not run #13916

Closed

hmellor deleted the cast-before-move branch August 26, 2022 15:09

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Cast on host instead of IPU when using `precision=16` #13880

Cast on host instead of IPU when using `precision=16` #13880

hmellor commented Jul 27, 2022 •

edited

Loading

rohitgr7 Jul 28, 2022

hmellor Jul 28, 2022

codecov bot commented Jul 28, 2022 •

edited

Loading

awaelchli left a comment

rohitgr7 left a comment

rohitgr7 Jul 28, 2022

carmocca Aug 3, 2022

justusschock Aug 3, 2022

	batch = apply_to_collection(batch, (FloatTensor, torch.cuda.FloatTensor), function=Tensor.half)
	batch = apply_to_collection(batch, Tensor, function=Tensor.half)

Cast on host instead of IPU when using precision=16 #13880

Cast on host instead of IPU when using precision=16 #13880

Conversation

hmellor commented Jul 27, 2022 • edited Loading

What does this PR do?

Does your PR introduce any breaking changes? If yes, please list them.

Before submitting

PR review

Did you have fun?

rohitgr7 Jul 28, 2022

Choose a reason for hiding this comment

hmellor Jul 28, 2022

Choose a reason for hiding this comment

codecov bot commented Jul 28, 2022 • edited Loading

Codecov Report

awaelchli left a comment

Choose a reason for hiding this comment

rohitgr7 left a comment

Choose a reason for hiding this comment

rohitgr7 Jul 28, 2022

Choose a reason for hiding this comment

carmocca Aug 3, 2022

Choose a reason for hiding this comment

justusschock Aug 3, 2022

Choose a reason for hiding this comment

Cast on host instead of IPU when using `precision=16` #13880

Cast on host instead of IPU when using `precision=16` #13880

hmellor commented Jul 27, 2022 •

edited

Loading

codecov bot commented Jul 28, 2022 •

edited

Loading