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

[AOTInductor] Use ProxyExecutor for aten op if c-shim is missing #113918

Closed
wants to merge 1 commit into from

Conversation

SherlockNoMad
Copy link
Contributor

@SherlockNoMad SherlockNoMad commented Nov 17, 2023

Summary:
As discussed in the meeting, we are inverting the policy on the use of proxy executor for aten fallbacks.
By default, aten fallback ops will use proxy executor, unless a c-shim is available.

Test Plan: CIs

Differential Revision: D51417683

cc @voznesenskym @penguinwu @EikanWang @jgong5 @Guobing-Chen @XiaobingSuper @zhuhaozhe @blzheng @wenzhe-nrv @jiayisunx @peterbell10 @ipiszy @yf225 @chenyang78 @kadeng @muchulee8 @aakhundov @ColinPeppler

Copy link

pytorch-bot bot commented Nov 17, 2023

🔗 Helpful Links

🧪 See artifacts and rendered test results at hud.pytorch.org/pr/113918

Note: Links to docs will display an error until the docs builds have been completed.

✅ You can merge normally! (2 Unrelated Failures)

As of commit e74ebbe with merge base 631fb33 (image):

BROKEN TRUNK - The following jobs failed but were present on the merge base:

👉 Rebase onto the `viable/strict` branch to avoid these failures

This comment was automatically generated by Dr. CI and updates every 15 minutes.

@facebook-github-bot
Copy link
Contributor

This pull request was exported from Phabricator. Differential Revision: D51417683

@SherlockNoMad SherlockNoMad added the topic: not user facing topic category label Nov 17, 2023
SherlockNoMad added a commit to SherlockNoMad/pytorch that referenced this pull request Nov 17, 2023
…orch#113918)

Summary:

As discussed in the meeting, we are inverting the policy on the use of proxy executor for aten fallbacks.
By default, aten fallback ops will use proxy executor, unless a c-shim is available.

Added test for aten.sort and aten.index.Tensor, as they are now runnable with proxy executor.

Test Plan: CIs

Differential Revision: D51417683
@facebook-github-bot
Copy link
Contributor

This pull request was exported from Phabricator. Differential Revision: D51417683

Copy link
Contributor

@chenyang78 chenyang78 left a comment

Choose a reason for hiding this comment

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

Have a minor question. LGTM overall. Thanks.

@@ -973,6 +973,7 @@ def forward(self, x):
example_inputs = (torch.randn(8, 4, 4, device=self.device),)
self.check_model(Model(), example_inputs)

@unittest.skipIf(IS_FBCODE, "Not runnable in fbcode")
Copy link
Contributor

Choose a reason for hiding this comment

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

Wondering why this test is not runnable in fbcode? Does the change of using ProxyExecutor as fallback by default affect this test? Thanks.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

In OSS ABI mode, this test is failing due to aten.index.Tensor missing c-shim, this is why we have it as xfail in the first place.
In fbcode ABI mode, this is failing due to extern_kernel_serializer not populated in AOTInductorModelRunner. I tried to add it, but this introduces dependency of fbcode module ‘deeplearning.aot_inductor.extern_node_thrift_serializer’ into an oss test file, which buck build failed.

As a result, I added the same test case to test_custom_op.py, as you can see in the fbcode diff.

SherlockNoMad added a commit to SherlockNoMad/pytorch that referenced this pull request Nov 17, 2023
…orch#113918)

Summary:

As discussed in the meeting, we are inverting the policy on the use of proxy executor for aten fallbacks.
By default, aten fallback ops will use proxy executor, unless a c-shim is available.

Added test for aten.sort and aten.index.Tensor, as they are now runnable with proxy executor.

Test Plan: CIs

Reviewed By: chenyang78

Differential Revision: D51417683
@facebook-github-bot
Copy link
Contributor

This pull request was exported from Phabricator. Differential Revision: D51417683

SherlockNoMad added a commit to SherlockNoMad/pytorch that referenced this pull request Nov 17, 2023
…orch#113918)

Summary:

As discussed in the meeting, we are inverting the policy on the use of proxy executor for aten fallbacks.
By default, aten fallback ops will use proxy executor, unless a c-shim is available.

Added test for aten.sort and aten.index.Tensor, as they are now runnable with proxy executor.

Test Plan: CIs

Reviewed By: chenyang78

Differential Revision: D51417683
@facebook-github-bot
Copy link
Contributor

This pull request was exported from Phabricator. Differential Revision: D51417683

SherlockNoMad added a commit to SherlockNoMad/pytorch that referenced this pull request Nov 17, 2023
…orch#113918)

Summary:

As discussed in the meeting, we are inverting the policy on the use of proxy executor for aten fallbacks.
By default, aten fallback ops will use proxy executor, unless a c-shim is available.

Added test for aten.sort and aten.index.Tensor, as they are now runnable with proxy executor.

Test Plan: CIs

Reviewed By: chenyang78

Differential Revision: D51417683
@facebook-github-bot
Copy link
Contributor

This pull request was exported from Phabricator. Differential Revision: D51417683

SherlockNoMad added a commit to SherlockNoMad/pytorch that referenced this pull request Nov 17, 2023
…orch#113918)

Summary:

As discussed in the meeting, we are inverting the policy on the use of proxy executor for aten fallbacks.
By default, aten fallback ops will use proxy executor, unless a c-shim is available.

Added test for aten.sort and aten.index.Tensor, as they are now runnable with proxy executor.

Test Plan: CIs

Reviewed By: chenyang78

Differential Revision: D51417683
@facebook-github-bot
Copy link
Contributor

This pull request was exported from Phabricator. Differential Revision: D51417683

…orch#113918)

Summary:

As discussed in the meeting, we are inverting the policy on the use of proxy executor for aten fallbacks.
By default, aten fallback ops will use proxy executor, unless a c-shim is available.

Added test for aten.sort and aten.index.Tensor, as they are now runnable with proxy executor.

Test Plan: CIs

Reviewed By: chenyang78

Differential Revision: D51417683
@facebook-github-bot
Copy link
Contributor

This pull request was exported from Phabricator. Differential Revision: D51417683

@facebook-github-bot
Copy link
Contributor

@pytorchbot merge

(Initiating merge automatically since Phabricator Diff has merged)

@pytorch-bot pytorch-bot bot added the ciflow/trunk Trigger trunk jobs on your pull request label Nov 18, 2023
@pytorchmergebot
Copy link
Collaborator

Merge started

Your change will be merged once all checks pass (ETA 0-4 Hours).

Learn more about merging in the wiki.

Questions? Feedback? Please reach out to the PyTorch DevX Team

Advanced Debugging
Check the merge workflow status
here

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants