Add missing types to inductor IR assert#96221
Add missing types to inductor IR assert#96221wconstab wants to merge 3 commits intogh/wconstab/124/basefrom
Conversation
[ghstack-poisoned]
🔗 Helpful Links🧪 See artifacts and rendered test results at hud.pytorch.org/pr/96221
Note: Links to docs will display an error until the docs builds have been completed. ⏳ No Failures, 1 PendingAs of commit 17ae642: This comment was automatically generated by Dr. CI and updates every 15 minutes. |
|
I mean, hopefully it is obvious what IR types are allowed? 🤔 It's not obvious to me though... |
|
I can approve this to unblock but I really have no idea how to evaluate the correctness of this PR. |
|
well, i added the assert mainly to catch the case where you forgot to wrap your lowering in a TensorBox, but i figured it'd be useful in general to have some invariants about the IR. in practice, it seems like there is still quite a lot of evolution of what the IR is, and most of it is related to dynamic shapes / sympy work. it would not be so bad to just widen the assert, and it would also be ok to remove the assert if its more trouble than its worth. |
Unclear if there is a more efficient way to define the allowed types for IR (or if we even need this, perhaps we just ditch the assert?) But Inductor experts can deteremine if these added ops are appropriate and if so they fix the reported issue. Fixes #96204 cc soumith voznesenskym penguinwu anijain2305 EikanWang jgong5 Guobing-Chen XiaobingSuper zhuhaozhe blzheng Xia-Weiwen wenzhe-nrv jiayisunx peterbell10 desertfire [ghstack-poisoned]
|
@pytorchbot merge |
|
Tests for this would be very helpful, since we're not sure if this is the final state. |
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 |
Merge failedReason: 1 mandatory check(s) failed. The first few are: Dig deeper by viewing the failures on hud |
I don't really know how one would go about testing this other than by running all of our existing inductor tests- i think the gap is that we don't test enough of inductor's op coverage period. (e.g. there are ops we're lowering to inductor in models that we're not covering with unit tests) |
Unclear if there is a more efficient way to define the allowed types for IR (or if we even need this, perhaps we just ditch the assert?) But Inductor experts can deteremine if these added ops are appropriate and if so they fix the reported issue. Fixes #96204 cc soumith voznesenskym penguinwu anijain2305 EikanWang jgong5 Guobing-Chen XiaobingSuper zhuhaozhe blzheng Xia-Weiwen wenzhe-nrv jiayisunx peterbell10 desertfire [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 |
Unclear if there is a more efficient way to define the allowed types for IR (or if we even need this, perhaps we just ditch the assert?) But Inductor experts can deteremine if these added ops are appropriate and if so they fix the reported issue. Fixes #96204 Pull Request resolved: pytorch/pytorch#96221 Approved by: https://github.com/ezyang
Unclear if there is a more efficient way to define the allowed types for IR (or if we even need this, perhaps we just ditch the assert?) But Inductor experts can deteremine if these added ops are appropriate and if so they fix the reported issue. Fixes #96204 Pull Request resolved: pytorch/pytorch#96221 Approved by: https://github.com/ezyang
Unclear if there is a more efficient way to define the allowed types for IR (or if we even need this, perhaps we just ditch the assert?) But Inductor experts can deteremine if these added ops are appropriate and if so they fix the reported issue. Fixes pytorch#96204 Pull Request resolved: pytorch#96221 Approved by: https://github.com/ezyang
3 participants
Stack from ghstack (oldest at bottom):
Unclear if there is a more efficient way to define the allowed types for IR (or if we even need this, perhaps we just ditch the assert?) But Inductor experts can deteremine if these added ops are appropriate and if so they fix the reported issue.
Fixes #96204
cc @soumith @voznesenskym @penguinwu @anijain2305 @EikanWang @jgong5 @Guobing-Chen @XiaobingSuper @zhuhaozhe @blzheng @Xia-Weiwen @wenzhe-nrv @jiayisunx @peterbell10 @desertfire