-
Notifications
You must be signed in to change notification settings - Fork 25.6k
Raise MutationError if there are side effects when returning generator #145223
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
Raise MutationError if there are side effects when returning generator #145223
Conversation
🔗 Helpful Links🧪 See artifacts and rendered test results at hud.pytorch.org/pr/145223
Note: Links to docs will display an error until the docs builds have been completed. ✅ No FailuresAs of commit 206caf7 with merge base 44b69b8 ( This comment was automatically generated by Dr. CI and updates every 15 minutes. |
@pytorchbot rebase |
@pytorchbot started a rebase job onto refs/remotes/origin/viable/strict. Check the current status here |
Successfully rebased |
test/dynamo/test_generator.py
Outdated
@unittest.expectedFailure | ||
def test_reconstruct_generator_mutate_tensor(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.
Can you file an issue for this once this PR goes in so that we can track it as a known issue?
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.
Here #146628
def whoo(t): | ||
yield t.sin() | ||
yield t.cos() | ||
yield t.tan() |
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.
@StrongerXi @anijain2305 do we have any side effects that aren't variable mutation? We're trying to test a generator with side effects
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.
Maybe register_hook
for input tensor?
Simpler proposal -- what about passing in some pre-existing dict or dummy object and update that (e.g., before the first yield)?.
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.
Oh, good thought. So, maybe it isn't safe to support any kind of variable mutation when reconstructing a generator.
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.
lgtm but have some comments, please read
Stack from ghstack (oldest at bottom):
enable_faithful_generator_behavior
flag to True #142513CLEANUP_THROW
bytecode #144420generator.throw(exception)
#144424generator.close()
#144423generator.send(..)
#144422generator.__iter__()
#144421cc @voznesenskym @penguinwu @EikanWang @jgong5 @Guobing-Chen @XiaobingSuper @zhuhaozhe @blzheng @wenzhe-nrv @jiayisunx @chenyang78 @kadeng @chauhang @amjames