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: tracing ivy.set_item with Ellipsis/unbound slices #28771

Merged
merged 4 commits into from
Jun 24, 2024
Merged

Conversation

Sam-Armstrong
Copy link
Contributor

@hmahmood24 just wanted your opinion on this.

The problem is if we trace through the manual set_item logic - _parse_slice/_parse_ellipsis, etc with an Ellipsis or unbounded slice in the query, the indices which get parsed out will then be hard coded into the traced graph. So the traced graph would only work with inputs of the specific shape it was traced with. We can get around this by using numpy as a intermediary step, but this makes the traced graph incompatible with tf.function.

My thought is that we can just use numpy whenever there is an ellipsis or unbounded slice in the query, then at least the traced graph will work when not compiled - this is what I've implemented here. Curious if you agree with my thinking though.

fyi, this issue with dynamic shapes was appearing a lot in the kornia integration testing, which is what brought this issue to light. The changes made here seem to solve most, if not all, the issues.

Copy link
Contributor

@hmahmood24 hmahmood24 left a comment

Choose a reason for hiding this comment

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

@Sam-Armstrong I think we moved away from using numpy in set_item and get_item altogether because of it being incompatible with tf.function and was causing tons of issues with native compilations iirc. I'm hesitant to go back to that logic again. Can we implement a partial mixed tensorflow backend impl of set_item which simply does x[query] = val for this particular condition? I think that would work since we enable numpy behaviour in the tensorflow backend and that should also work with tf.function I assume

@Sam-Armstrong
Copy link
Contributor Author

Sam-Armstrong commented Jun 24, 2024

@hmahmood24 no we can't do that, the experimental numpy behaviour in tensorflow doesn't support mutilation. I've dug into this and I'm pretty sure there's no way around this issue. Either we use numpy (so not tf.function compatible), or we use the manual implementation which will parse this ellipses and slices, but bake the parsed indices into the graph. I thought the condition implemented here was the best trade-off of these two.

@Sam-Armstrong
Copy link
Contributor Author

merging this as a temporary fix.

@Sam-Armstrong Sam-Armstrong merged commit d33939d into main Jun 24, 2024
3 of 5 checks passed
@Sam-Armstrong Sam-Armstrong deleted the ivy-set-item branch June 24, 2024 11:40
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.

2 participants