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

Fix failures in cpm_generate_pins-nested tests #724

Merged

Conversation

vyasr
Copy link
Contributor

@vyasr vyasr commented Dec 4, 2024

Description

Recent nightly failures of the cpm_generate_pins-nested test are unexpected because neither rapids-cmake, CPM, nor CMake have changed recently. The error shows that the list of projects to verify 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, while previous builds do not. That should not affect our tests because we set CPM_DOWNLOAD_ALL when populating the CMake cache, but it turns out that #348 introduced a bug where the always_download variable gets defaulted to OFF and because of how it propagates to CPM_DOWNLOAD_ALL in parent scope 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", but the entry in rapids-cmake's versions.json 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, 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).

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

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.

Checklist

  • I am familiar with the Contributing Guidelines.
  • New or existing tests cover these changes.
  • The documentation is up to date with these changes.
  • The cmake-format.json is up to date with these changes.
  • I have added new files under rapids-cmake/
    • I have added include guards (include_guard(GLOBAL))
    • I have added the associated docs/ rst file and update the api.rst

@vyasr vyasr added bug Something isn't working non-breaking Introduces a non-breaking change labels Dec 4, 2024
@vyasr vyasr self-assigned this Dec 4, 2024
@vyasr vyasr changed the title Don't set always_download by default in package_details Fix failures in cpm_generate_pins-nested tests Dec 4, 2024
Copy link
Contributor

@robertmaynard robertmaynard left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

An amazing write-up on this

rapids-cmake/cpm/detail/package_details.cmake Show resolved Hide resolved
@robertmaynard
Copy link
Contributor

As for Issue 2. I think it is safe to map the NO_CPM_CACHE test option to also set CPM_DOWNLOAD_<pkg> for every package that we have in our CPM testing cache

@robertmaynard
Copy link
Contributor

Problem 3: All of our tests of pinning are incorrectly checking for rmm with the wrong case:

  1. The project call will establish varibles based on the the casing in the project() it self. So projects like rmm and fmt for those variables will make them all upper case. ( as you pointed out for rmm )
  2. rapids-cmake uses rmm lowercase as that is the casing in the rapids_export call ( https://github.com/rapidsai/rmm/blob/branch-25.02/CMakeLists.txt#L170 ) .

I think the solution is that the test checks should use CPM_PACKAGE_${package}_SOURCE_DIR with the casing found in the versions.json file. ( https://github.com/rapidsai/rapids-cmake/blob/branch-25.02/rapids-cmake/cpm/detail/pinning_write_file.cmake#L46 ) This is what we use internally to rapids-cmake to build up the list of projects we can safely check for having source code

@vyasr
Copy link
Contributor Author

vyasr commented Dec 4, 2024

I think the solution is that the test checks should use CPM_PACKAGE_${package}_SOURCE_DIR with the casing found in the versions.json file. ( https://github.com/rapidsai/rapids-cmake/blob/branch-25.02/rapids-cmake/cpm/detail/pinning_write_file.cmake#L46 ) This is what we use internally to rapids-cmake to build up the list of projects we can safely check for having source code

Yes I was considering this as well. CPM sets the CPM_PACKAGE_* variables as internal so I wasn't sure if that was the wisest choice, but given our tight integration with CPM perhaps that is OK? I agree that if possible we want to use the name of the export and not the project name if the two are inconsistent.

@vyasr
Copy link
Contributor Author

vyasr commented Dec 4, 2024

As for Issue 2. I think it is safe to map the NO_CPM_CACHE test option to also set CPM_DOWNLOAD_<pkg> for every package that we have in our CPM testing cache

I think that this is orthogonal to NO_CPM_CACHE. The CPM cache only kicks in if CPMAddPackage is called so that we look in the source cache instead of accessing the network for downloads. What we need is to avoid this whole process being preempted by CPMFindPackage finding the built package somewhere on the system already. I think we may want to set CPM_DOWNLOAD_<pkg> for every test so that when NO_CPM_CACHE is on we guarantee downloading rather than using something on the system and when NO_CPM_CACHE is off we always use the source from the cache rather than a pre-built library somewhere on the system.

@robertmaynard
Copy link
Contributor

I think the solution is that the test checks should use CPM_PACKAGE_${package}_SOURCE_DIR with the casing found in the versions.json file. ( https://github.com/rapidsai/rapids-cmake/blob/branch-25.02/rapids-cmake/cpm/detail/pinning_write_file.cmake#L46 ) This is what we use internally to rapids-cmake to build up the list of projects we can safely check for having source code

Yes I was considering this as well. CPM sets the CPM_PACKAGE_* variables as internal so I wasn't sure if that was the wisest choice, but given our tight integration with CPM perhaps that is OK? I agree that if possible we want to use the name of the export and not the project name if the two are inconsistent.

Lets use it. We already rely on it inside the pinning logic

@robertmaynard
Copy link
Contributor

As for Issue 2. I think it is safe to map the NO_CPM_CACHE test option to also set CPM_DOWNLOAD_<pkg> for every package that we have in our CPM testing cache

I think that this is orthogonal to NO_CPM_CACHE. The CPM cache only kicks in if CPMAddPackage is called so that we look in the source cache instead of accessing the network for downloads. What we need is to avoid this whole process being preempted by CPMFindPackage finding the built package somewhere on the system already. I think we may want to set CPM_DOWNLOAD_<pkg> for every test so that when NO_CPM_CACHE is on we guarantee downloading rather than using something on the system and when NO_CPM_CACHE is off we always use the source from the cache rather than a pre-built library somewhere on the system.

That makes sense 👍

@vyasr
Copy link
Contributor Author

vyasr commented Dec 4, 2024

I uncovered one more issue in #725, but I'm not going to try and fix that here.

@vyasr vyasr marked this pull request as ready for review December 4, 2024 20:41
@vyasr vyasr requested a review from a team as a code owner December 4, 2024 20:41
@vyasr vyasr requested a review from robertmaynard December 4, 2024 20:41
@raydouglass raydouglass merged commit c82539c into rapidsai:branch-24.12 Dec 4, 2024
14 of 15 checks passed
@vyasr vyasr deleted the fix/package_details_always_download branch December 4, 2024 22:29
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working non-breaking Introduces a non-breaking change
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants