[ROCm] improve docker packages, fix bugs, enable tests, enable FFT by iotamudelta · Pull Request #10893 · pytorch/pytorch

iotamudelta · 2018-08-27T02:55:48Z

improve docker packages (install OpenBLAS to have at-compile-time LAPACK functionality w/ optimizations for both Intel and AMD CPUs)
integrate rocFFT (i.e., enable Fourier functionality)
fix bugs in ROCm caused by wrong warp size
enable more test sets, skip the tests that don't work on ROCm yet
don't disable asserts any longer in hipification
small improvements

…RAND_PR While there, add the remaining changes requested in upstream PR pytorch#10266

Reported by: bddqqp

Merge from upstream

Refactor unit test skip statements to use @skipIfRocm annotation

Merge from upstream

Replace hcRNG with rocRAND.

… tests for ROCm CI builds

Merge from upstream

aten/src/THC/THCScanUtils.cuh

 #include "THCDeviceUtils.cuh"

+#if defined(__HIP_PLATFORM_HCC__)
+#define WARP_SIZE 64


aten/src/THC/generic/THCTensorTopK.cu

-      sliceSize,                                                        \
-      k,                                                                \
-      inputSlices,                                                      \
+      static_cast<INDEX_T>(sliceSize),                                  \


docker/caffe2/jenkins/common/install_rocm.sh

 install_ubuntu() {
    apt-get update
    apt-get install -y wget
+    apt-get install -y libopenblas-dev


test/common_cuda.py

 TEST_MULTIGPU = TEST_CUDA and torch.cuda.device_count() >= 2
 CUDA_DEVICE = TEST_CUDA and torch.device("cuda:0")
-TEST_CUDNN = TEST_CUDA and torch.backends.cudnn.is_acceptable(torch.tensor(1., device=CUDA_DEVICE))
+TEST_CUDNN = TEST_CUDA and (TEST_WITH_ROCM or torch.backends.cudnn.is_acceptable(torch.tensor(1., device=CUDA_DEVICE)))


test/common_nn.py

        input_size=(4, 10),
-        reference_fn=lambda i, p: torch.mm(i, p[0].t()) + p[1].view(1, -1).expand(4, 8)
+        reference_fn=lambda i, p: torch.mm(i, p[0].t()) + p[1].view(1, -1).expand(4, 8),
+        test_cuda=(not TEST_WITH_ROCM)


test/test_cuda.py

 tests = [
-    ('add', small_3d, lambda t: [number(3.14, 3, t)]),
+    ('add', small_3d, lambda t: [number(3.14, 3, t)], '', types, False,
+        "skipIfRocm:ByteTensor,CharTensor,HalfTensor,ShortTensor"),


test/test_cuda.py

+    ('lerp', small_3d, lambda t: [small_3d(t), 0.3], '', types, False, "skipIfRocm:HalfTensor"),
+    ('max', small_3d_unique, lambda t: [], '', types, False, "skipIfRocm:HalfTensor"),
+    ('max', small_3d_unique, lambda t: [1], 'dim', types, False,
+        "skipIfRocm:ByteTensor,CharTensor,DoubleTensor,FloatTensor,HalfTensor,IntTensor,LongTensor,ShortTensor"),


tools/amd_build/pyHIPIFY/hipify-python.py

-        if not filepath.endswith("THCGeneral.h.in"):
-            output_source = disable_asserts(output_source)
+        # if not filepath.endswith("THCGeneral.h.in"):
+        #    output_source = disable_asserts(output_source)


ezyang

I'm primarily concerned with two things:

The &sizes in the FFT bindings https://github.com/pytorch/pytorch/pull/10893/files#r214418319 . This just looks totally wrong and I don't want to merge code that's wrong.
The manual specification of each of the types Float/Long/Int/etc... in the tests. This seems really delicate, and it would be much better if we weren't banging these out manually; instead, there should be predefined strings for the most common combinations of types that don't work.

ezyang · 2018-08-31T17:24:37Z

@pytorchbot retest this please

facebook-github-bot

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

Unfortunately the CUDA naming is ubiquitous and not removable.

iotamudelta · 2018-08-31T18:32:53Z

@jithunnair-amd can you comment on point no 2? Thanks!

…orm/pytorch

test_cuda - skipping all the types with skipIfRocm

facebook-github-bot

ezyang is landing this pull request. If you are a Facebook employee, you can view this diff on Phabricator.

Summary: * improve docker packages (install OpenBLAS to have at-compile-time LAPACK functionality w/ optimizations for both Intel and AMD CPUs) * integrate rocFFT (i.e., enable Fourier functionality) * fix bugs in ROCm caused by wrong warp size * enable more test sets, skip the tests that don't work on ROCm yet * don't disable asserts any longer in hipification * small improvements Pull Request resolved: pytorch/pytorch#10893 Differential Revision: D9615053 Pulled By: ezyang fbshipit-source-id: 864b4d27bf089421f7dfd8065e5017f9ea2f7b3b

…10893) Summary: * improve docker packages (install OpenBLAS to have at-compile-time LAPACK functionality w/ optimizations for both Intel and AMD CPUs) * integrate rocFFT (i.e., enable Fourier functionality) * fix bugs in ROCm caused by wrong warp size * enable more test sets, skip the tests that don't work on ROCm yet * don't disable asserts any longer in hipification * small improvements Pull Request resolved: pytorch#10893 Differential Revision: D9615053 Pulled By: ezyang fbshipit-source-id: 864b4d27bf089421f7dfd8065e5017f9ea2f7b3b

iotamudelta and others added 30 commits August 7, 2018 09:48

Merge remote-tracking branch 'rocm_upstream/enableunittests' into roc…

e9c047e

…RAND_PR While there, add the remaining changes requested in upstream PR pytorch#10266

Do not set SHARED flag unconditionally here.

f1c36e6

Reported by: bddqqp

Merge remote-tracking branch 'upstream/master'

8558486

Merge pull request #104 from iotamudelta/master

dee2f5b

Merge from upstream

Merge pull request #103 from jithunnair-amd/skipIfRocm_refactor

1f3544f

Refactor unit test skip statements to use @skipIfRocm annotation

Merge remote-tracking branch 'upstream/master'

4903af7

Merge branch 'master' into rocRAND_PR

d306547

Merge remote-tracking branch 'upstream/master'

410d539

Merge branch 'master' into rocRAND_PR

4692427

Merge pull request #105 from iotamudelta/master

fa99099

Merge from upstream

Merge remote-tracking branch 'upstream/master'

eb5975a

Merge branch 'master' into rocRAND_PR

ca94a4a

Update native_functions for MIOpen

78eea43

Merge branch 'master' into miopen_integration

3774e54

Fix build on cuda/cudnn platforms

ed19fc6

Merge pull request #41 from iotamudelta/rocRAND_PR

f025a43

Replace hcRNG with rocRAND.

Merge remote-tracking branch 'upstream/master'

cde2069

First patch to remove hipblas and replace with direct rocblas calls.

f363f87

Fix typo.

4819f55

rocBLAS supports strided batched gemms.

67f77f0

For ROCm, wire batched through to strided batched.

f6d3225

Use __HIP_PLATFORM_HCC__ instead of USE_ROCM macro.

00ec9d5

Fix typo.

db8a1c3

The API is slightly different - dereference.

31ec38e

setting of math mode not supported on ROCm / rocBLAS

2670223

Forget about fp16 for now.

8fa5130

Also disable here.

85bb272

To avoid double definitions.

54ef60b

Enable test_torch, test_dataloader, test_indexing and test_utils unit…

12491c7

… tests for ROCm CI builds

Merge pull request #107 from iotamudelta/master

90cbc01

Merge from upstream

ezyang reviewed Aug 31, 2018

View reviewed changes

aten/src/THC/THCScanUtils.cuh Outdated

#include "THCDeviceUtils.cuh"

#if defined(__HIP_PLATFORM_HCC__)

#define WARP_SIZE 64

This comment was marked as off-topic.

Sign in to view

This comment was marked as off-topic.

Sign in to view

ezyang reviewed Aug 31, 2018

View reviewed changes

aten/src/THC/generic/THCTensorTopK.cu

sliceSize, \

k, \

inputSlices, \

static_cast<INDEX_T>(sliceSize), \

This comment was marked as off-topic.

Sign in to view

This comment was marked as off-topic.

Sign in to view

ezyang reviewed Aug 31, 2018

View reviewed changes

docker/caffe2/jenkins/common/install_rocm.sh

install_ubuntu() {

apt-get update

apt-get install -y wget

apt-get install -y libopenblas-dev

This comment was marked as off-topic.

Sign in to view

This comment was marked as off-topic.

Sign in to view

ezyang reviewed Aug 31, 2018

View reviewed changes

ezyang requested changes Aug 31, 2018

View reviewed changes

facebook-github-bot reviewed Aug 31, 2018

View reviewed changes

iotamudelta added 6 commits August 31, 2018 13:10

Fix whitespace.

7a36758

Use proper typed signal_sizes vector

d40c328

Turn warp size define more unique.

e56c8b3

Add comment why static_cast is explicitly there.

3309bf5

Name warp size define uniquely.

9b2d654

Comment that this is code for TEST_MIOPEN

622f720

Unfortunately the CUDA naming is ubiquitous and not removable.

lcskrishna and others added 3 commits August 31, 2018 13:17

Merge branch 'dockerpackages' of https://github.com/ROCmSoftwarePlatf…

4285362

…orm/pytorch

skipping all the datatypes with skipIfRocm

21380d8

Merge pull request #166 from lcskrishna/cl/code-review

734d988

test_cuda - skipping all the types with skipIfRocm

ezyang approved these changes Sep 2, 2018

View reviewed changes

facebook-github-bot reviewed Sep 2, 2018

View reviewed changes

facebook-github-bot closed this in 33c7cc1 Sep 2, 2018

iotamudelta deleted the dockerpackages branch October 25, 2018 18:11

ezyang added open source merged labels Jun 24, 2019

Conversation

iotamudelta commented Aug 27, 2018

Uh oh!

This comment was marked as off-topic.

Uh oh!

This comment was marked as off-topic.

Uh oh!

This comment was marked as off-topic.

Uh oh!

This comment was marked as off-topic.

Uh oh!

This comment was marked as off-topic.

Uh oh!

This comment was marked as off-topic.

Uh oh!

This comment was marked as off-topic.

Uh oh!

This comment was marked as off-topic.

Uh oh!

This comment was marked as off-topic.

Uh oh!

This comment was marked as off-topic.

Uh oh!

This comment was marked as off-topic.

Uh oh!

This comment was marked as off-topic.

Uh oh!

This comment was marked as off-topic.

Uh oh!

This comment was marked as off-topic.

Uh oh!

This comment was marked as off-topic.

Uh oh!

This comment was marked as off-topic.

Uh oh!

This comment was marked as off-topic.

Uh oh!

This comment was marked as off-topic.

Uh oh!

ezyang left a comment

Choose a reason for hiding this comment

Uh oh!

ezyang commented Aug 31, 2018

Uh oh!

facebook-github-bot left a comment

Choose a reason for hiding this comment

Uh oh!

iotamudelta commented Aug 31, 2018

Uh oh!

facebook-github-bot left a comment

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

7 participants