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

Use nvcomp conda package. #13566

Merged
merged 6 commits into from
Jun 26, 2023
Merged

Conversation

bdice
Copy link
Contributor

@bdice bdice commented Jun 14, 2023

Description

This PR uses conda-forge packages of nvcomp rather than fetching a tarball. This means that the nvcomp binary should not be shipped in the libcudf conda package, but is instead listed as a dependency. This will reduce libcudf's conda package size.

Addresses part of #13230.

Checklist

  • I am familiar with the Contributing Guidelines.
  • New or existing tests cover these changes.
  • The documentation is up to date with these changes.

@github-actions github-actions bot added the conda label Jun 14, 2023
@bdice
Copy link
Contributor Author

bdice commented Jun 14, 2023

@robertmaynard @vyasr When I built this locally, it looked like CMake finds a local package of nvcomp, but the resulting libcudf conda package still ships the nvcomp library's contents inside of it. Do you think this could be an issue in rapids-cmake that fetches the proprietary binary even if a local package is found? I was a little unsure what might cause this behavior, since moving from CPM to a conda dependency usually fixes repackaging issues.

@vyasr
Copy link
Contributor

vyasr commented Jun 14, 2023

I'll touch base with you offline to repro the problem so that I can help you investigate.

@vyasr
Copy link
Contributor

vyasr commented Jun 14, 2023

My first guess for what's happening here is that the binaries for nvcomp are being downloaded prior to the CPMFindPackage call and as a result the downloaded package is always being found (either because it's earlier in the search path or because nvcomp_DIR is set) before the one in the PREFIX. That would explain why CMake is finding a local package but it's still getting bundled.

Creating a local env to test.

@robertmaynard
Copy link
Contributor

robertmaynard commented Jun 14, 2023

My first guess for what's happening here is that the binaries for nvcomp are being downloaded prior to the CPMFindPackage call and as a result the downloaded package is always being found (either because it's earlier in the search path or because nvcomp_DIR is set) before the one in the PREFIX. That would explain why CMake is finding a local package but it's still getting bundled.

That is exactly what is happening. The current logic around proprietary_binary means that it is always used when requested.
We could change/extend the semantics of this in rapids-cmake.

Edit:
The proprietary_binary was a way that we could ensure that a local existing version wouldn't be used but the requested pre-built version would be. So now we need to add a json control flag to proprietary_binary to determine if it should always be used or if a local version is sufficient.

@vyasr vyasr added improvement Improvement / enhancement to an existing function non-breaking Non-breaking change labels Jun 14, 2023
@vyasr
Copy link
Contributor

vyasr commented Jun 14, 2023

@robertmaynard my POC solution was to just insert a find_package call for nvcomp prior to the proprietary binary logic so that we can check if one exists before downloading. What you're suggesting sounds like a more explicit, user-controlled version of that where the caller specifies whether or not to allow local version usage. In that scenario, though, you'd still probably want the proprietary binary used if no local version is found. Would the effective solution then be that if the flag is set the proprietary binary logic instead adds the download dir to the end of the search path instead of the beginning so that the prefix dirs are searched first? Ideally I would have thought we would also want to avoid downloading altogether in that situation, which is why I opted for the solution I had, but maybe it's OK to always download and just not use?

@bdice
Copy link
Contributor Author

bdice commented Jun 14, 2023

We should avoid downloading the proprietary binary if a local package exists.

@robertmaynard
Copy link
Contributor

my POC solution was to just insert a find_package call for nvcomp prior to the proprietary binary logic so that we can check if one exists before downloading. What you're suggesting sounds like a more explicit, user-controlled version of that where the caller specifies whether or not to allow local version usage.

My proposal would be that rapids-cmake proprietary_binary json entry would obey the existing always_download key. So if an override contains:

    "nvcomp" : {
      "version" : "2.6.1",
     "always_download": true,
      "proprietary_binary" : {
        "x86_64-linux" :  "https://developer.download.nvidia.com/compute/nvcomp/${version}/local_installers/nvcomp_${version}_x86_64_${cuda-toolkit-version-major}.x.tgz",
        "aarch64-linux" : "https://developer.download.nvidia.com/compute/nvcomp/${version}/local_installers/nvcomp_${version}_SBSA_${cuda-toolkit-version-major}.x.tgz"
      }
    },

We wouldn't do a local search.

@bdice bdice marked this pull request as ready for review June 26, 2023 18:51
@bdice bdice requested a review from a team as a code owner June 26, 2023 18:51
@bdice
Copy link
Contributor Author

bdice commented Jun 26, 2023

I verified that all three C++ builds (CUDA 11.8 on amd64/arm64 and CUDA 12.0 on amd64) pulled the correct nvcomp package during their builds. All looks good in the build logs!

@bdice bdice self-assigned this Jun 26, 2023
@jakirkham
Copy link
Member

/merge

@rapids-bot rapids-bot bot merged commit aed7174 into rapidsai:branch-23.08 Jun 26, 2023
53 checks passed
@jakirkham
Copy link
Member

Thanks Bradley! 🙏

@bdice
Copy link
Contributor Author

bdice commented Jun 26, 2023

Last note: I confirmed that the libcudf conda packages shrank by a few MB, as expected. 😄

@vyasr vyasr mentioned this pull request Jul 13, 2023
3 tasks
rapids-bot bot pushed a commit that referenced this pull request Jul 17, 2023
Looks like some changes to point to a feature branch were accidentally committed in #13566.

Authors:
  - Vyas Ramasubramani (https://github.com/vyasr)

Approvers:
  - Bradley Dice (https://github.com/bdice)

URL: #13696
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
improvement Improvement / enhancement to an existing function non-breaking Non-breaking change
Projects
Archived in project
Development

Successfully merging this pull request may close these issues.

5 participants