[Bug] RecursionError in NNsightModel initialization
Closed this issue · 1 comments
NNsightModel
object initialization is broken.
Code Example:
from collections import OrderedDict
from nnsight import NNsight
import torch
input_size = 5
hidden_dims = 10
output_size = 2
torch.manual_seed(423)
net = torch.nn.Sequential(
OrderedDict(
[
("layer1", torch.nn.Linear(input_size, hidden_dims)),
("layer2", torch.nn.Linear(hidden_dims, output_size))
]
)
).requires_grad_(False)
input = torch.rand((1, input_size))
model = NNsight(net)
Error:
Traceback (most recent call last):
File “<project-path>/test.py", line 22, in <module>
model = NNsight(net)
File “<env-path>/lib/python3.10/site-packages/nnsight/models/NNsightModel.py", line 51, in __init__
self._model_key = model_key
File “<env-path>/lib/python3.10/site-packages/nnsight/models/NNsightModel.py", line 307, in __setattr__
setattr(self._envoy, key, value)
File “<env-path>/lib/python3.10/site-packages/nnsight/models/NNsightModel.py", line 319, in __getattr__
return getattr(self._envoy, key)
File “<env-path>/lib/python3.10/site-packages/nnsight/models/NNsightModel.py", line 319, in __getattr__
return getattr(self._envoy, key)
File “<env-path>/lib/python3.10/site-packages/nnsight/models/NNsightModel.py", line 319, in __getattr__
return getattr(self._envoy, key)
[Previous line repeated 993 more times]
RecursionError: maximum recursion depth exceeded.
The issue seems related to this commit ae30799 . The added changes are meant to add sub-envoys using the overloaded setattr functions, but, it is being called before a parent Envoy
has been set within the NNsightModel
class, as seen here: https://github.com/ndif-team/nnsight/blob/main/src/nnsight/models/NNsightModel.py#L78.
Potential fix:
This section of the code (https://github.com/ndif-team/nnsight/blob/main/src/nnsight/models/NNsightModel.py#L305-L307) seems unnecessary because the logic is already implemented in the Envoy
class to use setattr for adding new sub-envoys. Commenting it makes the example run without producing an error, however, I am not sure if this interferes with the intended improvement of the commit related to this issue.
@AdamBelfki3 Good catch thank you! Just pushed a fix. We definitely need a base NNsight model test added to the pytests