move example inputs to correct device when tracing module #4360

NumesSanguis · 2020-10-26T05:46:59Z

Continuation of pull request (which has been merged): #4142
This pull request addresses issues raised in the previous pull request.

These pull request address the original feature request: #4140

…Tensor; not supported log error when example_inputs is a dict; commented docstring trace example

pep8speaks · 2020-10-26T05:47:03Z

Hello @NumesSanguis! Thanks for updating this PR.

There are currently no PEP 8 issues detected in this Pull Request. Cheers! 🍻

Comment last updated at 2020-10-29 05:20:59 UTC

NumesSanguis · 2020-10-26T05:49:23Z

This pull request has a PEP8 issue. I don't know how to do a multi-line version of (line 1580):

>>> torch.jit.save(model.to_torchscript(method='trace', example_inputs=torch.randn(1, 64)), "model_trace.pt")  # doctest: +SKIP

Please let me know how to do this.

codecov · 2020-10-26T06:04:34Z

Codecov Report

Merging #4360 into master will increase coverage by 0%.
The diff coverage is 100%.

@@          Coverage Diff           @@
##           master   #4360   +/-   ##
======================================
  Coverage      93%     93%           
======================================
  Files         111     111           
  Lines        8092    8127   +35     
======================================
+ Hits         7500    7547   +47     
+ Misses        592     580   -12

justusschock

LGTM. Some minor comments though

pytorch_lightning/core/lightning.py

justusschock · 2020-10-26T07:27:05Z

pytorch_lightning/core/lightning.py

