add support for hint_override in mark_unbacked#162652
add support for hint_override in mark_unbacked#162652bobrenjc93 wants to merge 14 commits intogh/bobrenjc93/558/basefrom
Conversation
[ghstack-poisoned]
🔗 Helpful Links🧪 See artifacts and rendered test results at hud.pytorch.org/pr/162652
Note: Links to docs will display an error until the docs builds have been completed. ✅ No FailuresAs of commit 6318725 with merge base dac8a4b ( This comment was automatically generated by Dr. CI and updates every 15 minutes. |
cc ezyang SherlockNoMad EikanWang jgong5 wenzhe-nrv voznesenskym penguinwu Guobing-Chen XiaobingSuper zhuhaozhe blzheng jiayisunx ipiszy chenyang78 kadeng muchulee8 amjames chauhang aakhundov coconutruben Lucaskabela [ghstack-poisoned]
Very similar to #161007 except now for mark_unbacked. This consolidates the approach to only use the hint override for size hints in contrast to the original approach which actually allowed guarding on the hint_override. cc ezyang SherlockNoMad EikanWang jgong5 wenzhe-nrv voznesenskym penguinwu Guobing-Chen XiaobingSuper zhuhaozhe blzheng jiayisunx ipiszy chenyang78 kadeng muchulee8 amjames chauhang aakhundov coconutruben Lucaskabela [ghstack-poisoned]
Very similar to #161007 except now for mark_unbacked. This consolidates the approach to only use the hint override for size hints in contrast to the original approach which actually allowed guarding on the hint_override. cc ezyang SherlockNoMad EikanWang jgong5 wenzhe-nrv voznesenskym penguinwu Guobing-Chen XiaobingSuper zhuhaozhe blzheng jiayisunx ipiszy chenyang78 kadeng muchulee8 amjames chauhang aakhundov coconutruben Lucaskabela [ghstack-poisoned]
Very similar to #161007 except now for mark_unbacked. This consolidates the approach to only use the hint override for size hints in contrast to the original approach which actually allowed guarding on the hint_override. cc ezyang SherlockNoMad EikanWang jgong5 wenzhe-nrv voznesenskym penguinwu Guobing-Chen XiaobingSuper zhuhaozhe blzheng jiayisunx ipiszy chenyang78 kadeng muchulee8 amjames chauhang aakhundov coconutruben Lucaskabela [ghstack-poisoned]
Very similar to #161007 except now for mark_unbacked. This consolidates the approach to only use the hint override for size hints in contrast to the original approach which actually allowed guarding on the hint_override. cc ezyang SherlockNoMad EikanWang jgong5 wenzhe-nrv voznesenskym penguinwu Guobing-Chen XiaobingSuper zhuhaozhe blzheng jiayisunx ipiszy chenyang78 kadeng muchulee8 amjames chauhang aakhundov coconutruben Lucaskabela [ghstack-poisoned]
Very similar to #161007 except now for mark_unbacked. This consolidates the approach to only use the hint override for size hints in contrast to the original approach which actually allowed guarding on the hint_override. cc ezyang SherlockNoMad EikanWang jgong5 wenzhe-nrv voznesenskym penguinwu Guobing-Chen XiaobingSuper zhuhaozhe blzheng jiayisunx ipiszy chenyang78 kadeng muchulee8 amjames chauhang aakhundov coconutruben Lucaskabela [ghstack-poisoned]
|
another thing to think about is this: people used to depend on the fact that they pass fallback or else it would throw now with your change if hint is overriden it will just be no throw always implicitly on unbacked |
ezyang
left a comment
ezyang
left a comment
There was a problem hiding this comment.
Help me understand why this prevents people from guarding/adding runtime asserts based on the hints. It seems unsafe to allow arbitrary queries to symbolic_hint to rely on this. You need to make some assertion at the use site that you promise to ONLY use this in a heuristic nature, and that your output is sound even if it varies the full range.
Very similar to #161007 except now for mark_unbacked. cc ezyang SherlockNoMad EikanWang jgong5 wenzhe-nrv voznesenskym penguinwu Guobing-Chen XiaobingSuper zhuhaozhe blzheng jiayisunx ipiszy chenyang78 kadeng muchulee8 amjames chauhang aakhundov coconutruben Lucaskabela [ghstack-poisoned]
Very similar to #161007 except now for mark_unbacked. cc ezyang SherlockNoMad EikanWang jgong5 wenzhe-nrv voznesenskym penguinwu Guobing-Chen XiaobingSuper zhuhaozhe blzheng jiayisunx ipiszy chenyang78 kadeng muchulee8 amjames chauhang aakhundov coconutruben Lucaskabela [ghstack-poisoned]
I agree we discussed creating a new API or adding an to argument symbolic_hint that allow reading overrides with a comment that say you can only use it in a heuristic nature. The alternative as is (is people have to understand this is playing as actual hint that can implicitly control any inductor decisions including guarding (runtime assertions in this case). but i like the special API/Args to control when those are used better. |
|
The problem I'd specifically like to avoid is when we could have reported an error at compile time, but instead it got baked in as a runtime assert and you have to run it to see it overspecialized. |
Very similar to #161007 except now for mark_unbacked. cc ezyang SherlockNoMad EikanWang jgong5 wenzhe-nrv voznesenskym penguinwu Guobing-Chen XiaobingSuper zhuhaozhe blzheng jiayisunx ipiszy chenyang78 kadeng muchulee8 amjames chauhang aakhundov coconutruben Lucaskabela [ghstack-poisoned]
Apologies for the late response, I've been battling a pretty bad illness over the past few days. I agree with your concern and have updated the PR to include a flag requiring explicit opt-in to this behavior. |
|
Thanks, and hope you are feeling better. My puzzlement now is, why is this flag not implied by the fallback argument at all these call sites? |
My intuition is against muddying the waters since Are you implying deriving Or perhaps I'm misunderstanding? If you have a cleaner API proposal, I'm all ears! |
Very similar to #161007 except now for mark_unbacked. cc ezyang SherlockNoMad EikanWang jgong5 wenzhe-nrv voznesenskym penguinwu Guobing-Chen XiaobingSuper zhuhaozhe blzheng jiayisunx ipiszy chenyang78 kadeng muchulee8 amjames chauhang aakhundov coconutruben Lucaskabela [ghstack-poisoned]
Very similar to #161007 except now for mark_unbacked. cc ezyang SherlockNoMad EikanWang jgong5 wenzhe-nrv voznesenskym penguinwu Guobing-Chen XiaobingSuper zhuhaozhe blzheng jiayisunx ipiszy chenyang78 kadeng muchulee8 amjames chauhang aakhundov coconutruben Lucaskabela [ghstack-poisoned]
Very similar to #161007 except now for mark_unbacked. cc ezyang SherlockNoMad EikanWang jgong5 wenzhe-nrv voznesenskym penguinwu Guobing-Chen XiaobingSuper zhuhaozhe blzheng jiayisunx ipiszy chenyang78 kadeng muchulee8 amjames chauhang aakhundov coconutruben Lucaskabela [ghstack-poisoned]
|
Synced with @ezyang offline and collectively decided to consolidate on the |
|
@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 |
Very similar to pytorch#161007 except now for mark_unbacked. Pull Request resolved: pytorch#162652 Approved by: https://github.com/laithsakka
Very similar to pytorch#161007 except now for mark_unbacked. Pull Request resolved: pytorch#162652 Approved by: https://github.com/laithsakka
Very similar to pytorch#161007 except now for mark_unbacked. Pull Request resolved: pytorch#162652 Approved by: https://github.com/laithsakka
5 participants
Stack from ghstack (oldest at bottom):
Very similar to #161007 except now for mark_unbacked.
cc @ezyang @SherlockNoMad @EikanWang @jgong5 @wenzhe-nrv @voznesenskym @penguinwu @Guobing-Chen @XiaobingSuper @zhuhaozhe @blzheng @jiayisunx @ipiszy @chenyang78 @kadeng @muchulee8 @amjames @chauhang @aakhundov @coconutruben @Lucaskabela