Skip to content

Upgrade mkldnn bridge to reduce overhead of bridge itself#12164

Closed
gujinghui wants to merge 1 commit intopytorch:masterfrom
gujinghui:upgrade_mkldnn_bridge
Closed

Upgrade mkldnn bridge to reduce overhead of bridge itself#12164
gujinghui wants to merge 1 commit intopytorch:masterfrom
gujinghui:upgrade_mkldnn_bridge

Conversation

@gujinghui
Copy link
Collaborator

Upgrade mkldnn bridge to reduce overhead of bridge itself

@gujinghui
Copy link
Collaborator Author

@yinghai @jgong5

Pls review

@gujinghui
Copy link
Collaborator Author

@yinghai have no idead on the failure of "ci/circleci: Config Processing Error"
Pls help review

@gujinghui gujinghui force-pushed the upgrade_mkldnn_bridge branch from e6bb797 to 298d44f Compare September 30, 2018 07:40
@gujinghui
Copy link
Collaborator Author

@pytorchbot retest this please

Copy link
Contributor

@facebook-github-bot facebook-github-bot left a comment

Choose a reason for hiding this comment

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

wesolwsk has imported this pull request. If you are a Facebook employee, you can view this diff on Phabricator.

@gujinghui
Copy link
Collaborator Author

@wesolwsk
Is there any blocking issue for this PR?
If any, pls contact us anytime.

@wesolwsk
Copy link
Contributor

@gujinghui - I am getting conflicts when pulling from intel/mkl-dnn into our local repo. I will investigate further. Trying to get this done this week.

@jgong5
Copy link
Collaborator

jgong5 commented Oct 10, 2018

@wesolwsk Please let us know anything that we can help. We are glad to help.

@gujinghui
Copy link
Collaborator Author

@wesolwsk
We submit latest ideep update for this PR.
There is no change for mkldnn. We just fix some bugs in ideep for training.

Copy link
Contributor

@facebook-github-bot facebook-github-bot left a comment

Choose a reason for hiding this comment

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

wesolwsk has imported this pull request. If you are a Facebook employee, you can view this diff on Phabricator.

Signed-off-by: Gu, Jinghui <jinghui.gu@intel.com>
@gujinghui gujinghui force-pushed the upgrade_mkldnn_bridge branch from 5b1dda5 to 0b6fbab Compare October 30, 2018 02:02
@gujinghui
Copy link
Collaborator Author

@wesolwsk
Just submit a new version of ideep itself to fix a bug in ideep, instead of mkl-dnn. :)

@yinghai
Copy link
Contributor

yinghai commented Nov 9, 2018

@gujinghui Which commit of the bug fix is it?

@wesolwsk
Copy link
Contributor

wesolwsk commented Nov 9, 2018

@yinghai, do we still need this after the mkl-dnn an ideep upgrades?

@yinghai
Copy link
Contributor

yinghai commented Nov 9, 2018

@wesolwsk Yeah, we still need this. But @gujinghui seems to have force push this PR to update the IDEEP version again. We need to check whether it's in our internal repo first.

@jgong5
Copy link
Collaborator

jgong5 commented Nov 9, 2018

@yinghai @wesolwsk We may keep the IDEEP version with the one you are validating internally and rollback some commits if needed. We plan to PR the IDEEP header files into the PyTorch repo soon so that IDEEP changes can be easier to merge in the future.

@jgong5
Copy link
Collaborator

jgong5 commented Nov 10, 2018

@yinghai The "bug fix" @gujinghui referred to should be the one below which is about INT8 support so should be OK to ignore this time:
intel/ideep@e53c8c4

@yinghai
Copy link
Contributor

yinghai commented Nov 15, 2018

@wesolwsk ok to merge?

Copy link
Contributor

@facebook-github-bot facebook-github-bot left a comment

Choose a reason for hiding this comment

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

@wesolwsk has imported this pull request. If you are a Facebook employee, you can view this diff on Phabricator.

yinghai pushed a commit to yinghai/pytorch that referenced this pull request Nov 15, 2018
@yinghai
Copy link
Contributor

yinghai commented Nov 15, 2018

@gujinghui I'm reverting this as it's breaking the master. It probably needs a rebase and resubmission.

#14040

@gujinghui
Copy link
Collaborator Author

@yinghai
Sure. Will resubmit when done.

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.

6 participants