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

[FEA] Use rapids-core-dependencies to manage CCCL #1307

Open
vyasr opened this issue Jul 24, 2023 · 5 comments
Open

[FEA] Use rapids-core-dependencies to manage CCCL #1307

vyasr opened this issue Jul 24, 2023 · 5 comments
Labels
feature request New feature or request

Comments

@vyasr
Copy link
Contributor

vyasr commented Jul 24, 2023

Is your feature request related to a problem? Please describe.
Currently librmm packages contain vendored CCCL code because the packages are fetched via CPM. This vendoring behavior is potentially problematic since the vendored dependencies could clash with or even clobber those dependencies if installed upstream. The problem is compounded by the fact that the rest of RAPIDS also depends on these libraries and therefore pulls them transitively from librmm. In some cases (like thrust) RAPIDS requires patched versions of the libraries, making it impossible to use the original versions provided by e.g. the cuda-cccl conda packages.

Describe the solution you'd like
Now that rapids-cmake publishes packages, we should introduce a dependency on that package in librmm instead. That change would allow all of RAPIDS to single-source the (patched) CCCL dependencies without risk of clobbering.

@vyasr vyasr added feature request New feature or request ? - Needs Triage Need team to review and classify labels Jul 24, 2023
@bdice
Copy link
Contributor

bdice commented Jul 24, 2023

I'm planning to work on this after the CCCL 2.1.0 transition: rapidsai/rapids-cmake#399

@jgehrcke
Copy link

jgehrcke commented Feb 9, 2024

We were just bitten by this. Interesting. We have the same header twice in our dependency tree:

$ rg '__check_builtin\(is_referenceable\)'
targets/x86_64-linux/include/cuda/std/detail/libcxx/include/__config
722:#if 0 // __check_builtin(is_referenceable)
724:#endif // __check_builtin(is_referenceable)

include/rapids/libcudacxx/cuda/std/detail/libcxx/include/__config
716:#if __check_builtin(is_referenceable)

The first is as of cuda-cccl_linux-64-12.2.140, the second is as of librmm-23.12.00.

The first one contains a patch made in NVIDIA/libcudacxx#410 almost a year ago (thank you @wmaxey), specifically made for supporting newer clang versions.

The second one (vendored in librmm-23.12.00) does not contain that patch.

Hence, the second one does not compile with clang newer than 14 (well, it works with 14, but breaks with 16 and newer -- didn't test 15). We found this because clang-tidy failed:

$ clang-tidy --extra-arg=-ferror-limit=1 -p=build <redacted>.cpp 
118979 warnings and 2 errors generated.
Error while processing /home/<redacted>.cpp.
error: too many errors emitted, stopping now [clang-diagnostic-error]
/home/<redacted>/include/rapids/libcudacxx/cuda/std/detail/libcxx/include/__functional/../__type_traits/../__type_traits/is_referenceable.h:30:65: error: '_Tp' does not refer to a value [clang-diagnostic-error]
   30 |   : public integral_constant<bool, _LIBCUDACXX_IS_REFERENCEABLE(_Tp)>
      |                                                                 ^
/home/<redacted>/include/rapids/libcudacxx/cuda/std/detail/libcxx/include/__functional/../__type_traits/../__type_traits/is_referenceable.h:28:17: note: declared here
   28 | template <class _Tp>

I leave this here for others to have more luck with web search, this took forever for us to find the cause of!

If I understand correctly, the next RMM release including a4dd4f5 will resolve this for everyone, is that right?

@vyasr
Copy link
Contributor Author

vyasr commented Feb 23, 2024

@jgehrcke apologies for the slow response. The commit that you linked to will solve your particular problem by upgrading the version of CCCL embedded in rmm to include the clang patch that you need. It will not solve the broader issue of duplicating CCCL installations, but I don't think that's immediately relevant to what you're asking (please correct me if I'm wrong).

@bdice
Copy link
Contributor

bdice commented Apr 9, 2024

I'm moving this to the backlog -- I probably don't have time to work on this for 24.06. However, it's needed as a part of fixing up #1508 and preventing path conflicts for other RAPIDS repositories.

@bdice bdice removed their assignment Apr 9, 2024
@bdice bdice removed the ? - Needs Triage Need team to review and classify label Apr 9, 2024
@vyasr
Copy link
Contributor Author

vyasr commented Apr 10, 2024

FWIW by the time you do get around to this we may want something similar in place for wheels. The issue will be less pressing there though, it'll just be one of duplication and not really one of clobbering.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
feature request New feature or request
Projects
Status: Todo
Development

No branches or pull requests

3 participants