@@ -1591,8 +1594,12 @@ def to_torchscript(
                # if no example inputs are provided, try to see if model has example_input_array set
                if example_inputs is None:
                    example_inputs = self.example_input_array
+                # dicts are not supported, so show the user an error; not raising an error to show the original error
+                if type(example_inputs) == dict:
+                    log.error(f"`example_inputs` should be a Tensor or a tuple of Tensors, but got a dict.")


Suggested change

log.error(f"`example_inputs` should be a Tensor or a tuple of Tensors, but got a dict.")

raise TypeError(f"`example_inputs` should be a Tensor or a tuple of Tensors, but got a dict.")

@justusschock I on purpose used log.error, because the blocker should not be to_torchscript, but the logic should stay at torch.jit.trace's side.

Say for example that TorchScript is updated at PyTorch's side (torch.jit.trace() does now accept a dict). Now the to_torchscript() function can actually support a dict, but Lightning is unnecessarily blocking it. Also, the original error might be more insightful than just a "dict is not accepted".

log.error still gives this little bit of extra information to the user, but only acts as a friendly informer, instead of taking over the guard position.
If torch.jit.trace() does accept a dict in an update, we only have a nagging error, but not a showstopper.

I see. However, currently this is a showstopper and we should treat it like that imo.

if this isn't any longer in the future, we can simply remove this.

Also I'd wonder why we need it at all, if we say, we let torchscript handle this...

@justusschock This came forth from the discussion with @awaelchli here: #4142 (comment)

I see. However, currently this is a showstopper and we should treat it like that imo.

If the person reads the docs (generated from example_inputs: Optional[Union[torch.Tensor, Tuple[torch.Tensor]]]), it should already be clear that a dict should not be used. So I would argue it's not a show stopper, just something that can be encountered depending on how self.example_input_array is set.

Also I'd wonder why we need it at all, if we say, we let torchscript handle this...

It could be removed, but since in some cases self.example_input_array is set to a dict, it's just a nice heads-up to the user if this problem is encountered. Seeing this log.error just before the beginning of the Traceback is just a bit more user-friendly.

NumesSanguis · 2020-10-26T07:52:20Z

@justusschock Do you know how to solve my PEP8 issue I commented about above? Then I'll add that together with the fix(es) in your comments.

justusschock · 2020-10-26T07:58:06Z

@NumesSanguis what about

>>> torch.jit.save(
        model.to_torchscript(method="trace", example_inputs=torch.randn(1, 64)), "model_trace.pt"
    )  # doctest: +SKIP

?

Co-authored-by: Justus Schock <[email protected]>

NumesSanguis · 2020-10-26T08:21:08Z

@NumesSanguis what about

>>> torch.jit.save(
        model.to_torchscript(method="trace", example_inputs=torch.randn(1, 64)), "model_trace.pt"
    )  # doctest: +SKIP

?

@justusschock My IDE (PyCharm) didn't show correct coloring for writing it like that, but I updated my repo with your code lines. Let's see if the checks pass.

NumesSanguis · 2020-10-26T08:41:01Z

@justusschock Seems PyCharm was right, because the checks throw this error:

 1580             >>> torch.jit.save(
UNEXPECTED EXCEPTION: SyntaxError('unexpected EOF while parsing', ('<doctest pytorch_lightning.core.lightning.LightningModule.to_torchscript[4]>', 1, 16, 'torch.jit.save(\n'))
Traceback (most recent call last):

  File "/opt/hostedtoolcache/Python/3.6.12/x64/lib/python3.6/doctest.py", line 1330, in __run
    compileflags, 1), test.globs)

  File "<doctest pytorch_lightning.core.lightning.LightningModule.to_torchscript[4]>", line 1

    torch.jit.save(

                  ^

SyntaxError: unexpected EOF while parsing

Should I just remove this docstring test? Or allow for a PEP8 here?

justusschock · 2020-10-26T08:50:15Z

@Borda are you familiar with multiline docchecks?

NumesSanguis · 2020-10-26T08:53:52Z

I undid the commit with the following lines using a rebase.

>>> torch.jit.save(
        model.to_torchscript(method="trace", example_inputs=torch.randn(1, 64)), "model_trace.pt"
    )  # doctest: +SKIP

Instead I commented it as multi-line to stop the PEP8 error. Once I know how to do a multi-line docstring code example, I can undo this last PEP8 fix commit and fix it properly.

NumesSanguis · 2020-10-26T09:17:56Z

@justusschock It seems the secret was ...:

>>> torch.jit.save(model.to_torchscript(file_path="model_trace.pt", method='trace', # doctest: +SKIP
...                                     example_inputs=torch.randn(1, 64)))  # doctest: +SKIP

NumesSanguis · 2020-10-27T04:28:01Z

If the current use of log.error is acceptable, this pull request can be merged.

awaelchli

let's add a changelog message?

awaelchli · 2020-10-27T08:32:52Z

pytorch_lightning/core/lightning.py

@@ -1591,8 +1595,13 @@ def to_torchscript(
                # if no example inputs are provided, try to see if model has example_input_array set
                if example_inputs is None:
                    example_inputs = self.example_input_array
+                # dicts are not supported, so show the user an error; not raising an error to show the original error


I don't understand this comment, it is saying there is an error but there is not an error, what is it?
can we just remove the comment? the code should speak for itself

the inputs must be a tensor or tuple of tensors. IMO a better way to handle this is by wrapping the input tensor into a tuple and checking whether each element in the tuple is an instance of torch.Tensor or not.

Also with a test for the same.

@awaelchli good catch, that doesn't make sense indeed.
@rohitgr7 I think trace() already does internally this wrapping of a torch.Tensor in a tuple, so I don't think we have to add that again on Lightning's side?

I changed the comment to hopefully make more sense

give me sometime. Need to check what's the actual issue here. Is there something wrong from pytorch side or we are doing something wrong here? In the meantime can you open an issue on pytorch forums if possible? Maybe we can get a quick response there :) Would be good to resolve all issues in this PR itself to avoid any issues in the future related to to_torchscript. Also will make similar changes to to_onnx #4378.

This issue is already in the master, since this pull request was already merged: #4140
This pull request is just to add some quality of life changes to the previous one. If we merge this one, it's much easier for other people to reproduce this issue, because they will have the same error output (this pull request does not add a new problem, just 1 step closer to solving it).

We can just keep the original issue (#4140) open, and discuss this issue there, as it would be more easy to find compared to this comment thread. Then we can point a PyTorch forum issue to there. A new pull request can then target that specific dict improvement (which might be very deep), instead of making this PR huge.

Honestly, I would like to make all parts work nicely, but I'm not affected by the dict issue, and I already spend too much time on this pull request. The previous pull request already added everything needed for my use case, but this pull request is just an extra to make the previous one a little bit less rough.

Ok cool. Then let's remove the check for Mapping and merge this one since it doesn't throw any error with dict :)

@rohitgr7 Thanks. The logger error has been removed :)

@rohitgr7 I put a summary of the Dict issue here: #4140 (comment)
which should make the discussion a bit more visible for others.

pytorch_lightning/core/lightning.py

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

pytorch_lightning/core/lightning.py

Co-authored-by: Jeff Yang <[email protected]>

…trace

* use move_data_to_device instead of to; docstring also allow tuple of Tensor; not supported log error when example_inputs is a dict; commented docstring trace example * Use isinstance to check if example_inputs is a Mapping, instead of type Co-authored-by: Justus Schock <[email protected]> * import Mapping for isinstance check * multi-line docstring code to test TorchScript trace() * Fix PEP8 f-string is missing placeholders * minor code style improvements * Use (possibly user overwritten) transfer_batch_to_device instead of move_data_to_device Co-authored-by: Rohit Gupta <[email protected]> * fixed weird comment about trace() log error * Remove unused import Co-authored-by: Jeff Yang <[email protected]> * Remove logger warning about dict not example_inputs not supported by trace Co-authored-by: stef-ubuntu <[email protected]> Co-authored-by: Justus Schock <[email protected]> Co-authored-by: Adrian Wälchli <[email protected]> Co-authored-by: Rohit Gupta <[email protected]> Co-authored-by: Jeff Yang <[email protected]> (cherry picked from commit 9cfd299)

use move_data_to_device instead of to; docstring also allow tuple of …

cbbf04f

…Tensor; not supported log error when example_inputs is a dict; commented docstring trace example

NumesSanguis requested review from ananyahjha93, awaelchli, Borda, justusschock, nateraw, SeanNaren, tchaton, teddykoker and williamFalcon as code owners October 26, 2020 05:46

mergify bot requested a review from a team October 26, 2020 05:47

NumesSanguis mentioned this pull request Oct 26, 2020

Add trace functionality to the function to_torchscript #4142

Merged

7 tasks

justusschock approved these changes Oct 26, 2020

View reviewed changes

mergify bot requested a review from a team October 26, 2020 07:28

Use isinstance to check if example_inputs is a Mapping, instead of type

fb72416

Co-authored-by: Justus Schock <[email protected]>

import Mapping for isinstance check

8421c5e

stefempath added 2 commits October 26, 2020 18:20

multi-line docstring code to test TorchScript trace()

8cb11da

Fix PEP8 f-string is missing placeholders

95c00b7

awaelchli approved these changes Oct 27, 2020

View reviewed changes

mergify bot requested a review from a team October 27, 2020 08:40

awaelchli changed the title ~~Quality of life changes to previous TorchScript trace merged pull request~~ move example inputs to correct device when tracing module Oct 27, 2020

minor code style improvements

f5f7cf3

awaelchli added this to the 1.0.x milestone Oct 27, 2020

awaelchli added feature Is an improvement or enhancement torchscript labels Oct 27, 2020

rohitgr7 reviewed Oct 27, 2020

View reviewed changes

pytorch_lightning/core/lightning.py Outdated Show resolved Hide resolved

mergify bot requested a review from a team October 27, 2020 08:49

NumesSanguis and others added 3 commits October 27, 2020 18:25

Use (possibly user overwritten) transfer_batch_to_device instead of m…

7e769e8

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

Merge branch 'master' into master

0c71dba

fixed weird comment about trace() log error

c1fc753

ydcjeff reviewed Oct 27, 2020

View reviewed changes

pytorch_lightning/core/lightning.py Outdated Show resolved Hide resolved

mergify bot requested a review from a team October 27, 2020 13:00

NumesSanguis and others added 2 commits October 28, 2020 10:08

Remove unused import

d3da5f1

Co-authored-by: Jeff Yang <[email protected]>

Merge branch 'master' into master

8c82c58

s-rog approved these changes Oct 28, 2020

View reviewed changes

Merge branch 'master' into master

5b74cd3

rohitgr7 approved these changes Oct 29, 2020

View reviewed changes

rohitgr7 and others added 2 commits October 29, 2020 10:47

Merge branch 'master' into master

8bec5b7

Remove logger warning about dict not example_inputs not supported by …

a8c7e15

…trace

rohitgr7 merged commit 9cfd299 into Lightning-AI:master Oct 29, 2020

NumesSanguis mentioned this pull request Oct 29, 2020

Expand to_torchscript to support also TorchScript's trace method #4140

Closed

edenlightning added the hacktoberfest-accepted label Oct 30, 2020

This was referenced Nov 6, 2020

update changelog after 1.0.5 #4505

Merged

Missing TorchScript trace's update #4586

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

move example inputs to correct device when tracing module #4360

move example inputs to correct device when tracing module #4360

NumesSanguis commented Oct 26, 2020

pep8speaks commented Oct 26, 2020 •

edited

Loading

NumesSanguis commented Oct 26, 2020

codecov bot commented Oct 26, 2020 •

edited

Loading

justusschock left a comment

justusschock Oct 26, 2020

NumesSanguis Oct 26, 2020

justusschock Oct 26, 2020

NumesSanguis Oct 26, 2020 •

edited

Loading

NumesSanguis commented Oct 26, 2020

justusschock commented Oct 26, 2020

NumesSanguis commented Oct 26, 2020

NumesSanguis commented Oct 26, 2020

justusschock commented Oct 26, 2020

NumesSanguis commented Oct 26, 2020

NumesSanguis commented Oct 26, 2020 •

edited

Loading

NumesSanguis commented Oct 27, 2020

awaelchli left a comment

awaelchli Oct 27, 2020

rohitgr7 Oct 27, 2020 •

edited

Loading

rohitgr7 Oct 27, 2020

NumesSanguis Oct 27, 2020

NumesSanguis Oct 27, 2020

rohitgr7 Oct 28, 2020 •

edited

Loading

NumesSanguis Oct 29, 2020

rohitgr7 Oct 29, 2020

NumesSanguis Oct 29, 2020

NumesSanguis Oct 29, 2020

	log.error(f"`example_inputs` should be a Tensor or a tuple of Tensors, but got a dict.")
	raise TypeError(f"`example_inputs` should be a Tensor or a tuple of Tensors, but got a dict.")

move example inputs to correct device when tracing module #4360

move example inputs to correct device when tracing module #4360

Conversation

NumesSanguis commented Oct 26, 2020

pep8speaks commented Oct 26, 2020 • edited Loading

Comment last updated at 2020-10-29 05:20:59 UTC

NumesSanguis commented Oct 26, 2020

codecov bot commented Oct 26, 2020 • edited Loading

Codecov Report

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

NumesSanguis Oct 26, 2020 • edited Loading

Choose a reason for hiding this comment

NumesSanguis commented Oct 26, 2020

justusschock commented Oct 26, 2020

NumesSanguis commented Oct 26, 2020

NumesSanguis commented Oct 26, 2020

justusschock commented Oct 26, 2020

NumesSanguis commented Oct 26, 2020

NumesSanguis commented Oct 26, 2020 • edited Loading

NumesSanguis commented Oct 27, 2020

awaelchli left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

rohitgr7 Oct 27, 2020 • 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

rohitgr7 Oct 28, 2020 • 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

Choose a reason for hiding this comment

pep8speaks commented Oct 26, 2020 •

edited

Loading

codecov bot commented Oct 26, 2020 •

edited

Loading

NumesSanguis Oct 26, 2020 •

edited

Loading

NumesSanguis commented Oct 26, 2020 •

edited

Loading

rohitgr7 Oct 27, 2020 •

edited

Loading

rohitgr7 Oct 28, 2020 •

edited

Loading