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

fix: numpy indexing in tf backend getitem #28556

Merged
merged 5 commits into from
Mar 13, 2024
Merged

Conversation

mattbarrett98
Copy link
Contributor

No description provided.

@vedpatwardhan
Copy link
Contributor

vedpatwardhan commented Mar 12, 2024

lgtm! Feel free to merge the PR once the speedup is tested, thanks @mattbarrett98 😄
(The failures in the CI seem unrelated to the changes made)

@vedpatwardhan
Copy link
Contributor

probably let's also create the set_item PR if we can do something similar there, would be helpful for doing the benchmarks 😄

@mattbarrett98
Copy link
Contributor Author

lgtm! Feel free to merge the PR once the speedup is tested, thanks @mattbarrett98 😄 (The failures in the CI seem unrelated to the changes made)

haris said it seems to be working for the problematic models so all good there. How does the ci work, how come it is raising these errors if they aren't related? (although some of them definitely do seem to be unrelated)

@mattbarrett98
Copy link
Contributor Author

probably let's also create the set_item PR if we can do something similar there, would be helpful for doing the benchmarks 😄

seems it doesn't do the same for mutations, but this isn't a blocker for the demos anyway so it's okay (for now)

@vedpatwardhan
Copy link
Contributor

I think the main issue is the way mapping is computed across tests. So when we try to compute the mapping with coverage and we run all the tests (every saturday), basically many of the tests eventually end up using get_item when they call ivy.Array.__getitem__. So seems like there's quite a few tests that are considered related to the get_item function and so all of them get triggered. But given that many of them have already been failing, we don't need to worry about them too much. We only need to be concerned about failures marked as (main: pass) because those are the ones that pass on main and set_item seems to be one of them 😅

@vedpatwardhan
Copy link
Contributor

vedpatwardhan commented Mar 12, 2024

probably let's also create the set_item PR if we can do something similar there, would be helpful for doing the benchmarks 😄

seems it doesn't do the same for mutations, but this isn't a blocker for the demos anyway so it's okay (for now)

Yeah sure, but probably we can do something similar to this by just doing the getitem call instead of this line (where we have to convert the array to numpy before applying the get_item) and that should help just get the _parse_query helper reverted back to what it was before my commit which should get set_item passing as well. I know that it isn't as high priority but if it's a quick thing to do probably we can do that too, either in this PR or in a different PR. Actually @Ishticode could you please create this second PR? Let's merge this one for now in that case. Thanks @mattbarrett98, @Ishticode 😄

@mattbarrett98
Copy link
Contributor Author

I think the main issue is the way mapping is computed across tests. So when we try to compute the mapping with coverage and we run all the tests (every saturday), basically many of the tests eventually end up using get_item when they call ivy.Array.__getitem__. So seems like there's quite a few tests that are considered related to the get_item function and so all of them get triggered. But given that many of them have already been failing, we don't need to worry about them too much. We only need to be concerned about failures marked as (main: pass) because those are the ones that pass on main and set_item seems to be one of them 😅

i see, so the only one listed with main: pass is the set_item one you linked. however i just tried this on main and it does fail with the same, and this is a failure in the torch backend anyway which can't be related to these changes

@mattbarrett98
Copy link
Contributor Author

probably let's also create the set_item PR if we can do something similar there, would be helpful for doing the benchmarks 😄

seems it doesn't do the same for mutations, but this isn't a blocker for the demos anyway so it's okay (for now)

Yeah sure, but probably we can do something similar to this by just doing the getitem call instead of this line (where we have to convert the array to numpy before applying the get_item) and that should help just get the _parse_query helper reverted back to what it was before my commit which should get set_item passing as well. I know that it isn't as high priority but if it's a quick thing to do probably we can do that too, either in this PR or in a different PR. Actually @Ishticode could you please create this second PR? Let's merge this one for now in that case. Thanks @mattbarrett98, @Ishticode 😄

makes sense, i'd say let's give it at least a week or 2 so we can gauge how well the new solution is working

@mattbarrett98 mattbarrett98 merged commit 733510c into ivy-llc:main Mar 13, 2024
93 of 141 checks passed
@mattbarrett98 mattbarrett98 deleted the tgi branch March 13, 2024 15:09
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.

3 participants