Enable ROCm RNN-T Loss #2485

jpvillam-amd · 2022-06-14T19:25:11Z

Added HIPIFY code and small changes for ROCm. Targeting RNN-T loss.

mthrok · 2022-06-15T14:53:21Z

Thanks for the contribution. Can you rebase on the latest main branch?
We had a bunch of CI breakage but it's been fixed on main branch.

jpvillam-amd · 2022-06-15T15:12:45Z

Hi @jpvillam-amd

Thanks for the contribution. Can you rebase on the latest main branch? We had a bunch of CI breakage but it's been fixed on main branch.

Sure thing

mthrok · 2022-06-15T16:17:32Z

CircleCI is not running. Let me close and reopen to send the web hook again.

malfet

Please guard USE_ROCM with CMake version check (as TorchAudio should be builder on systems with CMake older than 3.21)

Also, can you please add an explanation why explicit #ifdef __HIP_PLATFORM_AMD__ must be explicitly added .cu files? I.e. why can't hipify module take care of those mechanistically changes?

malfet · 2022-06-15T22:10:34Z

CMakeLists.txt

@@ -85,6 +85,10 @@ if(USE_CUDA)
  enable_language(CUDA)
 endif()

+if(USE_ROCM)
+  enable_language(HIP)


HIP language is only available since CMake-3.21(see https://cmake.org/cmake/help/v3.21/command/enable_language.html), so please make USE_ROCM conditional on 3.21 CMake version

The need for enable_language(HIP) was removed in the latest commit.

Reverted the commit that refactored the HIP cmake integration -- will revisit in later PR.

malfet · 2022-06-15T22:13:10Z

torchaudio/csrc/rnnt/options.h

+#ifdef USE_ROCM
+  // the stream to launch kernels in when using GPU.
+  hipStream_t stream_;
 #endif


Hmm, can we just add #define cudaStream_t hipStream_t under first USE_ROCM and then we would not need to duplicate the code here?

typedef was added. Please re-review.

Added check for rocm language and CMake 3.21 Combined stream types between CUDA and HIP.

…nnt_merge Upstream audio rocm rnnt merge

facebook-github-bot · 2023-02-23T15:52:26Z

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

jithunnair-amd · 2023-03-07T17:22:46Z

@malfet @mthrok It's not clear to me at all if these CI failures need any action from our end, since the failures are CUDA related. Can you please let us know what the next step should be here? Do we need another rebase?

nateanl · 2023-03-07T20:17:31Z

cc @mthrok can you review this pr?

jithunnair-amd · 2023-07-13T06:22:09Z

@mthrok @malfet Can you please review?

facebook-github-bot · 2023-07-13T14:10:10Z

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

mthrok · 2023-07-13T14:11:34Z

@jithunnair-amd

Should we be concerned about ROCm build job failures??

libtorchaudio.so: undefined symbol: __kmpc_fork_call
https://github.com/pytorch/audio/actions/runs/5539782179/jobs/10111098244?pr=2485
https://github.com/pytorch/audio/actions/runs/5539782179/jobs/10111098349?pr=2485

jeffdaily · 2023-07-13T18:05:14Z

@jithunnair-amd

Should we be concerned about ROCm build job failures??

libtorchaudio.so: undefined symbol: __kmpc_fork_call https://github.com/pytorch/audio/actions/runs/5539782179/jobs/10111098244?pr=2485 https://github.com/pytorch/audio/actions/runs/5539782179/jobs/10111098349?pr=2485

Yes, we need to fix those before we can land this PR.

mthrok · 2023-07-14T20:16:45Z

@jithunnair-amd
Should we be concerned about ROCm build job failures??
libtorchaudio.so: undefined symbol: __kmpc_fork_call https://github.com/pytorch/audio/actions/runs/5539782179/jobs/10111098244?pr=2485 https://github.com/pytorch/audio/actions/runs/5539782179/jobs/10111098349?pr=2485

Yes, we need to fix those before we can land this PR.

@jeffdaily @jithunnair-amd Got it. Would you be able to look into them?

mthrok · 2023-08-16T16:27:51Z

@jeffdaily @jithunnair-amd

Seems like there was ROCm CI migration. Could you rebase or merge upstream? If possible, I would like to include this PR for the upcoming release.

This reverts commit 5af9613.

jeffdaily · 2023-08-17T17:37:14Z

@jeffdaily @jithunnair-amd

Seems like there was ROCm CI migration. Could you rebase or merge upstream? If possible, I would like to include this PR for the upcoming release.

Done. @mthrok

jeffdaily · 2023-08-17T17:41:04Z

Note that the CMakeLists.txt refactor was reverted. Let's first focus on landing this PR that adds RNN-T for ROCm, then a follow-up PR that modernizes the CMake files.

jeffdaily · 2023-08-18T20:39:33Z

Please guard USE_ROCM with CMake version check (as TorchAudio should be builder on systems with CMake older than 3.21)

Also, can you please add an explanation why explicit #ifdef __HIP_PLATFORM_AMD__ must be explicitly added .cu files? I.e. why can't hipify module take care of those mechanistically changes?

Would you be okay with accepting this PR as-is and then a follow-up PR that revisits the hipify strategy here? It would be nice to get the feature in. The hipify and CMakeLists.txt refactor was causing more trouble than expected, so it was backed out of this PR.

mthrok · 2023-08-18T21:08:13Z

Please guard USE_ROCM with CMake version check (as TorchAudio should be builder on systems with CMake older than 3.21)
Also, can you please add an explanation why explicit #ifdef __HIP_PLATFORM_AMD__ must be explicitly added .cu files? I.e. why can't hipify module take care of those mechanistically changes?

Would you be okay with accepting this PR as-is and then a follow-up PR that revisits the hipify strategy here? It would be nice to get the feature in. The hipify and CMakeLists.txt refactor was causing more trouble than expected, so it was backed out of this PR.

@jeffdaily Yes, build CI jobs were green, so I will merge this.

facebook-github-bot · 2023-08-18T21:09:08Z

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

facebook-github-bot · 2023-08-19T05:09:51Z

@mthrok merged this pull request in c593961.

jithunnair-amd · 2023-08-24T10:33:14Z

@mthrok This PR introduces a dependency on libomp.so which is available with a standard ROCm installation in /opt/rocm/llvm/lib/libomp.so. However, since it's not packaged with the torchaudio wheel currently, and is also not packaged with the torch wheels, it results in an import-time failure for torchaudio:

>>> import torchaudio
Traceback (most recent call last):
  File "<stdin>", line 1, in <module>
  File "/usr/local/lib/python3.8/dist-packages/torchaudio/__init__.py", line 1, in <module>
    from . import (  # noqa: F401
  File "/usr/local/lib/python3.8/dist-packages/torchaudio/_extension/__init__.py", line 45, in <module>
    _load_lib("libtorchaudio")
  File "/usr/local/lib/python3.8/dist-packages/torchaudio/_extension/utils.py", line 64, in _load_lib
    torch.ops.load_library(path)
  File "/usr/local/lib/python3.8/dist-packages/torch/_ops.py", line 839, in load_library
    ctypes.CDLL(path)
  File "/usr/lib/python3.8/ctypes/__init__.py", line 373, in __init__
    self._handle = _dlopen(self._name, mode)
OSError: libomp.so: cannot open shared object file: No such file or directory

I'm investigating how to package the libomp.so with the torchaudio wheel for ROCm. If there are any existing instances where you do the same for any build of torchaudio (ROCm or otherwise), please point me to it, so I can re-use the same flow.

cc @jeffdaily @atalman

atalman · 2023-08-29T00:50:58Z

@jeffdaily @jithunnair-amd how critical this PR is ? maybe we can just rever it ?

This reverts commit c593961.

Summary: This reverts commit c593961. Unblock 2.1.0 rc Pull Request resolved: #3586 Reviewed By: osalpekar Differential Revision: D48842032 Pulled By: atalman fbshipit-source-id: bbdf9e45c9aa5fde00f315a2ff491ed050bc1707

mthrok · 2023-09-07T15:40:31Z

Hi @jeffdaily @jithunnair-amd - So this PR was reverted due to the packaging issue. I wanted to have this in the upcoming release and I do welcome the re-attempt. As to make the process faster for the future, I invited @jeffdaily for write permission. I hope we can continue and iterate on this feature. Regards

This reverts commit 49d3eec.

pytorch-bot bot added the module: rocm label Jun 14, 2022

facebook-github-bot added the CLA Signed label Jun 14, 2022

jpvillam-amd force-pushed the rocm_rnnt branch from 4d15e8c to d46a05d Compare June 15, 2022 15:21

mthrok closed this Jun 15, 2022

mthrok reopened this Jun 15, 2022

malfet requested changes Jun 15, 2022

View reviewed changes

jpvillam-amd force-pushed the rocm_rnnt branch 2 times, most recently from 1cbcda2 to eeb8367 Compare June 23, 2022 15:46

jpvillam-amd force-pushed the rocm_rnnt branch 2 times, most recently from 332e298 to 45ec17a Compare July 6, 2022 00:00

jpvillam-amd added 4 commits July 5, 2022 19:00

Hipify rnnt GPU code

587eae1

Changed submodule to https for consistancy

a85fa41

[ROCm] Review comments

7769c21

Added check for rocm language and CMake 3.21 Combined stream types between CUDA and HIP.

Small whitespace changes

f7c3d42

jpvillam-amd force-pushed the rocm_rnnt branch from 45ec17a to 597a735 Compare July 6, 2022 16:20

Force OpenMP to rocm version.

597a735

mthrok self-assigned this Jul 19, 2022

pnunna93 and others added 2 commits December 5, 2022 12:53

Merge remote-tracking branch 'upstream/main' into rnnt_upstream_merge

04c3208

Merge pull request #7 from ROCmSoftwarePlatform/upstream_audio_rocm_r…

54822da

…nnt_merge Upstream audio rocm rnnt merge

mthrok requested a review from malfet December 13, 2022 16:44

jithunnair-amd and others added 3 commits February 17, 2023 16:27

Merge branch 'main' into rocm_rnnt

27ad25d

Set libomp.so link library path

c592261

Merge branch 'main' into rocm_rnnt

e8ad94c

Update library lists for ROCM in cmake file

feb6559

mthrok approved these changes Jul 13, 2023

View reviewed changes

jeffdaily added 2 commits August 17, 2023 17:21

Revert "refactor CMakeLists.txt for ROCm"

efa5448

This reverts commit 5af9613.

Merge remote-tracking branch 'upstream/main' into rocm_rnnt

a8b062c

Merge branch 'main' into rocm_rnnt

596f7e1

facebook-github-bot closed this in c593961 Aug 19, 2023

facebook-github-bot added the Merged label Aug 19, 2023

mthrok added new feature and removed improvement labels Aug 19, 2023

jithunnair-amd mentioned this pull request Aug 24, 2023

Package libomp.so with torchaudio wheel for ROCm #3575

Open

atalman added a commit to atalman/audio that referenced this pull request Aug 30, 2023

Revert "Enable ROCm RNN-T Loss (pytorch#2485)"

68c3b0e

This reverts commit c593961.

pruthvistony added a commit to ROCm/audio that referenced this pull request Nov 4, 2023

Revert "Revert "Enable ROCm RNN-T Loss (pytorch#2485)" (pytorch#3586)"

d3deb2e

This reverts commit 49d3eec.

pruthvistony added a commit to ROCm/audio that referenced this pull request Nov 19, 2023

Revert "Revert "Enable ROCm RNN-T Loss (pytorch#2485)" (pytorch#3586)"

1d54f55

This reverts commit 49d3eec.

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Enable ROCm RNN-T Loss #2485

Enable ROCm RNN-T Loss #2485

jpvillam-amd commented Jun 14, 2022 •

edited

Loading

mthrok commented Jun 15, 2022

jpvillam-amd commented Jun 15, 2022

mthrok commented Jun 15, 2022

malfet left a comment

malfet Jun 15, 2022

jeffdaily Jun 14, 2023

jeffdaily Aug 17, 2023

malfet Jun 15, 2022

jeffdaily Aug 17, 2023

facebook-github-bot commented Feb 23, 2023

jithunnair-amd commented Mar 7, 2023 •

edited

Loading

nateanl commented Mar 7, 2023

jithunnair-amd commented Jul 13, 2023

facebook-github-bot commented Jul 13, 2023

mthrok commented Jul 13, 2023 •

edited

Loading

jeffdaily commented Jul 13, 2023

mthrok commented Jul 14, 2023

mthrok commented Aug 16, 2023

jeffdaily commented Aug 17, 2023

jeffdaily commented Aug 17, 2023

jeffdaily commented Aug 18, 2023

mthrok commented Aug 18, 2023

facebook-github-bot commented Aug 18, 2023

facebook-github-bot commented Aug 19, 2023

jithunnair-amd commented Aug 24, 2023

atalman commented Aug 29, 2023

mthrok commented Sep 7, 2023

Enable ROCm RNN-T Loss #2485

Enable ROCm RNN-T Loss #2485

Conversation

jpvillam-amd commented Jun 14, 2022 • edited Loading

mthrok commented Jun 15, 2022

jpvillam-amd commented Jun 15, 2022

mthrok commented Jun 15, 2022

malfet left a comment

Choose a reason for hiding this comment

malfet Jun 15, 2022

Choose a reason for hiding this comment

jeffdaily Jun 14, 2023

Choose a reason for hiding this comment

jeffdaily Aug 17, 2023

Choose a reason for hiding this comment

malfet Jun 15, 2022

Choose a reason for hiding this comment

jeffdaily Aug 17, 2023

Choose a reason for hiding this comment

facebook-github-bot commented Feb 23, 2023

jithunnair-amd commented Mar 7, 2023 • edited Loading

nateanl commented Mar 7, 2023

jithunnair-amd commented Jul 13, 2023

facebook-github-bot commented Jul 13, 2023

mthrok commented Jul 13, 2023 • edited Loading

jeffdaily commented Jul 13, 2023

mthrok commented Jul 14, 2023

mthrok commented Aug 16, 2023

jeffdaily commented Aug 17, 2023

jeffdaily commented Aug 17, 2023

jeffdaily commented Aug 18, 2023

mthrok commented Aug 18, 2023

facebook-github-bot commented Aug 18, 2023

facebook-github-bot commented Aug 19, 2023

jithunnair-amd commented Aug 24, 2023

atalman commented Aug 29, 2023

mthrok commented Sep 7, 2023

jpvillam-amd commented Jun 14, 2022 •

edited

Loading

jithunnair-amd commented Mar 7, 2023 •

edited

Loading

mthrok commented Jul 13, 2023 •

edited

Loading