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

Enable audio build on Windows with CUDA #1787

Merged
merged 2 commits into from
Sep 24, 2021
Merged

Enable audio build on Windows with CUDA #1787

merged 2 commits into from
Sep 24, 2021

Conversation

mszhanyi
Copy link
Contributor

@mszhanyi mszhanyi commented Sep 23, 2021

With this PR, audio could be build on Windows with CUDA now.
It's from #1777
I've verified it locally.

@mszhanyi
Copy link
Contributor Author

@mthrok
Did you miss removing something related in this commit.
0f82217#diff-1e7de1ae2d059d21e1dd75d5812d5a34b0222cef273b7c3a2af62eb747f9d20a
image

without this, there's one error locally.
image
cc @malfet

Copy link
Contributor

@malfet malfet left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM if CI is green

Comment on lines +35 to +37
if(USE_CUDA)
set(CMAKE_CXX_STANDARD 17)
endif()
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

But that wouldn't work when TorchAudio is compiled with CUDA-10.2, would it?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It would.
https://app.circleci.com/pipelines/github/pytorch/audio/7396/workflows/058b3c2c-8421-40c2-915c-a8441deac091/jobs/339245.
It's a Visual Studio Issue. I verified that another workaround is to downgrade the Visual Studio to 16.8.5 which PyTorch Builder is using.
But I think user experience might better that the user doesn't need to select the compiler with the specific version.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The root cause is a regression in VS. microsoft/STL#2020

@mthrok
Copy link
Collaborator

mthrok commented Sep 23, 2021

@mthrok
Did you miss removing something related in this commit.
0f82217#diff-1e7de1ae2d059d21e1dd75d5812d5a34b0222cef273b7c3a2af62eb747f9d20a
image

without this, there's one error locally.
image
cc @malfet

The deletion itself is as-intended. The option was removed because libtorchaudio should be built all the time. libtorchaudio is where all the C++ implementation that do not require libpython dependency is placed.

What I might have missed is to check if the extension .dll is/should be recognized by setuptools.

The logic there is that;

  1. python setup.py build_ext invokes the compilation of extension module, expecting torchaudio._torchaudio and torchaudio.lib.libtorchaudio.
  2. cmake builds libtorchaudio and _torchaudio.
    For Linux and macOS they are libtorchaudio.so and _torchaudio.so. For Windows _torchaudio has to have pyd extension but I am not sure about libtorchaudio as it is C++ library without any Python component.
  3. python setup.py install picks up the compiled modules from build directory and place it in the installation location.
    Your error indicates that setuptools expect the libtorchaudio to have extension .pyd but, currently CMake produces libtorchaudio.dll thus setuptools cannot find libtorchaudio.

I am not sure what's the right fix for Windows, but one way it might work is to set the extension of libtorchaudio to pyd as well. You can try adding this for libtorchaudio here, then change this line from dll to pyd.

@mszhanyi
Copy link
Contributor Author

I am not sure what's the right fix for Windows, but one way it might work is to set the extension of libtorchaudio to pyd as well. You can try adding this for libtorchaudio here, then change this line from dll to pyd.

Thanks @mthrok . It works now. I've submitted another PR #1788 for these change.
Once #1788 is merged, It can be merged.

@mthrok mthrok merged commit cf0adb2 into pytorch:main Sep 24, 2021
mthrok pushed a commit to mthrok/audio that referenced this pull request Dec 13, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants