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.

Sign up for GitHub

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

Jump to bottom

[ROCM] add rocm support #1411

Merged
merged 6 commits into from
Apr 2, 2021
Merged

[ROCM] add rocm support #1411

merged 6 commits into from
Apr 2, 2021

Conversation

micmelesse
Copy link
Contributor

@micmelesse micmelesse commented Mar 29, 2021 •
edited
Loading

This PR adds support for building pytorch audio in ROCM. We currently disable 5 tests. There is still more work to be done but with the changes in this PR, we can build and test pytorch audio on ROCM.

@micmelesse micmelesse changed the title add rocm support [ROCM] add rocm support Mar 29, 2021
@micmelesse
Copy link
Contributor Author

@facebook-github-bot retest

@mthrok mthrok requested review from seemethere and malfet March 30, 2021 13:40
mthrok
mthrok reviewed Mar 30, 2021
View reviewed changes
Copy link
Collaborator

@mthrok mthrok 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 contribution.
As I have no experience/knowledge in ROCm, I will leave the review of the ROCm part to those with expertise, but one naive question I got is the following.

At this moment, torchaudio only contains Python code and custom CPU code. There is no custom CUDA code. (although we plan to do add some CUDA kernel)
I imagined that we could just build torchaudio with C++ complier and the corresponding PyTorch binary. Why does it need to ROCm at build time?

test/torchaudio_unittest/backend/soundfile/save_test.py Outdated Show resolved Hide resolved
test/torchaudio_unittest/common_utils/case_utils.py Outdated Show resolved Hide resolved
test/torchaudio_unittest/functional/torchscript_consistency_impl.py Outdated Show resolved Hide resolved
@micmelesse
Copy link
Contributor Author

Thanks for the contribution.
As I have no experience/knowledge in ROCm, I will leave the review of the ROCm part to those with expertise, but one naive question I got is the following.

At this moment, torchaudio only contains Python code and custom CPU code. There is no custom CUDA code. (although we plan to do add some CUDA kernel)
I imagined that we could just build torchaudio with C++ complier and the corresponding PyTorch binary. Why does it need to ROCm at build time?

@mthrok, the reason was because I encountered multiple cmake errors from various modules failing to find the path to various rocm libraries. I added the LoadHip cmake file and use it find all the various rocm libs.

@micmelesse
Copy link
Contributor Author

micmelesse commented Mar 30, 2021 •
edited
Loading

@mthrok What should I do about the failing circleci test, binary_macos_conda_py3.7. It seems to have nothing to do with my changes and I cannot rerun the test.

@mthrok
Copy link
Collaborator

mthrok commented Mar 30, 2021

@mthrok What should I do about the failing circleci test, binary_macos_conda_py3.7. It seems to have nothing to do with my changes and I cannot rerun the test.

Build issues are often caused by external reason or upstream issues so we can ignore it.

@micmelesse micmelesse marked this pull request as ready for review March 30, 2021 23:10
@mthrok
Copy link
Collaborator

mthrok commented Apr 2, 2021

Ideally, this should be tested and we should have ROCm build in our CI but that will be a lot of work and I do not want this PR to become stale for that reason, so I am going to merge this.

@mthrok mthrok merged commit a6cdd6c into pytorch:master Apr 2, 2021
yangarbiter added a commit to yangarbiter/audio that referenced this pull request Jul 12, 2021
@yangarbiter
Remove skipIfRocm from test_fileobj_flac in soundfile.save_test
c90d19c 
The test was broken from the beginning and for a reason unrelated to
ROCm, but was disabled for ROCm in pytorch#1411.
pytorch#1604 fixed the test so that we are re-enabling it.
yangarbiter added a commit to yangarbiter/audio that referenced this pull request Jul 12, 2021
@yangarbiter
Remove skipIfRocm from test_fileobj_flac in soundfile.save_test
6cdb9da 
The test was broken from the beginning and for a reason unrelated to
ROCm, but was disabled for ROCm in
pytorch#1411.
pytorch#1604
fixed the test so that we are re-enabling it.
yangarbiter added a commit that referenced this pull request Jul 12, 2021
@yangarbiter
Remove skipIfRocm from test_fileobj_flac in soundfile.save_test (#1626)
0e51320 
The test was broken from the beginning and for a reason unrelated to
ROCm, but was disabled for ROCm in #1411.
#1604 fixed the test so that we are re-enabling it.
nateanl pushed a commit to nateanl/audio that referenced this pull request Jul 28, 2021
@yangarbiter @nateanl
Remove skipIfRocm from test_fileobj_flac in soundfile.save_test (pyto…
27a28e5 
…rch#1626)

The test was broken from the beginning and for a reason unrelated to
ROCm, but was disabled for ROCm in pytorch#1411.
pytorch#1604 fixed the test so that we are re-enabling it.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@seemethere seemethere seemethere approved these changes

@mthrok mthrok mthrok approved these changes

@malfet malfet Awaiting requested review from malfet

Assignees
No one assigned
Labels
CLA Signed
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@micmelesse @mthrok @seemethere @facebook-github-bot