[JIT]Support adv indexing using list. #37848
Closed
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
ailzhang
changed the title
facebook-github-bot
added
the
oncall: jit
There was a problem hiding this comment.
@ailzhang has imported this pull request. If you are a Facebook employee, you can view this diff on Phabricator.
suo
requested changes
May 5, 2020
| @@ -571,7 +571,9 @@ def build_ExtSlice(ctx, base, extslice): | |||
| base = build_expr(ctx, expr.value) | |||
| sub_type = type(expr.slice) | |||
| if sub_type is ast.Index: | |||
| if isinstance(expr.slice.value, ast.Tuple) or isinstance(expr.slice.value, ast.List): | |||
| if isinstance(expr.slice.value, ast.Tuple): | |||
There was a problem hiding this comment.
If you are making a change to the python frontend, you probably need to make one to the string frontend as well. self.checkScript will make sure the behavior is the same between Python, TS+python, TS+no python.
There was a problem hiding this comment.
Btw I switched to use self.checkScript in the test and it passed. Is string frontend the one that takes in function strings and parse it? If so this change might be shared by both since it's done at python ast level I think?
ailzhang
force-pushed
the
fix_jit_adv_indexing_list
branch
from
3aa7050 to
adcc03d
Compare
ailzhang
force-pushed
the
fix_jit_adv_indexing_list
branch
from
adcc03d to
9307068
Compare
ailzhang
force-pushed
the
fix_jit_adv_indexing_list
branch
from
9307068 to
a3ff0fd
Compare
💊 Build failures summary and remediationsAs of commit a3ff0fd (more details on the Dr. CI page):
🕵️ 1 new failure recognized by patternsThe following build failures do not appear to be due to upstream breakages:
|
There was a problem hiding this comment.
@ailzhang has imported this pull request. If you are a Facebook employee, you can view this diff on Phabricator.
suo
reviewed
May 6, 2020
| if (subscript_expr.kind() == TK_NONE) { | ||
| type_hint = NoneType::get(); | ||
| Value* index; | ||
| if (subscript_expr.kind() == TK_LIST_LITERAL) { |
There was a problem hiding this comment.
One question, maybe as a followup to this PR: seems like we should be able to handle any type of list, not just a list literal? e.g., the follow should work:
def f(x):
ls = [0]
ls.append(1)
ls.append(2)
return x[ls]
There was a problem hiding this comment.
Yea good point, it should be possible (maybe by unwrapping a list out of the TK_VAR). I'll take a look and send a followup PR if it works. :D
facebook-github-bot
pushed a commit
that referenced
this pull request
May 8, 2020
Summary: Followup of #37848 I realized that it's better to condition on `Value` type instead of token type. So now it also support indexing through list variables (used to be list literal only). Also apparently our eager frontend accept indexing with float list as well, so matched this edge case behavior as well. Pull Request resolved: #37966 Reviewed By: suo Differential Revision: D21439642 Pulled By: ailzhang fbshipit-source-id: cedb8431ef38747d4aa9909a6bbf8e954dbe0e25
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
We used to only support indexing through
x[0, 1]x[(0, 1)]x[torch.tensor([0, 1])]This PR adds support for indexing through list which is equivalent to tensor.
x[[0, 1, 5]]x[[0, 1], [0, 1]]x[[[0, 1], [0, 1]], [[0, 1], [0, 1]]]Note for
x[[0, 1, 5]]we had a bug in AST conversion code so we used to treat it likex[0, 1, 5]which means it might accidentally run and produce wrong result(fixes #37286 fixes #18616), now that it's fixed we probably want to mark it as BC breaking.