device property #1791

Borda · 2020-05-12T08:57:09Z

What does this PR do?

PR review

Anyone in the community is free to review the PR once the tests have passed.
If we didn't discuss your PR in Github issues there's a high chance it will not be merged.

Did you have fun?

Make sure you had fun coding 🙃

awaelchli

I like it read only :)

consider also my comment here
#1790 (comment)
for better code style and flexibility

awaelchli · 2020-05-12T09:08:03Z

pytorch_lightning/trainer/distrib_data_parallel.py

-            self.device = torch.device('cuda', self.root_gpu)
+            self._device = torch.device('cuda', self.root_gpu)


we could remove all of these calls in the trainer by overloading .to() and .cuda() in LightningModule and setting the device there.

You also need to overload .cpu() :)

sorry, maybe i’m missiny something. The point of self.device is to have a readonly property to create tensors in memory directly.

If we overload the the .to() method like this for example:

def to(self, device): self._device = device return super().to(device)

Then we get the following benefits:

self.device property will not break when LightningModule is used as nn.Module without Trainer

When LightningModule is a nested LightningModule and user calls .to(), also the self.device properties of submodules get updated

The Trainer code does not need to set the device, it calls .to anyway, so the code is in one place and is easier to maintain.

I see only benefits atm

@justusschock also did it like this for metrics package

so we need to overwrite the following methods:

.to(...)

.cpu()

.cuda()
or am I missing any? @awaelchli ^^

yep, exactly, ~~although I suspect cpu and cuda already call .to internally. Not sure, need to check.~~ EDIT: nope they don't we need all three :)

well it seems to me that ideally, we want to rise the whole template from metrics...

probably not all. device.setter, dtype does not apply for LightningModule I think? I agree we should try to avoid code duplication.
@justusschock what do you think?

I think, while we do this, we should think about introducing the same for dtype, since when I create a tensor in a function, it usually involves a certain dtype as well. Although I'm not sure, if this would be reflected by amp as well...

codecov · 2020-05-12T09:31:26Z

Codecov Report

Merging #1791 into master will decrease coverage by 0%.
The diff coverage is 64%.

@@          Coverage Diff           @@
##           master   #1791   +/-   ##
======================================
- Coverage      88%     88%   -0%     
======================================
  Files          69      69           
  Lines        4316    4322    +6     
======================================
+ Hits         3805    3809    +4     
- Misses        511     513    +2

williamFalcon · 2020-05-12T10:56:22Z

yeah, good catch. This is meant as read-only.

the motivation is to support tensors on device directly.

torch.rand(..., device=self.device)

williamFalcon · 2020-05-12T10:58:07Z

pytorch_lightning/trainer/distrib_data_parallel.py

-            self.device = torch.device('cuda', self.root_gpu)
+            self._device = torch.device('cuda', self.root_gpu)


sorry, maybe i’m missiny something. The point of self.device is to have a readonly property to create tensors in memory directly.

mergify · 2020-05-12T11:23:31Z

Great job! =)

Borda · 2020-05-12T11:58:55Z

@awaelchli @justusschock I have just copy-pasted the basic template from Metrics as it contains all we need now and later we can just inherit it back... are you fine with this solution?

justusschock

You probably need to rise the corresponding tests as well :)

pytorch_lightning/core/properties.py

justusschock · 2020-05-12T12:34:28Z

Also I'd probably rename the Mixin to reflect which properties it provides to something like DeviceDtypeModuleMixin

Co-authored-by: Justus Schock <12886177+justusschock@users.noreply.github.com>

Borda · 2020-05-12T13:23:52Z

seems to fail on unrelated doctest (I have not changed Trainer)
looks like ellipses does not work...

…pytorch-lightning into feature/device

justusschock

LGTM, just one question

justusschock · 2020-05-12T13:43:11Z

pytorch_lightning/trainer/trainer.py

