[dynamo][user-defined] Remove __getattribute__ checks and add getsetdescriptor #144173
Conversation
[ghstack-poisoned]
🔗 Helpful Links🧪 See artifacts and rendered test results at hud.pytorch.org/pr/144173
Note: Links to docs will display an error until the docs builds have been completed. ✅ No FailuresAs of commit de2fa35 with merge base 0a94bb4 ( This comment was automatically generated by Dr. CI and updates every 15 minutes. |
cc voznesenskym penguinwu EikanWang jgong5 Guobing-Chen XiaobingSuper zhuhaozhe blzheng wenzhe-nrv jiayisunx chenyang78 kadeng chauhang amjames [ghstack-poisoned]
anijain2305
added
ciflow/trunk
cc voznesenskym penguinwu EikanWang jgong5 Guobing-Chen XiaobingSuper zhuhaozhe blzheng wenzhe-nrv jiayisunx chenyang78 kadeng chauhang amjames [ghstack-poisoned]
anijain2305
changed the title
| elif object_has_getattribute(self.value) and subobj is NO_SUCH_SUBOBJ: | ||
| raise AttributeError |
There was a problem hiding this comment.
Can you explain how this works? Where is this being handled?
There was a problem hiding this comment.
I added a comment in the latest commit. Dynamo has already tried tracing Getattribute by this point, and has encountered an AttributeError, so its right to return the AttributeError.
I tried really hard to write a test case with overridden __getattribute__ with side-effects. But I was unable to write one that fails on main branch, and passes with this PR. Everything I tried passes on main branch because either (1) there is a graph break on LOAD_ATTR (and therefore whole frame falls to eager), or (2) even though mutations are applied twice on main branch (one while invoking the getattribute method in _getattr_static and other with Dynamo residual bytecode) and is technically wrong, the residual bytecode writes the correct value at the end, so the tests still pass.
I did add a test for GetSetDescriptor in the latest commit.
…add getsetdescriptor" cc voznesenskym penguinwu EikanWang jgong5 Guobing-Chen XiaobingSuper zhuhaozhe blzheng wenzhe-nrv jiayisunx chenyang78 kadeng chauhang amjames [ghstack-poisoned]
|
@pytorchbot merge |
Merge startedYour change will be merged once all checks pass (ETA 0-4 Hours). Learn more about merging in the wiki. Questions? Feedback? Please reach out to the PyTorch DevX Team |
Stack from ghstack (oldest at bottom):
cc @voznesenskym @penguinwu @EikanWang @jgong5 @Guobing-Chen @XiaobingSuper @zhuhaozhe @blzheng @wenzhe-nrv @jiayisunx @chenyang78 @kadeng @chauhang @amjames