[non-strict export] uncovered cases of select and slice #157821
Conversation
avikchaudhuri
requested review from
angelayi,
tugsbayasgalan,
ydwu4 and
zhxchen17
as code owners
🔗 Helpful Links🧪 See artifacts and rendered test results at hud.pytorch.org/pr/157821
Note: Links to docs will display an error until the docs builds have been completed. ✅ No FailuresAs of commit eae96ee with merge base 752f202 ( This comment was automatically generated by Dr. CI and updates every 15 minutes. |
|
This pull request was exported from Phabricator. Differential Revision: D77943929 |
avikchaudhuri
force-pushed
the
export-D77943929
branch
from
b0dafe7 to
e938eb6
Compare
|
This pull request was exported from Phabricator. Differential Revision: D77943929 |
avikchaudhuri
force-pushed
the
export-D77943929
branch
from
e938eb6 to
44a6846
Compare
|
This pull request was exported from Phabricator. Differential Revision: D77943929 |
avikchaudhuri
force-pushed
the
export-D77943929
branch
from
44a6846 to
98887a5
Compare
Summary: Pull Request resolved: pytorch#157821 `None` and `Ellipsis` in multi-dimensional indexing was previously not covered. Moreover, we introduce a small optimization for `slice(None)` and a passthrough when symints do not appear in the indexing. The remaining case is where indexing is by tensor, which is fairly complicated; we passthrough in that case. Test Plan: added tests Rollback Plan: Differential Revision: D77943929
| if isinstance(item, slice): | ||
| if all(s is None for s in (item.start, item.stop, item.step)): | ||
| # no-op | ||
| return dim + 1, (lambda t: t, []) |
There was a problem hiding this comment.
this case should be covered by #157528, but it's fine to have
nit: this passthrough also applies when item.step == 1
There was a problem hiding this comment.
I can add the step=1 case.
| n_none_slices -= 1 | ||
|
|
||
| # only rewrite when there are symints | ||
| if has_symint: |
There was a problem hiding this comment.
It seems like this logic should still apply without symints? Testing locally, removing this branch passes the test too
There was a problem hiding this comment.
Yeah it should pass. Which is why I have the G_* modules which I used to test concrete inputs locally.
I just didn't want to perturb existing generated code too much. In a separate PR I had tried doing this for strict and had to update a gazillion tests.
| t = _method(t, *_args) | ||
| return t | ||
|
|
||
| return run, [], {} |
There was a problem hiding this comment.
in general it seems really weird that we need to handle all this logic here, and there's not some centralized handling we can easily dispatch to
There was a problem hiding this comment.
Yes, agreed. Unfortunately handling this in strict would need duplicate logic since all checks would be in terms of isinstance of Dynamo variables.
As a separate pass I can pull up the individual handlers (e.g., distributed) to the top level.
avikchaudhuri
force-pushed
the
export-D77943929
branch
from
98887a5 to
c7f48b2
Compare
avikchaudhuri
force-pushed
the
export-D77943929
branch
from
c7f48b2 to
71d221f
Compare
|
This pull request was exported from Phabricator. Differential Revision: D77943929 |
Summary: Pull Request resolved: pytorch#157821 `None` and `Ellipsis` in multi-dimensional indexing was previously not covered. Moreover, we introduce a small optimization for `slice(None)` and a passthrough when symints do not appear in the indexing. The remaining case is where indexing is by tensor, which is fairly complicated; we passthrough in that case. Test Plan: added tests Rollback Plan: Reviewed By: pianpwk Differential Revision: D77943929
|
This pull request was exported from Phabricator. Differential Revision: D77943929 |
avikchaudhuri
force-pushed
the
export-D77943929
branch
from
71d221f to
eae96ee
Compare
|
@pytorchbot merge (Initiating merge automatically since Phabricator Diff has merged) |
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 |
4 participants
Summary:
NoneandEllipsisin multi-dimensional indexing was previously not covered.Moreover, we introduce a small optimization for
slice(None)and a passthrough when symints do not appear in the indexing.The remaining case is where indexing is by tensor, which is fairly complicated; we passthrough in that case.
Test Plan:
added tests
Rollback Plan:
Differential Revision: D77943929