-
Notifications
You must be signed in to change notification settings - Fork 22.2k
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
[cond] error on closed over variables #99367
Conversation
As reported in #90469, the implementation of inlining nested function branches for `cond` doesn't properly handle variables captured from outer scopes. This leads to some examples accidentally working, some others generating incorrect code that don't crash but do the wrong thing, and still others that outright crash because of references to non-existent variables. Properly supporting closed variables is tricky (see #91981 for an abandoned attempt). While this is definitely something we should be able to support longer term, for now it is better to explicitly error and suggest the fix to the user (amounting to rewriting branches to take closed variables explicitly). Differential Revision: [D45058621](https://our.internmc.facebook.com/intern/diff/D45058621/) [ghstack-poisoned]
🔗 Helpful Links🧪 See artifacts and rendered test results at hud.pytorch.org/pr/99367
Note: Links to docs will display an error until the docs builds have been completed. ❌ 2 FailuresAs of commit 4a77cdc: NEW FAILURES - The following jobs have failed:
This comment was automatically generated by Dr. CI and updates every 15 minutes. |
This PR needs a labelIf your changes are user facing and intended to be a part of release notes, please use a label starting with If not, please add the To add a label, you can comment to pytorchbot, for example For more information, see |
As reported in #90469, the implementation of inlining nested function branches for `cond` doesn't properly handle variables captured from outer scopes. This leads to some examples accidentally working, some others generating incorrect code that don't crash but do the wrong thing, and still others that outright crash because of references to non-existent variables. Properly supporting closed variables is tricky (see #91981 for an abandoned attempt). While this is definitely something we should be able to support longer term, for now it is better to explicitly error and suggest the fix to the user (amounting to rewriting branches to take closed variables explicitly). Differential Revision: [D45058621](https://our.internmc.facebook.com/intern/diff/D45058621/) cc soumith voznesenskym penguinwu anijain2305 EikanWang jgong5 Guobing-Chen XiaobingSuper zhuhaozhe blzheng Xia-Weiwen wenzhe-nrv jiayisunx desertfire [ghstack-poisoned]
Pull Request resolved: #99367 As reported in #90469, the implementation of inlining nested function branches for `cond` doesn't properly handle variables captured from outer scopes. This leads to some examples accidentally working, some others generating incorrect code that don't crash but do the wrong thing, and still others that outright crash because of references to non-existent variables. Properly supporting closed variables is tricky (see #91981 for an abandoned attempt). While this is definitely something we should be able to support longer term, for now it is better to explicitly error and suggest the fix to the user (amounting to rewriting branches to take closed variables explicitly). ghstack-source-id: 186352080 Differential Revision: [D45058621](https://our.internmc.facebook.com/intern/diff/D45058621/)
As reported in #90469, the implementation of inlining nested function branches for `cond` doesn't properly handle variables captured from outer scopes. This leads to some examples accidentally working, some others generating incorrect code that don't crash but do the wrong thing, and still others that outright crash because of references to non-existent variables. Properly supporting closed variables is tricky (see #91981 for an abandoned attempt). While this is definitely something we should be able to support longer term, for now it is better to explicitly error and suggest the fix to the user (amounting to rewriting branches to take closed variables explicitly). Differential Revision: [D45058621](https://our.internmc.facebook.com/intern/diff/D45058621/) cc soumith voznesenskym penguinwu anijain2305 EikanWang jgong5 Guobing-Chen XiaobingSuper zhuhaozhe blzheng Xia-Weiwen wenzhe-nrv jiayisunx desertfire [ghstack-poisoned]
Pull Request resolved: #99367 As reported in #90469, the implementation of inlining nested function branches for `cond` doesn't properly handle variables captured from outer scopes. This leads to some examples accidentally working, some others generating incorrect code that don't crash but do the wrong thing, and still others that outright crash because of references to non-existent variables. Properly supporting closed variables is tricky (see #91981 for an abandoned attempt). While this is definitely something we should be able to support longer term, for now it is better to explicitly error and suggest the fix to the user (amounting to rewriting branches to take closed variables explicitly). ghstack-source-id: 186391673 Differential Revision: [D45058621](https://our.internmc.facebook.com/intern/diff/D45058621/)
As reported in #90469, the implementation of inlining nested function branches for `cond` doesn't properly handle variables captured from outer scopes. This leads to some examples accidentally working, some others generating incorrect code that don't crash but do the wrong thing, and still others that outright crash because of references to non-existent variables. Properly supporting closed variables is tricky (see #91981 for an abandoned attempt). While this is definitely something we should be able to support longer term, for now it is better to explicitly error and suggest the fix to the user (amounting to rewriting branches to take closed variables explicitly). Differential Revision: [D45058621](https://our.internmc.facebook.com/intern/diff/D45058621/) cc soumith voznesenskym penguinwu anijain2305 EikanWang jgong5 Guobing-Chen XiaobingSuper zhuhaozhe blzheng Xia-Weiwen wenzhe-nrv jiayisunx desertfire [ghstack-poisoned]
Pull Request resolved: #99367 As reported in #90469, the implementation of inlining nested function branches for `cond` doesn't properly handle variables captured from outer scopes. This leads to some examples accidentally working, some others generating incorrect code that don't crash but do the wrong thing, and still others that outright crash because of references to non-existent variables. Properly supporting closed variables is tricky (see #91981 for an abandoned attempt). While this is definitely something we should be able to support longer term, for now it is better to explicitly error and suggest the fix to the user (amounting to rewriting branches to take closed variables explicitly). ghstack-source-id: 186457954 Differential Revision: [D45058621](https://our.internmc.facebook.com/intern/diff/D45058621/)
As reported in #90469, the implementation of inlining nested function branches for `cond` doesn't properly handle variables captured from outer scopes. This leads to some examples accidentally working, some others generating incorrect code that don't crash but do the wrong thing, and still others that outright crash because of references to non-existent variables. Properly supporting closed variables is tricky (see #91981 for an abandoned attempt). While this is definitely something we should be able to support longer term, for now it is better to explicitly error and suggest the fix to the user (amounting to rewriting branches to take closed variables explicitly). Differential Revision: [D45058621](https://our.internmc.facebook.com/intern/diff/D45058621/) cc soumith voznesenskym penguinwu anijain2305 EikanWang jgong5 Guobing-Chen XiaobingSuper zhuhaozhe blzheng Xia-Weiwen wenzhe-nrv jiayisunx desertfire [ghstack-poisoned]
Pull Request resolved: #99367 As reported in #90469, the implementation of inlining nested function branches for `cond` doesn't properly handle variables captured from outer scopes. This leads to some examples accidentally working, some others generating incorrect code that don't crash but do the wrong thing, and still others that outright crash because of references to non-existent variables. Properly supporting closed variables is tricky (see #91981 for an abandoned attempt). While this is definitely something we should be able to support longer term, for now it is better to explicitly error and suggest the fix to the user (amounting to rewriting branches to take closed variables explicitly). ghstack-source-id: 186556432 Differential Revision: [D45058621](https://our.internmc.facebook.com/intern/diff/D45058621/)
Issue for follow up: #99401 |
@pytorchbot merge -f "unrelated ci failures (doctr)" |
Merge startedYour change will be merged immediately since you used the force (-f) flag, bypassing any CI checks (ETA: 1-5 minutes). Learn more about merging in the wiki. Questions? Feedback? Please reach out to the PyTorch DevX Team |
As of #99367 we error when cond branches look up closed vars. The suggested fix is to add these closed vars as args to the branches. However, while this works for tensor vars (and also primitive vars by explicit wrapping), this is impossible to do for function vars. Moreover, function vars are OK because we trace through them. So relaxing this restriction for function vars is a strict win. Differential Revision: [D45287893](https://our.internmc.facebook.com/intern/diff/D45287893/) [ghstack-poisoned]
As of #99367 we error when cond branches look up closed vars. The suggested fix is to add these closed vars as args to the branches. However, while this works for tensor vars (and also primitive vars by explicit wrapping), this is impossible to do for function vars. Moreover, function vars are OK because we trace through them. So relaxing this restriction for function vars is a strict win. Differential Revision: [D45287893](https://our.internmc.facebook.com/intern/diff/D45287893/) ghstack-source-id: 187112368 Pull Request resolved: #100013
As of #99367 we error when cond branches look up closed vars. The suggested fix is to add these closed vars as args to the branches. However, while this works for tensor vars (and also primitive vars by explicit wrapping), this is impossible to do for function vars. Moreover, function vars are OK because we trace through them. So relaxing this restriction for function vars is a strict win. Differential Revision: [D45287893](https://our.internmc.facebook.com/intern/diff/D45287893/) cc soumith voznesenskym penguinwu anijain2305 EikanWang jgong5 Guobing-Chen XiaobingSuper zhuhaozhe blzheng Xia-Weiwen wenzhe-nrv jiayisunx desertfire [ghstack-poisoned]
Pull Request resolved: #100013 As of #99367 we error when cond branches look up closed vars. The suggested fix is to add these closed vars as args to the branches. However, while this works for tensor vars (and also primitive vars by explicit wrapping), this is impossible to do for function vars. Moreover, function vars are OK because we trace through them. So relaxing this restriction for function vars is a strict win. ghstack-source-id: 187142449 Differential Revision: [D45287893](https://our.internmc.facebook.com/intern/diff/D45287893/)
As of #99367 we error when cond branches look up closed vars. The suggested fix is to add these closed vars as args to the branches. However, while this works for tensor vars (and also primitive vars by explicit wrapping), this is impossible to do for function vars. Moreover, function vars are OK because we trace through them. So relaxing this restriction for function vars is a strict win. Differential Revision: [D45287893](https://our.internmc.facebook.com/intern/diff/D45287893/) Pull Request resolved: #100013 Approved by: https://github.com/tugsbayasgalan
Stack from ghstack (oldest at bottom):
As reported in #90469, the implementation of inlining nested function branches for
cond
doesn't properly handle variables captured from outer scopes. This leads to some examples accidentally working, some others generating incorrect code that don't crash but do the wrong thing, and still others that outright crash because of references to non-existent variables.Properly supporting closed variables is tricky (see #91981 for an abandoned attempt). While this is definitely something we should be able to support longer term, for now it is better to explicitly error and suggest the fix to the user (amounting to rewriting branches to take closed variables explicitly).
Differential Revision: D45058621
cc @soumith @voznesenskym @penguinwu @anijain2305 @EikanWang @jgong5 @Guobing-Chen @XiaobingSuper @zhuhaozhe @blzheng @Xia-Weiwen @wenzhe-nrv @jiayisunx @desertfire