Fix immediate output getattr#2541
Conversation
tritolol
force-pushed
the
fix-immediate-output-getattr
branch
from
092ebff to
7eee9a9
Compare
|
@tritolol thanks for the pull request. I apologize that no one seems to have looked at this. Please rebase this change on top of tip of |
tritolol
force-pushed
the
fix-immediate-output-getattr
branch
from
600f0d8 to
5d51d4f
Compare
|
@TobyRoseman I'm not sure if you are automatically notified, so just in case you are not: The rebase is done. |
|
@TobyRoseman are any of the failing tests related to this PR? There is no indication of that for me. |
|
@tritolol - I don't think those failures are related to this change. Please rebase this change on top the latest |
tritolol
force-pushed
the
fix-immediate-output-getattr
branch
from
5d51d4f to
d0c66b1
Compare
|
@TobyRoseman rebased onto the latest |
|
Sorry @tritolol, we just updated our CI. I'm going to need you to rebase one more time on top of latest |
tritolol
force-pushed
the
fix-immediate-output-getattr
branch
from
d0c66b1 to
01918ad
Compare
|
@TobyRoseman rebased once more onto the latest |
| else: | ||
| new_nodes.append(node) | ||
|
|
||
| # check the getattr nodes not in the outputs |
There was a problem hiding this comment.
The removed code raised a RuntimeError whenever a getattr node appeared in the graph outputs, treating that as an invalid state. But that assumption is wrong — a model can legitimately return a parameter/attribute directly (e.g., return self.weight), which produces exactly this pattern. The fix replaces the hard error with correct handling: converting the output getattr node into a constant node. The getattr_nodes list was removed since it was only used to feed that error check.
|
thanks @tritolol |
2 participants
Fixes #2538.
Changed
remove_getattr_nodes()such that it adds a new constant op if it encounters a getattr op that is part of the graph's output. Before it raised an error wich caused some networks not to convert.Added a unit test for the scenario.