-
Notifications
You must be signed in to change notification settings - Fork 453
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
Add support for XLA_DISABLE_FUNCTIONALIZATION flag #4792
Conversation
This change adds support for XLA_DISABLE_FUNCTIONALIZATION. This involves restoring some of the deleted ops when we enabled functionalization and updating some ops to fall back to pre-functionalization state. This change also adds a new test suite that runs test_operations.py with functionalization disabled. Also, see note: [Disabling functionalization].
…CTIONALIZATION case
…zation is disabled
ddef0b9
to
d91bb3d
Compare
Locally I can see that
And also succeeds without the flag:
I'll let the CI verify other tests. |
d91bb3d
to
28a541b
Compare
28a541b
to
944d41f
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Generally LGTM. Please address my comments. Thanks for coming out with this PR so quickly.
@@ -965,6 +965,9 @@ def func(a, b): | |||
b = torch.ones([2, 2]) | |||
self.runAtenTest((a, b), func) | |||
|
|||
@unittest.skipIf( |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Instead of skipping the tests here, maybe you can utilize the env and have different assertEquals.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
These two tests didn't exist before functionalization was enabled (they were newly added as of #4158), should we still even run these when functionalization is disabled?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Forgot the details. Thanks for pointing it out. Then, let's skip.
Thanks for the review, Jiewen. Merging this now as CIs are green. |
Add support for XLA_DISABLE_FUNCTIONALIZATION flag
This change adds support for XLA_DISABLE_FUNCTIONALIZATION. This involves restoring some of the deleted ops when we enabled functionalization and updating some ops to fall back to pre-functionalization state. This change also adds a new test suite that runs test_operations.py with functionalization disabled.
Test plan:
source run_tests.sh
and CIOps that are completely restored:
Ops that are modified by checking
XLA_DISABLE_FUNCTIONALIZATION
flagSkipped tests (these two tests were added as part of the funtionalization commit, skipping when
XLA_DISABLE_FUNCTIONALIZATION
is set to true)Fixed tests
slice
)as_strided
,as_strided_
)convlution_backward
)diagonal
)expand
)view
)select_symint
)view
,new_empty_strided_symint
)view
)slice
)select_symint
)select_symint
)slice
)select_symint
)new_empty_strided_symint
)view
)view
)view
)convlution_backward
)convlution_backward
)cc @alanwaketan