Pytorch-lightning: AttributeError: 'xxx' object has no attribute 'tng_dataloader' continued...

[sebftw]

This is the same issue as mentioned in https://github.com/williamFalcon/pytorch-lightning/issues/125.

Describe the bug
Whenever a @property raises an AttributeError, it will be looked up in __getattr__ (if defined), and then the original error message is lost. And LightingModule inherits __getattr__ from torch.nn.Module.

This makes debugging difficult as you lose line number and you get an error message that is misleading - the attribute exists! Look at the minimal example in the next section to see what I mean.

To Reproduce

import torch

class Foo(torch.nn.Module):
    @property
    def bar(self):
        return torch.does_not_exist  # Raises an AttributeError.

Foo().bar

The output produced:

Traceback (most recent call last):
  File "C:/minimal.py", line 8, in <module>
    Foo().bar
  File "C:\Program Files\Python37\lib\site-packages\torch\nn\modules\module.py", line 591, in __getattr__
    type(self).__name__, name))
AttributeError: 'Foo' object has no attribute 'bar'

The trace says the bar attribute does not exist, but it does. It is the does_not_exist attribute that is nonexistent. Now if you look in the stack trace in https://github.com/williamFalcon/pytorch-lightning/issues/125 it is even worse.

Expected behavior
One would expect the stack trace to be

Traceback (most recent call last):
  File "C:/minimal.py", line 7, in bar
    return torch.does_not_exist  # Raises an AttributeError.
AttributeError: module 'torch' has no attribute 'does_not_exist'

I think it could be very useful to add try...catch to the pl.data_loader.
One could also make a pl.attribute decorator that users (and pl.data_loader) can use, and document its use case in the docs. It should simply extend the usual @attribute with a try...catch.
An example of a more desirable stack trace

Traceback (most recent call last):
  File "C:/minimal.py", line 7, in bar
    return torch.does_not_exist  # Raises an AttributeError.
AttributeError: module 'torch' has no attribute 'does_not_exist'

The above exception was the direct cause of the following exception:

Traceback (most recent call last):
  File "C:/minimal.py", line 11, in <module>
    Foo().bar
  File "C:/minimal.py", line 9, in bar
    raise RuntimeError('An AttributeError was encountered in Foo.bar.') from e
RuntimeError: An AttributeError was encountered in Foo.bar.

In this case the error is correctly shown to be at line 7.

This stack trace is achieved by replacing line 7 with:

try:
    return torch.does_not_exist  # Raises an AttributeError.
except AttributeError as e:
    raise RuntimeError('An AttributeError was encountered in Foo.bar.') from e

Additional context
See also the discussion in https://github.com/pytorch/pytorch/issues/13981 of the problem. It seems to be a troublesome design decision of the language that will be hard to get around.

Do you think @pl.attribute is a good idea?
There seem to be no other way around the issue if we must use nn.Module and @attribute.

[williamFalcon]

@sebftw read my mind haha. was about to add this bug! want to give it a shot?

[williamFalcon]

@sebftw it probably has to catch all exceptions though. the attribute doesn't exist is hiding whatever issue the dataloader code has (ie: user error), so we want to forward that to the user via catch Exception as e:

[sebftw]

@williamFalcon Yeah. I can try and look into it tomorrow. I think we only have to catch AttributeError as other exceptions will produce a normal stack trace.

[sebftw]

I have made a sketch of the implementation and how to test it.

import torch.nn as nn
import pytest

# ------------------------------------------------------------------------
# IMPLEMENTATION
# ------------------------------------------------------------------------

errormessage = 'An AttributeError was encountered: '
def safe_property(fn):
    @property
    def wrapper(*args, **kwargs):
        try:
            return fn(*args, **kwargs)
        except AttributeError as e:
            raise RuntimeError(errormessage + str(e)) from e
    return wrapper


# ------------------------------------------------------------------------
# TESTS
# ------------------------------------------------------------------------

returnexample = 1
exceptionexample = ZeroDivisionError
informative_error_message = "module 'torch.nn' has no attribute 'does_not_exist'"

class PropertyTest(nn.Module): # Note it is an nn.Module.
    @property
    def test_property(self):
        return returnexample

    @safe_property
    def test_safe_property(self):
        return returnexample

    @property
    def test_property_attributeerror(self):
        raise AttributeError(informative_error_message)
        # ^ Equivalent to: return nn.does_not_exist, which will also raise AttributeError.

    @safe_property
    def test_safe_property_attributeerror(self):
        raise AttributeError(informative_error_message)

    @property
    def test_property_error(self):
        raise exceptionexample(informative_error_message)

    @safe_property
    def test_safe_property_error(self):
        raise exceptionexample(informative_error_message)


def test_same_return():
    testobj = PropertyTest()
    # Test that they both return the same.
    assert testobj.test_property == testobj.test_safe_property

def test_same_interface():
    # This is a design decision we haven't discussed.
    # Only include if we find it important that the safe property doesn't do anything more
    #  than a normal property, i.e. has the same interface and all.
    assert dir(PropertyTest.test_property) == dir(PropertyTest.test_property)
    assert isinstance(PropertyTest.test_safe_property, type(PropertyTest.test_property))  # <class 'property'>

def test_error():
    # In this we test that normal exceptions are raised normally.
    testobj = PropertyTest()
    # We expect an exception to be raised and propagated if it is of any type other than AttributeError.
    with pytest.raises(exceptionexample) as exc_info:
        testobj.test_property_error

    # This checks that in this case the exception and message matches that of a normal property.
    with pytest.raises(exc_info.type, match=exc_info.value.args[0]):
        testobj.test_safe_property_error

def test_attributeerror():
    # In this we test that AttributeErrors are better handled by the safe_property.
    testobj = PropertyTest()
    # In the case of an AttributeError, we expect an uninformative error message from nn.Module.
    with pytest.raises(AttributeError) as exc_info:
        testobj.test_property_attributeerror

    # Check that PyTorch has indeed removed the informative message completely.
    # This is the reason we are making a safe_property, so if this test fails we may not even
    #  need the decorator we are testing here.
    assert informative_error_message not in exc_info.value.args[0]

    # This tests that safe_attribute raises an exception still containing the
    #  informative error message.
    with pytest.raises(BaseException, match=informative_error_message):
        testobj.test_safe_property_attributeerror

if __name__ == '__main__':
    pytest.main([__file__])

With this code, accessing test_safe_property_attributeerror will raise a RuntimeError with the message An AttributeError was encountered: module 'torch.nn' has no attribute 'does_not_exist'.

I am not sure how the tests are structured - I am new to using pytest - but I'll make a pull request for the safe property now.

[williamFalcon]

@sebftw let's do this as a simple try catch in the dataloader decorator. A principle of lightning is to give users less to remember.

[sebftw]

Good idea

[williamFalcon]

@sebftw any updates on this?

[sebftw]

@williamFalcon I made a PR. https://github.com/williamFalcon/pytorch-lightning/pull/161

[williamFalcon]

@sebftw thanks for the PR! merged

[metrofun]

@williamFalcon @sebftw Does it only fix the suppresion of AttributeError inside the dataloader and not the other user errors?

[sebftw]

@metrofun Yes

[metrofun]

Unless I misunderstood the context, wouldn't it be more reasonable to surface any kind of errors from the user defined code, not just the AttributeError?

[sebftw]

@metrofun Other exceptions are not supressed, so they will surface as normal. It is just AttributeError which is an odd case, so we catch it and re-raise it as a RuntimeError as our fix.

[metrofun]

@sebtfw Oh, I see, thanks for the explanation