Skip to content

check params shape for mkldnn_convolution#76526

Closed
iexpos wants to merge 5 commits intopytorch:masterfrom
iexpos:master
Closed

check params shape for mkldnn_convolution#76526
iexpos wants to merge 5 commits intopytorch:masterfrom
iexpos:master

Conversation

@iexpos
Copy link

@iexpos iexpos commented Apr 28, 2022

Fixes #73193

Follow check rules from native/Convolution.cpp without transpose supported.
Seems that mkldnn_convolution does not support transpose. ideep has special api for that.

@facebook-github-bot
Copy link
Contributor

facebook-github-bot commented Apr 28, 2022

Hi @iexpos!

Thank you for your pull request and welcome to our community.

Action Required

In order to merge any pull request (code, docs, etc.), we require contributors to sign our Contributor License Agreement, and we don't seem to have one on file for you.

Process

In order for us to review and merge your suggested changes, please sign at https://code.facebook.com/cla. If you are contributing on behalf of someone else (eg your employer), the individual CLA may not be sufficient and your employer may need to sign the corporate CLA.

Once the CLA is signed, our tooling will perform checks and validations. Afterwards, the pull request will be tagged with CLA signed. The tagging process may take up to 1 hour after signing. Please give it that time before contacting us about it.

If you have received this in error or have any questions, please contact us at cla@fb.com. Thanks!

@facebook-github-bot
Copy link
Contributor

facebook-github-bot commented Apr 28, 2022 •
edited
Loading

🔗 Helpful links

✅ No Failures (0 Pending)

As of commit 94bead7 (more details on the Dr. CI page):

Expand to see more

💚 💚 Looks good so far! There are no failures yet. 💚 💚

This comment was automatically generated by Dr. CI (expand for details).

Please report bugs/suggestions to the (internal) Dr. CI Users group.

Click here to manually regenerate this comment.

