-
Notifications
You must be signed in to change notification settings - Fork 19.5k
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
Solve memory inefficiency in RNNs #16174
Conversation
Thanks for your pull request! It looks like this may be your first contribution to a Google open source project. Before we can look at your pull request, you'll need to sign a Contributor License Agreement (CLA). For more information, open the CLA check for this pull request. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for the PR!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM, thanks
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There are test failures, please take a look: https://source.cloud.google.com/results/invocations/919aaccc-7dd1-4d0e-9912-0596f18e60c1/targets/keras%2Fgithub%2Fubuntu%2Fgpu%2Fpresubmit/log
@fchollet just saw the test log. I think it comes from the following: The error happens in the tests that combine CPU and GPU environments. I see three options to solve this:
I think the ideal solution would be 3. For the time being, I would go for 2. Let me know what you think! PS: I had executed the tests locally with no errors. I have re-executed them again and no errors were found, as can be seen in this notebook. |
Let's do option 2. Option 3. is not realistically doable given that the computation is delegated to cuDNN. |
I believe it should be fixed now. I have executed all tests locally and there are no errors. However, there were no errors also before this fix, as I mentioned in my last comment. Let's see Thanks again for your time @fchollet |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM, thanks!
@fchollet I saw how the tests you requested worked but the tests that have just executed do not. Edit: I have checked the code of the test that fails and apparently it has nothing to do with my updates. The model being tested is this one |
I think this is possibly a case of flaky test. Let's try rerunning the tests. |
The tests have passed! I am not sure if you need to approve my last review request, as the PR is "Approved by Reviewer" but it also shows that there is 1 pending reviewer. Anyway, thanks a lot for your time! It was a good learning experience as my first contribution @fchollet |
As discussed with @mattdangerw, I am opening this PR to fix #16113 memory inefficiency in RNNs.