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

[inductor] remove fft and svd ops from fake_incorrect_kernels #103616

Closed
wants to merge 7 commits into from

Conversation

@pytorch-bot
Copy link

pytorch-bot bot commented Jun 14, 2023

🔗 Helpful Links

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

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

❌ 1 New Failure

As of commit b5b2c93:

NEW FAILURE - The following job has failed:

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

nkaretnikov added a commit that referenced this pull request Jun 14, 2023
ghstack-source-id: d2d3cc4998f690a9ac0865552229854d66afde6c
Pull Request resolved: #103616
@nkaretnikov nkaretnikov added ciflow/trunk Trigger trunk jobs on your pull request topic: not user facing topic category ciflow/periodic Trigger jobs ran periodically on master (periodic.yml) on the PR ciflow/slow labels Jun 14, 2023
…_kernels`"

cc voznesenskym penguinwu @EikanWang jgong5 @Guobing-Chen @XiaobingSuper zhuhaozhe blzheng @Xia-Weiwen wenzhe-nrv jiayisunx peterbell10 ipiszy ngimel yf225 chenyang78

[ghstack-poisoned]
nkaretnikov added a commit that referenced this pull request Jun 15, 2023
ghstack-source-id: 944be0964bcd91561dfe162688d000cd73b7b9d4
Pull Request resolved: #103616
…_kernels`"

cc voznesenskym penguinwu @EikanWang jgong5 @Guobing-Chen @XiaobingSuper zhuhaozhe blzheng @Xia-Weiwen wenzhe-nrv jiayisunx peterbell10 ipiszy ngimel yf225 chenyang78

[ghstack-poisoned]
nkaretnikov added a commit that referenced this pull request Jun 15, 2023
ghstack-source-id: 6fba6a1aa7b9d362bed66fdf11d1eead7b6aef6b
Pull Request resolved: #103616
@nkaretnikov

This comment was marked as outdated.

Copy link
Contributor

@eellison eellison left a comment

Choose a reason for hiding this comment

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

LGTM if CI Passes

@eellison
Copy link
Contributor

actually, can you remove https://github.com/pytorch/pytorch/blob/main/test/test_ops.py#L1969-L1990 as part of this ?

@pytorchmergebot

This comment was marked as outdated.

@pytorchmergebot

This comment was marked as outdated.

@nkaretnikov
Copy link
Collaborator Author

nkaretnikov commented Jun 15, 2023

@eellison would failures in fake_tensor_stride_failing_ops be a blocker for merging this if that fails? or can I merge without?

@eellison
Copy link
Contributor

Yea.. If there are striding failures then I think we shouldn't land this. Otherwise we'll get runtime assert failures in inductor

…_kernels`"

cc voznesenskym penguinwu @EikanWang jgong5 @Guobing-Chen @XiaobingSuper zhuhaozhe blzheng @Xia-Weiwen wenzhe-nrv jiayisunx peterbell10 ipiszy ngimel yf225 chenyang78

[ghstack-poisoned]
nkaretnikov added a commit that referenced this pull request Jun 18, 2023
ghstack-source-id: c33ce81eda0b9ecce4020410b6f4f313e4146109
Pull Request resolved: #103616
@nkaretnikov

This comment was marked as outdated.

@pytorchmergebot

This comment was marked as outdated.

…_kernels`"

cc voznesenskym penguinwu EikanWang jgong5 Guobing-Chen XiaobingSuper zhuhaozhe blzheng Xia-Weiwen wenzhe-nrv jiayisunx peterbell10 ipiszy ngimel yf225 chenyang78

[ghstack-poisoned]
@pytorchmergebot

This comment was marked as outdated.

