Skip to content
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

[Script] Be more careful when generating ast.ExtSlice for Subscript #15483

Merged
merged 2 commits into from
Aug 5, 2023

Conversation

kparzysz-quic
Copy link
Contributor

The ast.ExtSlice expects a non-empty list, otherwise evaluation fails with error: empty dims on ExtSlice. Also, each element in "dims" list of ExtSlice must be either Slice or Index.

In python3.8 an expression A[()] is parsed (by ast) as Subscript with slice being Index(value=Tuple(elts=[])). When we translate a subscript from doc.AST to ast, we unconditionally convert every tuple to ast.ExtSlice, which in this case is incorrect.

The fix is to map empty tuple back to the Index(Tuple[])) instead of ExtSlice. In other cases, ensure that members of ExtSlice are of correct types.

The ast.ExtSlice expects a non-empty list, otherwise evaluation
fails with "error: empty dims on ExtSlice". Also, each element
in "dims" list of ExtSlice must be either Slice or Index.

In python3.8 an expression A[()] is parsed (by ast) as Subscript
with slice being Index(value=Tuple(elts=[])). When we translate a
subscript from doc.AST to ast, we unconditionally convert every
tuple to ast.ExtSlice, which in this case is incorrect.

The fix is to map empty tuple back to the Index(Tuple[])) instead
of ExtSlice. In other cases, ensure that members of ExtSlice are
of correct types.
@tvm-bot
Copy link
Collaborator

tvm-bot commented Aug 4, 2023

Thanks for contributing to TVM! Please refer to the contributing guidelines https://tvm.apache.org/docs/contribute/ for useful information and tips. Please request code reviews from Reviewers by @-ing them in a comment.

  • No users to tag found in teams: script See #10317 for details

Generated by tvm-bot

2 similar comments
@tvm-bot
Copy link
Collaborator

tvm-bot commented Aug 4, 2023

Thanks for contributing to TVM! Please refer to the contributing guidelines https://tvm.apache.org/docs/contribute/ for useful information and tips. Please request code reviews from Reviewers by @-ing them in a comment.

  • No users to tag found in teams: script See #10317 for details

Generated by tvm-bot

@tvm-bot
Copy link
Collaborator

tvm-bot commented Aug 4, 2023

Thanks for contributing to TVM! Please refer to the contributing guidelines https://tvm.apache.org/docs/contribute/ for useful information and tips. Please request code reviews from Reviewers by @-ing them in a comment.

  • No users to tag found in teams: script See #10317 for details

Generated by tvm-bot

@kparzysz-quic
Copy link
Contributor Author

cc: @Hzfengsy @junrushao @yzh119

Copy link
Contributor

@Lunderberg Lunderberg left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thank you for the fix, and LGTM! It looks like this is specifically for python 3.8 and earlier, as the ast.Index and ast.ExtSlice classes were deprecated in python 3.9.

I'm rather surprised on how this test was able to pass, as the CI runs on python3.8.16, and this test case also uses length-zero tuples as an index. Does the bug only trigger when the object is passed to an external macro?

@kparzysz-quic
Copy link
Contributor Author

kparzysz-quic commented Aug 4, 2023

This happens when we evaluate and expression containing () as an index, specifically when it get translated back into ast and then passed to eval. I encountered this while using macros, and I also thought it should happen without macros too, but I wasn't able to reproduce it with a simple testcase. I can dig into this some more to see why it worked there, but failed in a macro.

@junrushao
Copy link
Member

Interesting findings! When initiating TVMScript, we tested bunch of cases in certain python versions, but not all - which might be the source of such issues

@kparzysz-quic kparzysz-quic merged commit 18467c9 into apache:main Aug 5, 2023
@kparzysz-quic kparzysz-quic deleted the index-empty-tuple branch August 5, 2023 00:41
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants