gschramm / parallelproj

code for parallel TOF and NONTOF projections
MIT License
30 stars 9 forks source link

`cp` not found #88

Closed lukepolson closed 2 months ago

lukepolson commented 2 months ago

If torch is installed but cupy is not installed, then is_cuda_array(img) will trigger true in line 482 of src/parallelproj/backend.py but then it will fail since cp is not imported. Linked to pull request #87

gschramm commented 2 months ago

Hi Luke, thanks for reporting this issue.

  1. Can you confirm that is_cuda_array(torch.ones((2,2,2)), device = "cpu") = True. It is supposed to return False. The idea of is_cuda_array is to return True for cupy arrays and torch gpu tensors (all arrays that live on a cuda gpu) and False otherwise.

  2. I agree that if people want to use torch with gpu tensors, than cupy should be installed as well. But that should be fixed in the installation instructions.

Georg

lukepolson commented 2 months ago

The issue is that you can have torch tensors on GPU even if cupy is not installed. In that case, all the if statements (the ones I've editted) return true, but cp is never defined because line 29 yields cupy_enabled as False.

gschramm commented 2 months ago

I agree. But the solution is to have better install instructions mentioning that if you want to you torch gpu arrays, you need to install cupy as well. In the case that people use torch on non-cuda systems (like I sometimes do), there is no need to install cupy (and parallelproj works then)

lukepolson commented 2 months ago

Ah I see what you're saying. The pull request should probably go along with the change in installation instructions, just in case somebody has PyTorch but not Cupy. In the present case, they'll get a cp not defined error, but after the pull request, it will simply run on cpu. Maybe I can add some sort of warning in the pull request if you think that would be best, e.g. "Warning, CuPy install needed for GPU implementation, currently running on CPU", or something like that

gschramm commented 2 months ago

There are many scenarios where people might run pytorch without cupy (e.g. me on my macbook using pytorch CPU tensors). So using pytorch without cupy is fine - just not in the case when you want to use pytorch CUDA tensors.

I will update the install instructions stating that if people want to use pytorch CUDA tensors, cupy needs to be there as well.

lukepolson commented 2 months ago

Ah apologies I mis-spoke on the last comment, what I meant is if somebody has pytorch-GPU installed without CuPy: in this case the library breaks. By the way, I suspect many potential users will be in this category. But yes, I see what you're saying!

gschramm commented 2 months ago

Exactly. So we need to make sure that the install instructiongs clearly say that if you want torch CUDA, you also need cupy.

lukepolson commented 2 months ago

Sure, but I would also suggest updating the code with #87 in case somebody installs without installing cupy. And maybe add some sort of warning as well. (A scenario I can think of is that somebody installs parallelproj, then later installs torch for some machine learning or something, and then finds that parallelproj stops running).

By the way Georg, I think the ultimately solution here is to simply publish to PyPi ;). I'd be more than happy to contribute to this via a github workflow that builds binary wheels for the available platforms and auto publishes (similar to what numpy does here).

gschramm commented 2 months ago

(1) #87 does not work. If the input is an CUDA image / tensor, we cannot run on the CPU. You first had to convert to a CPU array / tensor to do that and you probably don't want that. If you are projecting CUDA arrays directly, and you don't have cupy installed, an expection must be thrown. But the best solution is to be more clear in the install instruction.

Note that using torch on a cuda system, but sticking to CPU tensors still works and is a valid (but not super useful) option.

(2) Getting all the correct cuda dependecies installed is far from trivial (numpy does not have that problem). conda-forge automatically solves that for you by building against multiple cuda versions and making sure that the correct cudatoolkit gets installed (which is not availalble on pypi).

Do you know how to solve all the cuda depedency issues using pypi? It is not only about the builds, but also about making sure that all (non-python) depdencies get correctly installed. The conda-forge people have done a great job to solve this.

lukepolson commented 2 months ago

Hi Georg, I agree that updating the install instruction is important. But keep in mind that the environment can change post install, and this might cause the code to break.

For (2), it looks like CuPy (for example) builds various wheels for different cuda versions, as you mention what is done on conda. They do this through a separate library for building called cuda-release-tools. After a brief look, it seems like they manually set up different OS/CUDA configurations and build each wheel that way. Definitely not a simple solution, especially given (as you said) that the conda-forge people have solved this.

gschramm commented 2 months ago

"environment can change post install", true but people need to track their depedencies and afaik, any decent package manager won't delete packages. So installing toch and cupy together with parallelproj from the start is the key.

Note that package resolution can be difficult which is why changing the env. sometimes is tricky / creates conflicts.

gschramm commented 2 months ago

@lukepolson : see #89 and https://parallelproj--89.org.readthedocs.build/en/89/installation.html (new read the docs build based on PR). What do you think about the install instructions now?

lukepolson commented 2 months ago

Hi Georg, it seems much better now as it enforces cupy to be installed. Thanks :). Btw, I also added explicit install instructions for parallelproj on PyTomography's documentation in order to get it working:

https://pytomography.readthedocs.io/en/latest/install.html