@@ -529,6 +529,10 @@ def __init__(
        # Callback system
        self.on_init_end()

+    @property
+    def device(self) -> Union[None, str, object]:


To me it is not clear, why the trainer should have such a property as well?

it was there before, I just made it as read-only, but agree that it is strange

I would remove it, it's not needed as far as I can tell, since now it has shifted over to the module

williamFalcon · 2020-05-12T13:58:03Z

i love how this escalated haha

Borda · 2020-05-12T21:52:57Z

it seems that there is API change in pt 1.5

    device, dtype, non_blocking = torch._C._nn._parse_to(*args, **kwargs)
ValueError: too many values to unpack (expected 3)

but it is strange that the torch master uses the same https://github.com/pytorch/pytorch/blob/master/torch/nn/modules/module.py

justusschock · 2020-05-13T13:31:11Z

@Borda Just FYI: it doesn't use the same. There is an additional argument for formatting introduced...
https://github.com/pytorch/pytorch/blob/master/torch/nn/modules/module.py#L453

Borda · 2020-05-13T13:46:42Z

@Borda Just FYI: it doesn't use the same. There is an additional argument for formatting introduced...
https://github.com/pytorch/pytorch/blob/master/torch/nn/modules/module.py#L453

yes, but pt < 1.5 has only three output vars, right...

Borda added the feature Is an improvement or enhancement label May 12, 2020

Borda requested review from williamFalcon, awaelchli, justusschock and a team May 12, 2020 08:57

awaelchli approved these changes May 12, 2020

View reviewed changes

mergify bot requested a review from a team May 12, 2020 09:09

justusschock approved these changes May 12, 2020

View reviewed changes

mergify bot requested a review from a team May 12, 2020 09:12

Borda force-pushed the feature/device branch from 57a9b75 to 9cab2f4 Compare May 12, 2020 09:42

Borda changed the title ~~device property~~ [wip] device property May 12, 2020

williamFalcon approved these changes May 12, 2020

View reviewed changes

williamFalcon changed the title ~~[wip] device property~~ device property May 12, 2020

williamFalcon changed the title ~~device property~~ [wip] device property May 12, 2020

Borda changed the title ~~[wip] device property~~ device property May 12, 2020

justusschock requested changes May 12, 2020

View reviewed changes

justusschock reviewed May 12, 2020

View reviewed changes

pytorch_lightning/core/properties.py Show resolved Hide resolved

mergify bot requested a review from a team May 12, 2020 12:54

Borda added 4 commits May 12, 2020 15:05

device property

0081a9f

add/copy properties

ce761a9

inherit

14aaace

rename

a37cd6d

Borda force-pushed the feature/device branch from 824c39f to a37cd6d Compare May 12, 2020 13:07

Apply suggestions from code review

d036491

Co-authored-by: Justus Schock <12886177+justusschock@users.noreply.github.com>

Borda added 2 commits May 12, 2020 15:27

dtype

2d51765

Merge branch 'feature/device' of https://github.com/PyTorchLightning/…

70fe31c

…pytorch-lightning into feature/device

Borda requested a review from justusschock May 12, 2020 13:32

justusschock approved these changes May 12, 2020

View reviewed changes

Borda added this to the 0.7.6 milestone May 12, 2020

Borda mentioned this pull request May 12, 2020

Add forgotten deterministic trainer flag to doctest #1805

Merged

5 tasks

prop

cd39d2d

pt api

3e81a67

williamFalcon merged commit 10ce1c0 into master May 13, 2020

Borda deleted the feature/device branch May 13, 2020 06:47

awaelchli mentioned this pull request May 16, 2020

remove obsolete self._device in Trainer #1849

Merged

5 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

device property #1791

device property #1791

Borda commented May 12, 2020

awaelchli left a comment •

edited

Loading

awaelchli May 12, 2020

justusschock May 12, 2020

williamFalcon May 12, 2020

awaelchli May 12, 2020 •

edited

Loading

awaelchli May 12, 2020

Borda May 12, 2020

awaelchli May 12, 2020 •

edited

Loading

Borda May 12, 2020

awaelchli May 12, 2020 •

edited

Loading

justusschock May 12, 2020

codecov bot commented May 12, 2020 •

edited

Loading

williamFalcon commented May 12, 2020

williamFalcon May 12, 2020

mergify bot commented May 12, 2020

Borda commented May 12, 2020

justusschock left a comment

justusschock commented May 12, 2020

Borda commented May 12, 2020 •

edited

Loading

justusschock left a comment

justusschock May 12, 2020

Borda May 12, 2020

awaelchli May 12, 2020

williamFalcon commented May 12, 2020

Borda commented May 12, 2020 •

edited

Loading

justusschock commented May 13, 2020

Borda commented May 13, 2020

		self.device = torch.device('cuda', self.root_gpu)
		self._device = torch.device('cuda', self.root_gpu)

device property #1791

device property #1791

Conversation

Borda commented May 12, 2020

What does this PR do?

PR review

Did you have fun?

awaelchli left a comment • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

awaelchli May 12, 2020 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

awaelchli May 12, 2020 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

awaelchli May 12, 2020 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

codecov bot commented May 12, 2020 • edited Loading

Codecov Report

williamFalcon commented May 12, 2020

Choose a reason for hiding this comment

mergify bot commented May 12, 2020

Borda commented May 12, 2020

justusschock left a comment

Choose a reason for hiding this comment

justusschock commented May 12, 2020

Borda commented May 12, 2020 • edited Loading

justusschock left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

williamFalcon commented May 12, 2020

Borda commented May 12, 2020 • edited Loading

justusschock commented May 13, 2020

Borda commented May 13, 2020

awaelchli left a comment •

edited

Loading

awaelchli May 12, 2020 •

edited

Loading

awaelchli May 12, 2020 •

edited

Loading

awaelchli May 12, 2020 •

edited

Loading

codecov bot commented May 12, 2020 •

edited

Loading

Borda commented May 12, 2020 •

edited

Loading

Borda commented May 12, 2020 •

edited

Loading