Testing subclassing
Added two tests to check if the subclassing logic defined in TorchAOBaseTensor _init_subclass _ works correctly. Refer the comment.
The first test fails for:
self.assertIsNot(Parent._ATEN_OP_TABLE, Child._ATEN_OP_TABLE)
self.assertIsNot(Parent._TORCH_FN_TABLE, Child._TORCH_FN_TABLE)
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.
:link: Helpful Links
:test_tube: See artifacts and rendered test results at hud.pytorch.org/pr/pytorch/ao/3153
- :page_facing_up: Preview Python docs built from this PR
Note: Links to docs will display an error until the docs builds have been completed.
:heavy_exclamation_mark: 1 Active SEVs
There 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?
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?
yeah, each sublcass should have their own dict, I think the current impl is problematic
@jerryzh168 ,
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'.
Should we start working on changing the implementation?
sorry I missed the comment
the new test with real op fails for the same line self.assertIsNot(Parent._ATEN_OP_TABLE, Child._ATEN_OP_TABLE).
yeah we need to make sure parent and child classes are not sharing the table, we should fix this
For the inheritance test case,
yeah seems reasonable to me as well, please feel free to start changing this