[Dynamo][pytree] handle isinstance(...) check for polyfilled class
#146921
Conversation
🔗 Helpful Links🧪 See artifacts and rendered test results at hud.pytorch.org/pr/146921
Note: Links to docs will display an error until the docs builds have been completed. ❌ 2 New FailuresAs of commit 72d44f2 with merge base 1677a31 ( NEW FAILURES - The following jobs have failed:
This comment was automatically generated by Dr. CI and updates every 15 minutes. |
XuehaiPan
requested review from
anijain2305,
jansel and
zou3519
and removed request for
jansel
| x = [1, [2, [3, 4]]] | ||
| leaves, treespec, _ = fn(x, y) | ||
| # Compiled function returns an instance of the polyfilled class instead of the original class | ||
| self.assertIsInstance(treespec, polyfilled_cxx_pytree.PyTreeSpec) |
There was a problem hiding this comment.
My point is, it is wrong for the compiled function to return an instance of the polyfilled class instead of the original class. The compiled function needs to return an instance of the original class.
There was a problem hiding this comment.
We either need Animesh's polyfill infra for classes, or we need to actually make a TreeSpecVariable in Dynamo that has a reconstruct method. I'd prefer hardening the polyfill infra for classes because that is more generically applicable.
There was a problem hiding this comment.
The compiled function needs to return an instance of the original class.
That is the ideal solution for polyfilling a class. We need to find a way to batch register the Python version of the polyfill methods of the C++ class. cc @anijain2305 about the class polyfill infra design.
There was a problem hiding this comment.
After a second thought, I think polyfilling the methods and using a variable tracker of instance original class during inlining will cause performance issues. Also, it is not easy to polyfill C++ descriptors and support the pybind11 property and read-only property.
The compiled function needs to return an instance of the original class.
We should use the polyfilled class object while inlining the graph and find a way to convert between the original/polyfilled class instances at the graph boundaries.
There was a problem hiding this comment.
We should use the polyfilled class object while inlining the graph and find a way to convert between the original/polyfilled class instances at the graph boundaries.
Yes
ghstack-source-id: 35cfbc4 Pull Request resolved: pytorch#146921
ghstack-source-id: 0246bf1 Pull Request resolved: pytorch#146921
ghstack-source-id: f0e34ee Pull Request resolved: pytorch#146921
|
Looks like this PR hasn't been updated in a while so we're going to go ahead and mark this as |
4 participants
Stack from ghstack (oldest at bottom):
isinstance(...)check for polyfilled class #146921isinstance(...)check for type tuple #146984Fixes https://github.com/pytorch/pytorch/pull/137398/files#r1951280557
Related:
cc @zou3519 @voznesenskym @penguinwu @EikanWang @jgong5 @Guobing-Chen @XiaobingSuper @zhuhaozhe @blzheng @wenzhe-nrv @jiayisunx @chenyang78 @kadeng @chauhang @amjames