-
Notifications
You must be signed in to change notification settings - Fork 345
Testing subclassing #3153
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: main
Are you sure you want to change the base?
Testing subclassing #3153
Conversation
🔗 Helpful Links🧪 See artifacts and rendered test results at hud.pytorch.org/pr/pytorch/ao/3153
Note: Links to docs will display an error until the docs builds have been completed. ❗ 1 Active SEVsThere are 1 currently active SEVs. If your PR is affected, please view them below: This comment was automatically generated by Dr. CI and updates every 15 minutes. |
Is that the expected behavior @jerryzh168 ? If it is, then the if statement will work. But it does seem problematic. Shouldn't each subclass rebuilt its own copy of the dict? |
) | ||
|
||
def test_subclassing(self): | ||
class Parent(TorchAOBaseTensor): |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
the test should be doing this:
1. define Parent class
2. Parent class implements some method
# can do a real implementation or just some dummy one
@Parent.implements(aten.cat)
def _(..):
...
3. some child class inherits from parent: Child(Parent)
4. make sure the same op still works
by calling the op (e.g. aten.cat) and make sure it works (make sure it's called)
can do some additional table checks
tensor_data_names = ["qdata"] | ||
tensor_attribute_names = ["attr"] | ||
|
||
# ensure child has copied parent ops |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
yeah I think idea is the same, but just changing it to real op implementation with implements
and implements_torch_function
would be better
Parent._ATEN_OP_TABLE[Parent]["new_op"] = "added_later" | ||
self.assertNotIn("new_op", Child._ATEN_OP_TABLE[Child]) | ||
|
||
def test_multiple_inheritance(self): |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
yeah testing multiple inheritance is helpful as well, thanks
yeah, each sublcass should have their own dict, I think the current impl is problematic |
the new test with real op fails for the same line self.assertIsNot(Parent._ATEN_OP_TABLE, Child._ATEN_OP_TABLE). Also when it is called using the child tensor, it works. For the inheritance test case, it fails at self.assertEqual(C._ATEN_OP_TABLE[C]["shared"], "from_b") AssertionError: 'from_a' != 'from_b'
@jerryzh168 , Also when it is called using the child tensor, it works. For the inheritance test case, Should we start working on changing the implementation? |
Added two tests to check if the subclassing logic defined in TorchAOBaseTensor _init_subclass _ works correctly. Refer the comment.
The first test fails for:
telling us that Parent._ATEN_OP_TABLE and Child._ATEN_OP_TABLE are the same object, meaning the class attribute dictionary was shared, not re-initialized per subclass.