Skip to content

Conversation

@Chillee
Copy link
Collaborator

@Chillee Chillee commented May 21, 2019

Fixes #20495 .

Now for

        class A(torch.jit.ScriptModule):
            def __init__(self):
                super(A, self).__init__()

            @torch.jit.script_method
            def forward(self, x):
                return x + self.whatisgoingon

        class B(A):
            def __init__(self):
                super(B, self).__init__()
            @torch.jit.script_method
            def bar(self, x):
                return x * x 
        
        A()

it does

RuntimeError: 
attribute 'whatisgoingon' does not exist:
@torch.jit.script_method
def forward(self, x):
    return x + self.whatisgoingon
               ~~~~~~~~~~~~~~~~~~ <--- HERE

I added a test in test_jit.py as well.

@pytorchbot pytorchbot added oncall: jit Add this issue/PR to JIT oncall triage queue module: pybind Related to our Python bindings / interactions with other Python libraries labels May 21, 2019
Copy link
Contributor

@facebook-github-bot facebook-github-bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@Chillee has imported this pull request. If you are a Facebook employee, you can view this diff on Phabricator.

@Chillee Chillee requested a review from jamesr66a May 21, 2019 23:17
@Chillee Chillee changed the title [jit] Improve error message for attribute missing [jit] Improve error message for missing attribute May 22, 2019
Copy link
Contributor

@facebook-github-bot facebook-github-bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@Chillee has imported this pull request. If you are a Facebook employee, you can view this diff on Phabricator.

@jamesr66a
Copy link
Collaborator

Good start!

I think this new error is actually inaccurate in the case where someone has explicitly assigned a NoneType to a module attribute:

import torch

class FooBar(torch.jit.ScriptModule):
    def __init__(self, my_optional):
        super().__init__()
        self.my_optional = my_optional

    @torch.jit.script_method
    def forward(self, x):
        if self.my_optional is not None:
            return torch.neg(x) + self.my_optional
        return torch.neg(x)

x = torch.rand(3, 4)
fb = FooBar(None)
print(fb(x))
RuntimeError: 
attribute 'my_optional' does not exist:
@torch.jit.script_method
def forward(self, x):
    if self.my_optional is not None:
       ~~~~~~~ <--- HERE
        return torch.neg(x) + self.my_optional
    return torch.neg(x)

Let's see if we can put this error message in a branch on hasattr in ModuleValue::attr here:

if (py::object attr = py::getattr(py_module_, field.c_str(), py::none())) {

Then, we'll have the old message (directing the user to add the attr to __constants) when there's an explicit None attribute, but the new message when we truly don't have the attribute.

@Chillee
Copy link
Collaborator Author

Chillee commented May 22, 2019

Nice catch - didn't realize that those had the same behavior in Python.

Added your example in a test.

Copy link
Collaborator

@jamesr66a jamesr66a left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good! Let's see if the tests pass and then I'll approve

@Chillee Chillee force-pushed the jitimproveattributeerror branch from c8acadd to c70cc6f Compare May 22, 2019 19:39
@Chillee
Copy link
Collaborator Author

Chillee commented May 23, 2019

I believe the remaining failing tests are flaky/broken.

Copy link
Contributor

@facebook-github-bot facebook-github-bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@Chillee is landing this pull request. If you are a Facebook employee, you can view this diff on Phabricator.

@facebook-github-bot
Copy link
Contributor

@Chillee merged this pull request in 52d2789.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Merged module: pybind Related to our Python bindings / interactions with other Python libraries oncall: jit Add this issue/PR to JIT oncall triage queue

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Improve attribute missing error message.

6 participants