Skip to content
This repository was archived by the owner on Apr 7, 2023. It is now read-only.

How to deal with the version lock with Thrust? #4

Closed
leofang opened this issue May 29, 2020 · 17 comments · Fixed by #6
Closed

How to deal with the version lock with Thrust? #4

leofang opened this issue May 29, 2020 · 17 comments · Fixed by #6

Comments

@leofang
Copy link
Member

leofang commented May 29, 2020

Hi, @jakirkham told me this feedstock, thanks for making it live. Since newer CUB will be version-locked with Thrust (see the links below), I am a bit worried about using this feedstock and, in particular, whether things can/cannot be overridden. Can NVIDIA folks provide comments and suggestions?

refs:

  1. Adding CUB Version Check NVIDIA/thrust#1082
  2. https://github.com/NVlabs/cub/issues/182#issuecomment-630938376
  3. [REVIEW] fetch thrust/cub from github rapidsai/rmm#378 (comment)
@kkraus14
Copy link
Contributor

Version locking with thrust should already be handled by the thrust feedstock: https://github.com/conda-forge/thrust-feedstock/blob/master/recipe/meta.yaml#L16-L18

I think the bigger challenge is aligning to a CUDA toolkit version. I'm not sure if Thrust/CUB make any guarantees outside of the version that's shipped in the toolkit, but maybe @brycelelbach or someone from his team can comment here on how we could handle toolkit version compatibility.

@leofang
Copy link
Member Author

leofang commented May 29, 2020

Oh, OK, didn't know there's a thrust-feedstock. Thanks Keith!

But this only makes the problem more complicated! Say I install cub from CF but not thrust, then when I compile with nvcc, the local thrust (assuming the users have a local installation of CUDA Toolkit, not cudatoolkit from CF) discovered from standard include path may conflict with CF's cub.

@brycelelbach
Copy link

Today we only guarantee that Thrust/CUB works with the version of CUDA they ship with. Realistically they probably work with older versions too.

My recommendation is to either:

  • Pull your own version of Thrust and CUB to ensure they are compatible, or
  • Use Thrust and CUB from the Toolkit.

@jakirkham
Copy link
Member

cc @rongou

@leofang
Copy link
Member Author

leofang commented May 29, 2020

Pull your own version of Thrust and CUB to ensure they are compatible

Right, thanks Bryce. This was what you suggested to me in another thread.

For the purposes of both this feedstock and decoupling from what's available in a local CUDA installation, perhaps we need to run_export thrust so that the compatible cub and thrust are always installed together?

Use Thrust and CUB from the Toolkit

@brycelelbach Is it possible to add a pragma guard to CUB to detect if older Thrust is used? Something similar to the version lock implemented in Thrust, except that we don't lock? This way we provide some sort of compatibility guarantee for using newer CUB + older CUDA Toolkit.

@jakirkham
Copy link
Member

FWIW we can add selectors to control which version of Thrust and CUB is used with which CUDA versions. Also we can turn this into a metapackage for some versions of the CUDA Toolkit that contains it.

@rongou
Copy link
Contributor

rongou commented May 29, 2020

I added a note to the readme to tell people to install the thrust conda package.

I guess theoretically you could use cub without thrust, so I'm not sure we should add a runtime requirement to thrust from cub.

@leofang
Copy link
Member Author

leofang commented May 29, 2020

FWIW we can add selectors to control which version of Thrust and CUB is used with which CUDA versions. Also we can turn this into a metapackage for some versions of the CUDA Toolkit that contains it.

Oh, I was hoping that newer Thrust+CUB could work with the oldest CUDA we support (either 8.0 or 9.2), is it not the case?

Also, even if cudatoolkit is updated to CUDA 11, we still can't turn this into a metapackage, because it doesn't come with headers, just shared libraries.

@leofang
Copy link
Member Author

leofang commented May 29, 2020

I added a note to the readme to tell people to install the thrust conda package.

The problem is no one would read the readme on a feedstock.......Many conda users don't even know what a feedstock is. They just do conda install and expect things to work.

I guess theoretically you could use cub without thrust, so I'm not sure we should add a runtime requirement to thrust from cub.

According to @brycelelbach this is not true. CUB depends on Thrust: https://github.com/NVlabs/cub/issues/182#issuecomment-631272979. (I was surprised too!)

@leofang
Copy link
Member Author

leofang commented May 31, 2020

Thanks @rongou for quickly addressing this in #6. I guess we'll need to spread the wisdom that @brycelelbach left in #4 (comment) to downstream.

@brycelelbach
Copy link

brycelelbach commented May 31, 2020 via email

@rongou
Copy link
Contributor

rongou commented Jun 1, 2020

It's now taken care of in conda-forge packages. Thrust requires the same version of CUB, and CUB requires the same version of thrust. If you install either one through conda, you should get both.

@jakirkham
Copy link
Member

I think we should use run_constrained instead of run to restrict the dependency. Otherwise we will run into issues upgrading due to the cycle.

Alternatively we could try merging these into the same feedstock and using split packages to produce both packages as part of the build.

@rongou
Copy link
Contributor

rongou commented Jun 1, 2020

I installed 1.9.9 and then ran an update. It seems to work:

$ conda update --all
Collecting package metadata (current_repodata.json): done
Solving environment: done

## Package Plan ##

  environment location: /home/rou/miniconda3/envs/foo


The following packages will be downloaded:

    package                    |            build
    ---------------------------|-----------------
    cub-1.9.10                 |                2         170 KB  conda-forge
    thrust-1.9.10              |                0         452 KB  conda-forge
    ------------------------------------------------------------
                                           Total:         622 KB

The following packages will be UPDATED:

  cub                                               1.9.9-0 --> 1.9.10-2
  thrust                                            1.9.9-0 --> 1.9.10-0


Proceed ([y]/n)? 


Downloading and Extracting Packages
thrust-1.9.10        | 452 KB    | ######################################################################################################################################################################## | 100% 
cub-1.9.10           | 170 KB    | ######################################################################################################################################################################## | 100% 
Preparing transaction: done
Verifying transaction: done
Executing transaction: done

@jakirkham
Copy link
Member

Sure it works now. The concerning I'm trying to raise is, what happens when we need to publish new packages for a new version?

@rongou
Copy link
Contributor

rongou commented Jun 4, 2020

I played around with subpackages, but looks like we can't specify cyclic dependencies between subpackages. Also there is only one about section, so we are effectively combining thrust and cub into one package, which might be confusing.

With the current approach, publishing a new release of thrust and cub is not atomic, but if they are close enough, perhaps it won't cause too much disruption.

Feel free to send a PR if you have a better solution.

@jakirkham
Copy link
Member

It's possible to have multiple outputs with different about sections. Basically any field that can be used normally, can be used with split packages.

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging a pull request may close this issue.

5 participants