Always download CCCL.

[bdice]

Description

I saw an issue in rapidsai/raft#2092 that can be fixed by always downloading CCCL, so that we can get the required patches for CCCL::Thrust install rules. This affects CUDA 12.2 builds because that CTK version ships CCCL 2.2.0 (therefore it has a matching local package).

xref: rapidsai/raft#2092 (comment)

Note: This PR should target 24.02 even if we slip CUDA 12.2 conda/wheel builds to 24.04, since it fixes an issue with source builds using CUDA 12.2.

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]

I'm not sure that this is the fix that we want, it's too narrow. IIUC we'd have a similar problem for any package that has patches? I think what we want to do is to add logic to rapids_cpm_find that checks if a PATCH_COMMAND is passed and forces downloading if so. That will generally work for the case that CPM is well designed for, i.e. building from source. The downside is that in our common use case of conda packaging it will force clobbering because everything will download, but I think that's OK since it's already effectively the case.

[vyasr]

It looks like we're already observing this behavior in at least one place. Here's an rmm PR where spdlog is being installed into the conda environment and used, but we have a patch for spdlog.

That raises a separate question of whether the patch is maybe unnecessary? But the behavior we're observing doesn't seem right to me.

[vyasr]

Hmm but this is tricky. It seems like PATCH_COMMAND accepts 0 or 1 arguments. @robertmaynard is that intentional, or are we exploiting some implementation detail of ExternalProject_Add here? The documentation doesn't show that as being allowed. Specifically I'm referring to the fact that in most of our cpm functions we call PATCH_COMMAND ${patch_command} rather than PATCH_COMMAND "${patch_command}". Without the latter the patch command is not guaranteed to exist and cannot be checked against the empty string in rapids_cpm_find, which makes things tricky because then I don't know how to robustly verify whether a patch command was provided without reimplementing the parsers for CPM[Add|Find]Package and the underlying FetchContent functionality. Should we update all our cpm functions to specify the patch command as above? Alternatively, should they be checking whether the patch command is empty or not, and if so omit specifying it in the call to rapids_cpm_find?

[bdice]

@vyasr Thanks for digging into this. I wondered the same kind of thing but didn't know how to pursue that question as far as you have. I'll wait for feedback from @robertmaynard on this, but we want to figure this out for 24.02 because we don't want builds from source with CUDA 12.2 to be broken.

[vyasr]

For sure, and I agree on the timeline. If necessary we can always merge a stopgap solution while brainstorming about the longer-term solution.

Thinking about this more, the right solution is quite tricky, but perhaps involves multiple pieces:

• A breaking change to CPM.cmake where CPMFindPackage disallows passing a PATCH_COMMAND
• A change to rapids_cpm_find that dispatches appropriately to CPMFindPackage or CPMAddPackage depending on whether a PATCH_COMMAND was specified.
• A change to all the cpm modules in rapids-cmake that only conditionally adds the PATCH_COMMAND if patches are defined.

Maybe I'm overthinking this and there's a simpler way, I'll wait for Robert in case I'm overlooking obvious simpler solutions. I've started prototyping 2/3 above, though, so I can push those changes if we agree that they're right. The change to CPM.cmake isn't necessary for us to proceed.

[robertmaynard]

Specifically I'm referring to the fact that in most of our cpm functions we call PATCH_COMMAND ${patch_command} rather than PATCH_COMMAND "${patch_command}". Without the latter the patch command is not guaranteed to exist and cannot be checked against the empty string in rapids_cpm_find

ExternalProject treats an empty string or no string the same as a way to disable / ignore that command. So in both cases that would resolve to no PATCH_COMMAND running. This is a core detail in how the parser in ExternalProject works, and is leveraged as a feature on other STEPS as a way to turn them off ( install, configure, etc ).

the right solution is quite tricky, but perhaps involves multiple pieces:

Rapids-cmake needs to obey source tree overrides even when in the presence of PATCH commands being provided. This allows clients that need a specific local source to not be ignored ( e.g. offline builds ).

I think we can modify 2 and 3 as follows and ensure we always use a patch version of a project when provided:

• A change to rapids_cpm_find that dispatches appropriately to CPMAddPackage when CPM_${name}_SOURCE or a non empty string PATCH_COMMAND value was specified.

• A change to all the cpm modules in rapids-cmake that wraps the PATCH_COMMAND in quotes so that we get an empty string when no patch is defined.

[vyasr]

Sounds good. I'll work on that change, should be able to have something by Tuesday (Monday is a holiday).

[bdice]

#525 will replace this PR.

[vyasr]

Closing in favor of #525