handle unbacked index in meta_select #156906
Conversation
[ghstack-poisoned]
🔗 Helpful Links🧪 See artifacts and rendered test results at hud.pytorch.org/pr/156906
Note: Links to docs will display an error until the docs builds have been completed. ❌ 13 New Failures, 1 Unrelated FailureAs of commit d8a0615 with merge base d98fa4a ( NEW FAILURES - The following jobs have failed:
UNSTABLE - The following job is marked as unstable, possibly due to flakiness on trunk:
This comment was automatically generated by Dr. CI and updates every 15 minutes. |
| torch._check( | ||
| index >= 0, | ||
| lambda: "unbacked index assumed >=0, if that's not correct consider adding a torch._check on index {index}", | ||
| ) |
There was a problem hiding this comment.
I think this along with slicing, is one of the unsafe cases for compile?
There was a problem hiding this comment.
why would you say its unsafe?do you mean that users can hit this error by unsafe?
I think we are free to define unbacked semantics, and as long as in the error message
we specify how the user can control the behaviour of this by adding torch.check, then we are good.
for example here index is either >=0 or <0 currently we do not support a choice that allow for both in compile
(while we could potentially with your idea of allocating a new unbacked symbol) but at least for now.
we assume positive by default if it end up wrong users can go back and decide on the neg by adding a torch check.
they anyway would have had to do that in the case of DDE.
There was a problem hiding this comment.
I guess, users can have a model that sometimes indexes positively, sometimes negatively, turn on torch.compile + capture_scalar_outputs, and now they hit Runtime assertion: u0 >= 0 when they do the negative index?
There was a problem hiding this comment.
yes but they will get
Runtime assertion: u0 >= 0 "unbacked index assumed >=0, if that's not correct consider adding a torch._check on index u0"
note that before this they would hit a DDE, the model wont compile anyway.
do you say that we rather throw DDE than have this unbacked semantics?
There was a problem hiding this comment.
Before this, they could still graph break and their program could run both positive & negative indexing with capture_scalar_outputs = True.
Now with the assert, they're restricted to just one of those options?
There was a problem hiding this comment.
regarding graphs break i think the moment users flipped capture_scalar_outputs, they opted into unbacked semantics if they prefer to graph break they probably should keep capture_scalar_outputs turned off.
that said i agree its worth to look into how hard is it to allocate a new unbacked symbol to resolve this before landing this, if its not too hard its better indeed. i will look into that.
laithsakka
changed the title
|
closing in favor of #157605 |
2 participants
Stack from ghstack (oldest at bottom):