Skip to content
This repository has been archived by the owner on Nov 17, 2023. It is now read-only.

MKL-DNN RNN checks NDArray version #16071

Merged
merged 3 commits into from
Sep 4, 2019

Conversation

zixuanweeei
Copy link
Contributor

Description

This should fix ISSUE #16037. A member variable weights_version is added to RNNOp class. Besides, instead of using Storage::Handle to hold the temp space, this PR turns to use NDArray for the management of temp space. @ciyongch @PatricZhao @TaoLv @ZhennanQin

Checklist

Changes

  • Weights version (NDArray.version()) check, unit test added
  • Storage::Handle -> NDArray

@zixuanweeei
Copy link
Contributor Author

Hi, @matteosal. This PR aims to fix the problem filed by #16037. Hope it could work. Would you mind checking on this branch to see whether it works for your cases? It will be very helpful. Thanks.

@matteosal
Copy link
Contributor

Tested, it fixes the issue

@zixuanweeei
Copy link
Contributor Author

Tested, it fixes the issue

Thanks a lot 🙂.

@zixuanweeei
Copy link
Contributor Author

CI passed. Please take a look @PatricZhao @ciyongch

@pengzhao-intel
Copy link
Contributor

@ciyongch please help take a reveiw

Copy link
Contributor

@ciyongch ciyongch left a comment

Choose a reason for hiding this comment

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

LGTM.

Copy link
Contributor

@pengzhao-intel pengzhao-intel left a comment

Choose a reason for hiding this comment

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

Thanks for the quick fix. Merging now.

@pengzhao-intel pengzhao-intel merged commit 07b4470 into apache:master Sep 4, 2019
gyshi pushed a commit to gyshi/incubator-mxnet that referenced this pull request Sep 7, 2019
* MKL-DNN RNN checks NDArray version

* Add UT

* Use default_context()
gyshi pushed a commit to gyshi/incubator-mxnet that referenced this pull request Sep 7, 2019
* MKL-DNN RNN checks NDArray version

* Add UT

* Use default_context()
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants