[REVIEW] use installed C++ RMM in python build #588
Conversation
For all I can tell it's not actually recognized by setuptools, nor actually used, and in any case, there is no more actual library.
Various existing hardcoded paths were not actually used anymore, this is generally cleaner, and importantly, if the C++ build pulled in external dependencies that were not locally installed, this will ensure that these are used in the python build.
|
Can one of the admins verify this patch? |
1 similar comment
|
Can one of the admins verify this patch? |
|
ok to test |
|
Please update the changelog in order to start CI tests. View the gpuCI docs here. |
|
@germasch just informing you since you are a prolific external contributor to RMM now. We are nearing the end of the 0.16 dev cycle: Schedule here. We are entering the burndown phase tomorrow. In a nutshell, burndown means "no new PRs". If you think something should get into 0.16, please open a PR today, unless it's a significant change, in which case it should probably wait until 0.17. Tomorrow branch-0.17 will be created so you can target it with new PRs. See https://docs.rapids.ai/releases/process/ |
# Conflicts: # CHANGELOG.md
kkraus14
added
2 - In Progress
|
For what it's worth, touching the Assigning to 0.17 for this reason and will update the branch once created. |
|
I agree that this should be checked over carefully. But I also think the changes are small enough that it is possible for an experienced python person to review this and be pretty confident that nothing unexpected will happen (the maybe most complex part, getting I don't really care myself (I'm only using the C++ side of RMM), but it's worth mentioning that it fixes an actual issue (#584), which I had reproduced here. I didn't put a "fixes xxx" github comment to that effect since it's only true after my other cmake PR is merged. |
|
Or maybe I can't get it past the style checker, anyway :( When I ran |
germasch
changed the title
If it is not set, fall back to using the uninstalled headers in the source tree.
kkraus14
added
3 - Ready for review
kkraus14
added
5 - Ready to Merge
This PR switches the build of the python rmm to use the installed C++ RMM.
For the most part, this is just a cleanup, since the installed header-only RMM is the same as the one in the source tree.
But combined with PR #580, it will address issues like #584, where there is no spdlog preinstalled on the system. Currently, cmake will pull in its own spdlog in this case, but that spdlog won't be available to build the python modules. #580 will install spdlog together with RMM in this case, and with this PR, the python rmm build will use it.
Also, on a side note, for all I can tell, right now the build of the python modules will not use the downloaded Thrust 1.10.0, either, but whatever Thrust will be found on the Python side (ie., probably the one from the CUDA toolkit).