Windows compatibility #29
Conversation
|
Might close Issue #18 |
| @@ -1,14 +1,14 @@ | |||
| from setuptools import setup | |||
| from torch.utils.cpp_extension import BuildExtension, CUDAExtension | |||
| import os | |||
| from make import build | |||
There was a problem hiding this comment.
The setup is used also in the CI/CD pipeline to create the precompiled packages so I would keep both the Linux version with the make.py file and the new Windows version ad add an if os.name=='posix' to choose between the two versions of the code
There was a problem hiding this comment.
Ah I see. I will add the if os.name=='posix'.
Since this will include setting cuda_home = os.getenv("CUDA_HOME", "/usr/local/cuda") again, what do you think about replacing this with from torch.utils.cpp_extension import CUDA_HOME? On Manjaro, for example, the path to cuda is CUDA_PATH="/opt/cuda" and CUDA_HOME is not set. The torch import would take care of that.
There was a problem hiding this comment.
Perfect, thanks.
Replacing the CUDA_HOME definition seems a good idea!
src/backprojection.cu
Outdated
| __device__ T toType(float); | ||
|
|
||
| template<> | ||
| __device__ float toType(float f) |
There was a problem hiding this comment.
Why are these required?
There was a problem hiding this comment.
The cuda compiler on windows doesn't allow implicit casts from 32bit floats to 16bit floats. In radon_backward_kernel, for example, accumulator is a 32bit float array, but output might be 16bit float. toType casts 32bit floats to the target type.
There was a problem hiding this comment.
I just found out that CUDAExtension defines __CUDA_NO_HALF_CONVERSIONS__ for nvcc, which deactivates these implicit conversions. Since you don't compile the cuda files with CUDAExtension, you didn't get these errors.
There are 2 possibilities now:
- keep the templated casts in the cuda files
- undefine
__CUDA_NO_HALF_CONVERSIONS__forCUDAExtension
There was a problem hiding this comment.
I am ok with keeping the templated casts. Please put them in a separate file in order to avoid duplication of code
|
Thank you very much for taking the time to do this PR, I really appreciate it. |
|
I haven't checked the code updates on Windows yet. I will let you know if it still works soon. |
|
Works on my Windows pc. I'm not sure why the CI build fails, though. Seems like it cannot find numpy for python 3.6 |
This should make building the package work on both Linux and Windows.
Probably needs a bit of cleaning up (the templated function
toTypeis redundantly defined in backprojection.cu and forward.cu, and some code is not used any more, e.g. make.py and ptx_annotation.py).Tested on Windows 10 (CUDA 11.4, GTX 1060), Manjaro (CUDA 11.4, RTX A6000) and Ubuntu 18.04 (CUDA 11.4, Tesla V100).