-
Notifications
You must be signed in to change notification settings - Fork 25.6k
[dynamo][easy] Miscellaneous fixes #144141
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
Conversation
[ghstack-poisoned]
🔗 Helpful Links🧪 See artifacts and rendered test results at hud.pytorch.org/pr/144141
Note: Links to docs will display an error until the docs builds have been completed. ✅ No FailuresAs of commit 2620393 with merge base 48a05ee ( 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]
res = opt_fn(mv_out_op, torch.empty(0), torch.ones(3, 3), torch.ones(3)) | ||
self.assertEqual(ref, res) | ||
|
||
def test_dict_mutation_side_effect(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.
moved to test_dicts.py
|
||
torch._dynamo.testing.standard_test(self, fn, 1, expected_ops=1) | ||
|
||
@unittest.skipIf(sys.version_info[:2] <= (3, 8), "Requires astunparse") |
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.
not relevant after cpp guards
@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 |
if item.is_immutable(): | ||
return False | ||
if isinstance(item.mutation_type, AttributeMutationNew): | ||
if isinstance(item.mutation_type, (AttributeMutationNew, ValueMutationNew)): |
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.
This one is a bit surprising, since ValueMutationNew
suggests we don't need to track it for side effects, e.g., it's a dictionary/list created by the code Dynamo is tracing through, and its state would be fully represented by ListVariable
or ConstDictVariable
.
It's possible that we attached the wrong mutation_type
somewhere, or I might be missing something.
Pull Request resolved: #144158 Approved by: https://github.com/williamwen42, https://github.com/jansel ghstack dependencies: #144129, #144130, #144141
Pull Request resolved: #144163 Approved by: https://github.com/yanboliang, https://github.com/williamwen42, https://github.com/jansel ghstack dependencies: #144129, #144130, #144141, #144158
…ble (#144160) Pull Request resolved: #144160 Approved by: https://github.com/williamwen42, https://github.com/jansel ghstack dependencies: #144129, #144130, #144141, #144158, #144163
Stack from ghstack (oldest at bottom):
cc @voznesenskym @penguinwu @EikanWang @jgong5 @Guobing-Chen @XiaobingSuper @zhuhaozhe @blzheng @wenzhe-nrv @jiayisunx @chenyang78 @kadeng @chauhang @amjames