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.

Sign up for GitHub

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

Jump to bottom

Add deprecation warnings for usage of ALL #339

Merged
merged 4 commits into from
Jan 17, 2023
Merged

Add deprecation warnings for usage of ALL #339

merged 4 commits into from
Jan 17, 2023

Conversation

robertmaynard
Copy link
Contributor

Description

Adds deprecation notices to usage of ALL with CMAKE_CUDA_ARCHITECTURES and rapids_cmake_set_architectures. This is done to remove the ambiguity between all ( all archs ) and ALL ( all archs that RAPIDS supports ). Instead projects should use RAPIDS as the magic value.

Fixes #316

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

@robertmaynard robertmaynard requested a review from a team as a code owner January 10, 2023 18:16
@robertmaynard robertmaynard added feature request New feature or request non-breaking Introduces a non-breaking change 2 - In Progress Currenty a work in progress labels Jan 10, 2023
@robertmaynard
Copy link
Contributor Author

robertmaynard commented Jan 10, 2023 •
edited
Loading

Need to update the following projects to use RAPIDS in the build.sh:

bdice
bdice reviewed Jan 10, 2023
View reviewed changes
Copy link
Contributor

@bdice bdice left a comment

Choose a reason for hiding this comment

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

All looks good! Two minor questions.

rapids-cmake/cuda/detail/architectures_policy.cmake Outdated
if(value STREQUAL "ALL")
set(new_value "RAPIDS")
if(called_from STREQUAL "FROM_INIT")
rapids_cmake_policy(DEPRECATED_IN 23.02 REMOVED_IN 23.06 MESSAGE
Copy link
Contributor

Choose a reason for hiding this comment

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

Why wait until 23.06?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

We have non rapids projects ( cuco / nvbench ) that aren't on the same release candence, so I want to give ample time to allow for all users of rapids-cmake to upgrade.

Copy link
Contributor

Choose a reason for hiding this comment

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

Aha! Sounds good.

testing/cuda/CMakeLists.txt Outdated Show resolved Hide resolved
@robertmaynard robertmaynard added 3 - Ready for Review Ready for review by team and removed 2 - In Progress Currenty a work in progress labels Jan 10, 2023
@robertmaynard
Copy link
Contributor Author

With PR's open for all RAPIDS projects to use the RAPIDS magic value, I think we are unblocked on merging this.

@robertmaynard
Copy link
Contributor Author

/merge

@rapids-bot rapids-bot bot merged commit e988663 into rapidsai:branch-23.02 Jan 17, 2023
@robertmaynard robertmaynard deleted the fea/add-ALL-deprecation-warning branch January 17, 2023 18:07
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@bdice bdice bdice approved these changes

Assignees
No one assigned
Labels
3 - Ready for Review Ready for review by team feature request New feature or request non-breaking Introduces a non-breaking change
Projects
rapids-cmake
Archived in project
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Add deprecation notices to ALL
2 participants
@robertmaynard @bdice