[FEA]: Use std::optional instead of thrust::optional
#1464
Merged
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
We already require c++17, so there is no benefit on pulling in `thrust::optional` as a dependency.
github-actions
bot
added
Python
miscco
added
feature request
miscco
added
non-breaking
|
I have added a set of deprecation warnings, that should ensure that we do not break c++ code explicitly passing |
harrism
reviewed
Feb 10, 2024
harrism
approved these changes
Feb 15, 2024
|
Seems one of the tests failed with oom could someone rerun |
|
@miscco I triggered a rerun. |
|
@miscco is this ready to take out of draft? |
|
Oh yeah this should be fine to go |
miscco
changed the title
std::optional instead of thrust::optionalstd::optional instead of thrust::optional
bdice
reviewed
Feb 23, 2024
python/rmm/_lib/memory_resource.pyx
Outdated
| @@ -25,6 +25,7 @@ from libc.stddef cimport size_t | |||
| from libc.stdint cimport int8_t, int64_t, uintptr_t | |||
| from libcpp cimport bool | |||
| from libcpp.memory cimport make_unique, unique_ptr | |||
| from libcpp.optional cimport make_optional, optional | |||
There was a problem hiding this comment.
@miscco I pushed some changes here -- now that we're using the STL version, we don't need the extern definition from Thrust at all. I did need to make a couple small tweaks (casting to size_t) to make the libcpp.optional work, though.
bdice
approved these changes
Feb 23, 2024
|
/merge |
trxcllnt
added a commit
to trxcllnt/raft
that referenced
this pull request
Feb 26, 2024
rapids-bot bot
pushed a commit
to rapidsai/raft
that referenced
this pull request
Feb 27, 2024
Update `thrust::optional` to `std::optional` to align with rapidsai/rmm#1464. Authors: - Paul Taylor (https://github.com/trxcllnt) Approvers: - Corey J. Nolet (https://github.com/cjnolet) - Mark Harris (https://github.com/harrism) URL: #2199
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.
We already require c++17, so there is no benefit on pulling in
thrust::optionalas a dependency.In addition thrust versiones its types with the cuda architecture, so using those types in API boundaries is not recommended.
This could be a potentially breaking change if a user would explicitly pass in
thrust::optionalinto those APIs which seems unlikely