Removes legacy rapids-cmake cython implementations as it is deprecated in 24.08 #614
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
|
@vyasr It would be good to get your opinion on this PR. I was worried that dropping the |
robertmaynard
added
breaking
2 tasks
|
I don't think that's a major concern. In theory I suppose it could happen from copy-pasting top-level CMakeLists.txt files, which is I imagine how many projects that aren't RAPIDS projects I originally set up ended up with this code, but I'm not too worried about it. OTOH I could see an argument for doing what you've done so that in the long run we could go back to |
I like this option as well so lets go forward wit the PR as is. |
robertmaynard
force-pushed
the
policy/remove_cython
branch
from
2c33727 to
9dfcdb4
Compare
vyasr
approved these changes
May 23, 2024
|
/merge |
7 tasks
raydouglass
pushed a commit
that referenced
this pull request
Dec 4, 2024
[Recent nightly failures of the `cpm_generate_pins-nested` test](https://github.com/rapidsai/rapids-cmake/actions/runs/12133373349/attempts/1) are unexpected because neither rapids-cmake, CPM, nor CMake have changed recently. [The error](https://github.com/rapidsai/rapids-cmake/actions/runs/12133373349/job/33828828013#step:8:5066) shows that [the list of projects to verify](https://github.com/rapidsai/rapids-cmake/blob/branch-25.02/testing/cpm/verify_generated_pins/CMakeLists.txt#L28) is being completely filtered out because none of them have a `<project>_SOURCE_DIR` variable set. This led me down a rabbit hole of finding a number of different issues in both rapids-cmake itself and in tests. I'll detail them one by one below. ### Problem 1: Our logic for when to download packages for which rapids-cmake provides specialized `rapids_cpm*` commands is wrong The first piece of evidence here is the difference between recent builds of our CI images. [The most recent builds of miniforge-cuda have spdlog in the base environment](https://github.com/rapidsai/ci-imgs/actions/runs/12145567995/job/33867533318#step:8:274), while [previous builds do not](https://github.com/rapidsai/ci-imgs/actions/runs/12036072167/job/33556500014#step:8:307). That should not affect our tests [because we set `CPM_DOWNLOAD_ALL` when populating the CMake cache](https://github.com/rapidsai/rapids-cmake/blob/branch-25.02/testing/utils/fill_cache/CMakeLists.txt#L36), but it turns out that #348 introduced a bug where [the `always_download` variable gets defaulted to OFF](https://github.com/rapidsai/rapids-cmake/blob/branch-25.02/rapids-cmake/cpm/detail/package_details.cmake#L95) and because of [how it propagates to `CPM_DOWNLOAD_ALL` in parent scope](https://github.com/rapidsai/rapids-cmake/blob/branch-25.02/rapids-cmake/cpm/detail/package_details.cmake#L118) we wind up with `CPM_DOWNLOAD_ALL` always being false. As a result, we have actually been ignoring `CPM_DOWNLOAD_ALL` for any packages for which `rapids_cpm_package_details` is called (the more specific `CPM_DOWNLOAD_<project>` should still have been working since that takes precedence). Solution: We unset the `always_download` variable initially instead of setting it to off so that the check is handled correctly. ### Problem 2: Even with the above logic fixed, the actual test will use the spdlog from the environment The above fix ensures that when we populate the CPM source cache in our tests we download spdlog instead of finding it in the environment. However, when tests are subsequently run they will still prefer the package unless `CPM_DOWNLOAD_ALL` (or `CPM_DOWNLOAD_<package>`) is set because of how CPM prioritizes. Solution: To resolve this, for all rapids-cpm tests we are now setting `CPM_DOWNLOAD_<pkg>` for all of the packages that have been added to our CPM cache. This fix is safe since it effectively just avoids finding local copies of the built package; the source will always come from the cache by design. This change is sufficient for tests to pass, but on closer inspection we see that only spdlog is verified by the verification script and not rmm or fmt. That's because of the next problems. ### Problem 3: All of our tests of pinning are incorrectly checking for rmm with the wrong case [The `project` call in rmm uses the uppercase name "RMM"](https://github.com/rapidsai/rmm/blob/branch-25.02/CMakeLists.txt#L25), but [the entry in rapids-cmake's versions.json](https://github.com/rapidsai/rapids-cmake/blob/branch-25.02/rapids-cmake/cpm/versions.json#L66) uses the lowercase name. As a result, our current logic for verifying generate pins can never test rmm because the project [is initially filtered based on `<project>_SOURCE_DIR` being defined](https://github.com/rapidsai/rapids-cmake/blob/branch-25.02/testing/cpm/verify_generated_pins.cmake#L33), which will use the uppercase name from the `project` call, but when actually verifying we use `rapids_cpm_package_details` to get the versions.json data _using the same string_, which will now have the wrong case if we passed the uppercase version originally. ### Problem 4: All of our tests of pinning are incorrectly checking for fmt with the wrong case This is the same problem as the above. [fmt uses `project(FMT)`](https://github.com/fmtlib/fmt/blob/master/CMakeLists.txt#L151). Solution for 3 and 4: We now use the `CPM_PACKAGE_<project>_SOURCE_DIR` variables instead of the `<project>_SOURCE_DIR` variables. The former are guaranteed to be using the export names of the package and will therefore always be case-consistent with the values in versions.json. ### Other changes in this PR - [This gtest test](https://github.com/rapidsai/rapids-cmake/blob/branch-25.02/testing/cpm/cpm_find-gtest-no-gmock/CMakeLists.txt) is explicitly based on finding a mocked up version of the package locally, so with the changes in this PR we now have to turn of `CPM_DOWNLOAD_GTest` explicitly inside the test. - I removed scikit-build from the testing CPM cache since as of #614 we no longer support its usage via the legacy rapids-cython module. The change to `verify_generated_pins.cmake` ensures that all of the above changes are actually having the desired effect, since now rather than silently passing when only a subset of projects requesting verification are downloaded we will error. All projects must be pulled correctly from the CPM cache during the test (which implicitly checks all four problems above) in order for tests to pass.
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Description
rapids-cmake cython module has been deprecated since 24.02 and now can be removed in 24.08
This removes the cython module and instead only offers cython-core.
Checklist
cmake-format.jsonis up to date with these changes.