pytorchmergebot pushed a commit that referenced this pull request Jun 18, 2023
ghstack-source-id: 7b8c1a6f2f8e69199d4969750986a1dd8eecc9ea
Pull Request resolved: #103616
…_kernels`"

cc voznesenskym penguinwu @EikanWang jgong5 @Guobing-Chen @XiaobingSuper zhuhaozhe blzheng @Xia-Weiwen wenzhe-nrv jiayisunx peterbell10 ipiszy ngimel yf225 chenyang78

[ghstack-poisoned]
nkaretnikov added a commit that referenced this pull request Jun 18, 2023
ghstack-source-id: c00d1167a8373ce17592d05f9a9d4e9b2bb04a2e
Pull Request resolved: #103616
@nkaretnikov
Copy link
Collaborator Author

@eellison Please take another look: I've pushed an update implementing stride logic from _exec_fft in meta_fft_c2c.

@XiaobingSuper Could you test if this PR fixes the issues you were having with GoogleFnet? This PR now runs the repro from #88248 (comment) instead of erroring out (note: the trailing paren is missing in the repro).

Using FallbackKernel: torch.ops.prims.convert_element_type.default
Using FallbackKernel: aten._fft_c2c.default
tensor([[[  740.4529+0.0000j,   126.5869-135.1518j, -1001.8738-182.1360j,
...

@nkaretnikov nkaretnikov marked this pull request as ready for review June 19, 2023 04:25
@nkaretnikov
Copy link
Collaborator Author

@pytorchbot rebase

@pytorchmergebot
Copy link
Collaborator

@pytorchbot started a rebase job onto refs/remotes/origin/viable/strict. Check the current status here

…_kernels`"

cc voznesenskym penguinwu EikanWang jgong5 Guobing-Chen XiaobingSuper zhuhaozhe blzheng Xia-Weiwen wenzhe-nrv jiayisunx peterbell10 ipiszy ngimel yf225 chenyang78

[ghstack-poisoned]
@pytorchmergebot
Copy link
Collaborator

Successfully rebased gh/nkaretnikov/138/orig onto refs/remotes/origin/viable/strict, please pull locally before adding more changes (for example, via ghstack checkout https://github.com/pytorch/pytorch/pull/103616)

pytorchmergebot pushed a commit that referenced this pull request Jun 21, 2023
ghstack-source-id: 0a3100f574f314875c6ebf2f55a55d3359148642
Pull Request resolved: #103616
Copy link
Contributor

@eellison eellison left a comment

Choose a reason for hiding this comment

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

😍 😍 😍



# See _fft_c2c_cufft in aten/src/ATen/native/cuda/SpectralOps.cpp
# and _fft_c2c_mkl in aten/src/ATen/native/mkl/SpectralOps.cpp
Copy link
Collaborator

Choose a reason for hiding this comment

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

I believe cpu and cuda results diverge in some cases because cuFFT can only transform up to 3 dimensions at once, so we end up taking multiple passes for higher dimensions. This is still better than assuming contiguous tough.

@nkaretnikov
Copy link
Collaborator Author

@pytorchbot merge -f "the slow test failure is also on hud"

@pytorchmergebot
Copy link
Collaborator

Merge started

Your change will be merged immediately since you used the force (-f) flag, bypassing any CI checks (ETA: 1-5 minutes).

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

@XiaobingSuper
Copy link
Collaborator

@eellison Please take another look: I've pushed an update implementing stride logic from _exec_fft in meta_fft_c2c.

@XiaobingSuper Could you test if this PR fixes the issues you were having with GoogleFnet? This PR now runs the repro from #88248 (comment) instead of erroring out (note: the trailing paren is missing in the repro).

Using FallbackKernel: torch.ops.prims.convert_element_type.default
Using FallbackKernel: aten._fft_c2c.default
tensor([[[  740.4529+0.0000j,   126.5869-135.1518j, -1001.8738-182.1360j,
...

@nkaretnikov, thanks, I checked it on my side, and it works now.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
ciflow/inductor ciflow/periodic Trigger jobs ran periodically on master (periodic.yml) on the PR ciflow/slow ciflow/trunk Trigger trunk jobs on your pull request Merged module: inductor open source topic: not user facing topic category
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

6 participants