ao icon indicating copy to clipboard operation
ao copied to clipboard

Testing subclassing

Open Krishn1412 opened this issue 6 months ago • 5 comments

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.

Krishn1412 avatar Oct 10 '25 21:10 Krishn1412

:link: Helpful Links

:test_tube: 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.

: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.

pytorch-bot[bot] avatar Oct 10 '25 21:10 pytorch-bot[bot]

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?

Krishn1412 avatar Oct 10 '25 21:10 Krishn1412

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 avatar Oct 10 '25 22:10 jerryzh168

@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?

Krishn1412 avatar Oct 12 '25 12:10 Krishn1412

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

jerryzh168 avatar Nov 24 '25 22:11 jerryzh168