@iexpos iexpos changed the title check params shape for mkldnn_convolution (#73193) check params shape for mkldnn_convolution Apr 28, 2022
@facebook-github-bot
Copy link
Contributor

facebook-github-bot commented Apr 28, 2022

Thank you for signing our Contributor License Agreement. We can now accept your code for this (and any) Meta Open Source project. Thanks!

@iexpos iexpos closed this Apr 29, 2022
@iexpos iexpos reopened this Apr 29, 2022
@mruberry mruberry requested a review from mingfeima May 3, 2022 09:15
@mruberry
Copy link
Collaborator

mruberry commented May 3, 2022

@VitalyFedyunin @malfet -- would one of you suggest an appropriate reviewer for this PR?

@ejguan ejguan added module: cpu CPU specific problem (e.g., perf, algorithm) triaged This issue has been looked at a team member, and triaged and prioritized into an appropriate module module: mkldnn Related to Intel IDEEP or oneDNN (a.k.a. mkldnn) integration and removed triaged This issue has been looked at a team member, and triaged and prioritized into an appropriate module labels May 3, 2022
@bdhirsh bdhirsh requested a review from VitalyFedyunin May 4, 2022 21:00
@bdhirsh bdhirsh added the triaged This issue has been looked at a team member, and triaged and prioritized into an appropriate module label May 4, 2022
@mingfeima mingfeima requested a review from XiaobingSuper May 13, 2022 02:56
XiaobingSuper
XiaobingSuper reviewed May 13, 2022
View reviewed changes
@iexpos iexpos requested a review from XiaobingSuper May 13, 2022 13:14
@XiaobingSuper
Copy link
Collaborator

XiaobingSuper commented May 15, 2022

@VitalyFedyunin

@github-actions
Copy link
Contributor

github-actions bot commented Jul 14, 2022

Looks like this PR hasn't been updated in a while so we're going to go ahead and mark this as Stale.
Feel free to remove the Stale label if you feel this was a mistake.
If you are unable to remove the Stale label please contact a maintainer in order to do so.
If you want the bot to never mark this PR stale again, add the no-stale label.
Stale pull requests will automatically be closed after 30 days of inactivity.

@github-actions github-actions bot added the Stale label Jul 14, 2022
@lezcano
Copy link
Collaborator

lezcano commented Jul 30, 2022

cc @jbschlosser

jbschlosser
jbschlosser reviewed Aug 1, 2022
View reviewed changes
Copy link
Contributor

@jbschlosser jbschlosser left a comment

Choose a reason for hiding this comment

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

LGTM, thanks for the fix! Do you mind rebasing to see if we can silence some of the spurious CI checks?

@iexpos
Copy link
Author

iexpos commented Aug 2, 2022

LGTM, thanks for the fix! Do you mind rebasing to see if we can silence some of the spurious CI checks?

@jbschlosser Already rebased to latest, please check.

@jbschlosser
Copy link
Contributor

jbschlosser commented Aug 2, 2022

@pytorchbot merge

@pytorchmergebot
Copy link
Collaborator

pytorchmergebot commented Aug 2, 2022

@pytorchbot successfully started a merge and created land time checks. See merge status here and land check progress here

pytorchmergebot pushed a commit that referenced this pull request Aug 2, 2022
@pytorchmergebot
check params shape for mkldnn_convolution (#76526)
9abbf05 
Fixes #73193

Follow check rules from native/Convolution.cpp without transpose supported.
Seems that mkldnn_convolution does not support transpose. ideep has special api for that.
Pull Request resolved: #76526
Approved by: https://github.com/XiaobingSuper, https://github.com/mingfeima, https://github.com/jbschlosser
@pytorchmergebot
Copy link
Collaborator

pytorchmergebot commented Aug 2, 2022

Merge failed due to Failed to merge; some land checks failed: pull, pull / win-vs2019-cpu-py3 / test (default, 1, 2, windows.4xlarge)
Raised by https://github.com/pytorch/pytorch/actions/runs/2783046253 If you believe this is an error, you can use the old behavior with @pytorchbot merge -g (optionally with the ciflow/trunk to get land signals) or use `@pytorchbot merge -f "". For more information see the bot wiki.

@jbschlosser
Copy link
Contributor

jbschlosser commented Aug 2, 2022

@pytorchbot merge -f "unrelated errors"

@pytorchmergebot
Copy link
Collaborator

pytorchmergebot commented Aug 2, 2022

@pytorchbot successfully started a merge job. Check the current status here

@github-actions
Copy link
Contributor

github-actions bot commented Aug 2, 2022

Hey @iexpos.
You've committed this PR, but it does not have both a 'release notes: ...' and 'topics: ...' label. Please add one of each to the PR. The 'release notes: ...' label should represent the part of PyTorch that this PR changes (fx, autograd, distributed, etc) and the 'topics: ...' label should represent the kind of PR it is (not user facing, new feature, bug fix, perf improvement, etc). The list of valid labels can be found here for the 'release notes: ...' and here for the 'topics: ...'.
For changes that are 'topic: not user facing' there is no need for a release notes label.

@jbschlosser jbschlosser added release notes: nn release notes category topic: bug fixes topic category labels Aug 2, 2022
facebook-github-bot pushed a commit that referenced this pull request Aug 3, 2022
@facebook-github-bot
check params shape for mkldnn_convolution (#76526) (#76526)
f4a33bf 
Summary:
Fixes #73193

Follow check rules from native/Convolution.cpp without transpose supported.
Seems that mkldnn_convolution does not support transpose. ideep has special api for that.

Pull Request resolved: #76526
Approved by: https://github.com/XiaobingSuper, https://github.com/mingfeima, https://github.com/jbschlosser

Test Plan: contbuild & OSS CI, see https://hud.pytorch.org/commit/pytorch/pytorch/9f5f6ba68337f071f3141309348b0eed103a8d18

Reviewed By: kit1980

Differential Revision: D38359687

fbshipit-source-id: 3b8f88bde97bb800329baa3a6ad189de1849712c
@jiayisunx jiayisunx mentioned this pull request Nov 23, 2022
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@XiaobingSuper XiaobingSuper XiaobingSuper approved these changes

@mingfeima mingfeima mingfeima approved these changes

@jbschlosser jbschlosser jbschlosser approved these changes

@VitalyFedyunin VitalyFedyunin Awaiting requested review from VitalyFedyunin

Assignees

No one assigned

Labels

cla signed Merged module: cpu CPU specific problem (e.g., perf, algorithm) module: mkldnn Related to Intel IDEEP or oneDNN (a.k.a. mkldnn) integration open source release notes: nn release notes category Stale topic: bug fixes topic category triaged This issue has been looked at a team member, and triaged and prioritized into an appropriate module

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

Floating point exception in mkldnn_convolution