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

Make CUDNN an alias of MIOPEN for HIP ops #12278

Closed
wants to merge 14 commits into from

Conversation

bddppq
Copy link
Contributor

@bddppq bddppq commented Oct 3, 2018

This is mostly for reusing all the cudnn test cases in our python operator_tests.

@bddppq
Copy link
Contributor Author

bddppq commented Oct 3, 2018

@bddppq
Copy link
Contributor Author

bddppq commented Oct 3, 2018

@petrex @ashishfarmer There are some tests failing for the MIOPEN conv ops, could you help taking a look? error message, detailed jenkins log

@petrex
Copy link
Contributor

petrex commented Oct 3, 2018 via email

@ashishfarmer
Copy link

This is a regression that was caused by my earlier check in for group conv optimizations. The changes in PR#12273 will fix this issue. Could we land that one first please

@petrex
Copy link
Contributor

petrex commented Oct 3, 2018

#12273 @bddppq

@bddppq
Copy link
Contributor Author

bddppq commented Oct 3, 2018

@ashishfarmer @petrex ah I see. landing #12273

@bddppq
Copy link
Contributor Author

bddppq commented Oct 3, 2018

@bddppq
Copy link
Contributor Author

bddppq commented Oct 3, 2018

@petrex @ashishfarmer there is a new error in test_convolution_layout https://gist.github.com/bddppq/b40b12637acb9b2554412525d1e78094

@petrex
Copy link
Contributor

petrex commented Oct 3, 2018 via email

@ashishfarmer
Copy link

@bddppq, It is trying to run NHWC using MIOpen. Currently we don't support NHWC layout in MIOpen.
In the conv_test.py, there needs a check

if _run_in_hip(gc, dc) and (engine == 'CUDNN') and (order != 'NCHW'):
            return

similar to the checks in spatial_bn_op_test.py

@bddppq
Copy link
Contributor Author

bddppq commented Oct 4, 2018

@ashishfarmer Thanks for looking! Could you take this PR over to fix all the failed miopen tests?

@bddppq bddppq added the module: rocm AMD GPU support for Pytorch label Oct 4, 2018
@ashishfarmer
Copy link

Sure thing. Will check and enable appropriate tests for miopen

@ashishfarmer
Copy link

@pytorchbot retest this please

engine_list.append('CUDNN')
if _run_in_hip(gc, dc):
if order == 'NCHW':
engine_list.append('CUDNN')

This comment was marked as off-topic.

@bddppq
Copy link
Contributor Author

bddppq commented Oct 6, 2018

@ashishfarmer
Copy link

ashishfarmer commented Oct 9, 2018

@bddppq, triggered tests after the commits: https://ci.pytorch.org/jenkins/job/caffe2-builds/job/py2-clang7-rocmdeb-ubuntu16.04-trigger-test/1015/
The test that is failing is RNN cell test, and may have nothing to do with CUDNN aliasing. We are looking into the failure, but as far as changes for this PR are concerned, it looks done.

@petrex
Copy link
Contributor

petrex commented Oct 10, 2018

failing test : operator_test.rnn_cell_test.RNNCellTest.test_lstm_with_dot_attention_different_dim (from pytest)

IIRC , miopen does not support attention in lstm at this moment. Shall we skip this test?
@rohithkrn pls chime in.

@rohithkrn
Copy link
Contributor

This test doesn't use miopen implementation of LSTM.

@ashishfarmer
Copy link

Triggered tests: https://ci.pytorch.org/jenkins/job/caffe2-builds/job/py2-clang7-rocmdeb-ubuntu16.04-trigger-test/2115/

@bddppq, the PR is ready for review

@@ -19,6 +19,9 @@
import unittest
import os

def _run_in_hip(gc, dc):

This comment was marked as off-topic.

def test_spatialbn_test_mode_1d(
self, size, input_channels, batch_size, seed, order, epsilon,
inplace, engine, gc, dc):
# Currently MIOPEN SpatialBN only supports 2D
if _run_in_hip(gc, dc) and engine != "":

This comment was marked as off-topic.

def test_spatialbn_train_mode_gradient_check_1d(
self, size, input_channels, batch_size, seed, order, epsilon,
momentum, engine, gc, dc):
# Currently MIOPEN SpatialBN only supports 2D
if _run_in_hip(gc, dc) and engine != "":
return

This comment was marked as off-topic.

def test_spatialbn_test_mode_3d(
self, size, input_channels, batch_size, seed, order, epsilon,
inplace, engine, gc, dc):
# Currently MIOPEN SpatialBN only supports 2D
if _run_in_hip(gc, dc) and engine != "":
return

This comment was marked as off-topic.

@bddppq
Copy link
Contributor Author

bddppq commented Oct 24, 2018

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.

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

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
module: rocm AMD GPU support for Pytorch
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants