search

conda-forge / cupy-feedstock

A conda-smithy repository for cupy.
BSD 3-Clause "New" or "Revised" License
5 stars 23 forks source link

Using CUB package (instead of vendoring) #60

Closed jakirkham closed 4 years ago

jakirkham commented 4 years ago

Recently cub got added to conda-forge. It would make sense (once we start building with cub again) to use the cub package instead of downloading our own copy.

leofang commented 4 years ago

I am not sure why we wanna use it, could you elaborate @jakirkham?

For CUDA < 11, if CUB is bundled (cupy/cupy#2584) then we just use the bundled version.

For CUDA 11+, CUB is version-locked to Thrust, and Bryce has recommended to only use the version that comes with Thrust from the CUDA Toolkit, which I think would be available in the docker image.

jakirkham commented 4 years ago

We could use selectors to grab it when appropriate and skip otherwise.

leofang commented 4 years ago

By "when appropriate" you meant depending on CuPy and CUDA versions?

jakirkham commented 4 years ago

Yeah that's right.

leofang commented 4 years ago

By the way, with https://github.com/nsls-ii-forge/cupy-feedstock I got a lot more confident of brining back CUB. It's enabled there (with CUB 1.8.0) and full tests are run. Only a few minor, irrelevant issues occurred.

leofang commented 4 years ago

Opened an issue for the cub package: https://github.com/conda-forge/cub-feedstock/issues/4.

jakirkham commented 4 years ago

Great! Glad to hear CUB support is working. 😄

I think we really need to get your PR ( https://github.com/cupy/cupy/pull/2584 ) across the line. Happy to help however I can.

Ok, thanks will take a look 🙂

leofang commented 4 years ago

I think we really need to get your PR ( cupy/cupy#2584 ) across the line.

Yeah I agree. We probably discussed somewhere else that https://github.com/chainer/chainer-test/pull/564 seems to be the only blocker.

Once it's merged, we can start discussing how to address the Thrust-CUB locking issue in CuPy. For example, we could order the search path like this (which is currently done): 

-I/usr/local/cuda/include -Icupy/core/include/cupy

so that for CUDA 11+ we use those from the CUDA Toolkit, and for CUDA <11 we use Thrust from Toolkit + bundled CUB (which is considered interoperable). We could also just ignore the bundled CUB for CUDA 11+. As for this feedstock, it might be easier if we just use CF's cub and thrust as you suggested? We have more time to work out these thoughts.

jakirkham commented 4 years ago

Is that still a concern given PfN is moving away from Chainer to PyTorch?

jakirkham commented 4 years ago

Yeah I think those packages can become empty packages for CUDA 11+. So it will make it easy for us to depend on them for different CUDA builds.

leofang commented 4 years ago

Is that still a concern given PfN is moving away from Chainer to PyTorch?

Right, because Chainer-test sets up the CI for both Chainer and CuPy.

leofang commented 4 years ago

Hi @jakirkham The discussion in cupy/cupy#3445 concludes that CuPy will no longer allow user-supplied CUB location. The official policy is to use the bundled CUB -- either with CuPy or with CUDA (11+). I think it makes sense because then those combinations will be tested by CuPy's CI, whereas conda's CUB won't be tested anywhere.

Given that, I'm gonna close this issue. Please feel free to jump in there if you find we overlooked anything in the discussion. Then, we can consider reopening this one. Thanks!