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

Update for thrust 1.17 and fixes to accommodate for cuDF Buffer refactor #4871

Merged
merged 8 commits into from Aug 25, 2022

Conversation

dantegd
Copy link
Member

@dantegd dantegd commented Aug 22, 2022

No description provided.

@dantegd dantegd added bug Something isn't working 2 - In Progress Currenty a work in progress non-breaking Non-breaking change labels Aug 22, 2022
@dantegd dantegd added this to PR-WIP in v22.10 Release via automation Aug 22, 2022
@dantegd dantegd changed the title Update for thrust 1.17 Update for thrust 1.17 and fixes to accommodate for cuDF Buffer refactor Aug 23, 2022
@github-actions github-actions bot added the Cython / Python Cython or Python issue label Aug 23, 2022
@dantegd dantegd marked this pull request as ready for review August 23, 2022 17:43
@dantegd dantegd requested review from a team as code owners August 23, 2022 17:43
Copy link
Contributor

@wphicks wphicks left a comment

Choose a reason for hiding this comment

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

Two questions, neither of which should block the merge. Looks good!

cpp/cmake/thirdparty/get_gputreeshap.cmake Outdated Show resolved Hide resolved
python/cuml/common/array.py Show resolved Hide resolved
v22.10 Release automation moved this from PR-WIP to PR-Reviewer approved Aug 23, 2022
@dantegd
Copy link
Member Author

dantegd commented Aug 23, 2022

rerun tests

@galipremsagar
Copy link
Contributor

rerun tests

1 similar comment
@cjnolet
Copy link
Member

cjnolet commented Aug 24, 2022

rerun tests

Comment on lines 331 to 340
def host_serialize(self):
"""Serialize data and metadata associated with host memory.
Returns
-------
header : dict
The metadata required to reconstruct the object.
frames : list
The Buffers or memoryviews that the object should contain.
:meta private:
"""
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
def host_serialize(self):
"""Serialize data and metadata associated with host memory.
Returns
-------
header : dict
The metadata required to reconstruct the object.
frames : list
The Buffers or memoryviews that the object should contain.
:meta private:
"""
def host_serialize(self):
"""
Serialize data and metadata associated with host memory.
Returns
-------
header : dict
The metadata required to reconstruct the object.
frames : list
The Buffers or memoryviews that the object should contain.
:meta private:
"""

Copy link
Contributor

Choose a reason for hiding this comment

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

This is to fix the current error in CI:


Warning, treated as error:
/workspace/python/cuml/common/array.py:docstring of cuml.common.array.CumlArray.host_serialize:6:Unexpected indentation.
make: *** [Makefile:20: html] Error 2

@dantegd
Copy link
Member Author

dantegd commented Aug 25, 2022

rerun tests

@@ -221,7 +221,7 @@ __global__ void excess_sample_with_replacement_kernel(
// compute the adjacent differences according to the functor
// TODO: Replace deprecated 'FlagHeads' with 'SubtractLeft' when it is available
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
// TODO: Replace deprecated 'FlagHeads' with 'SubtractLeft' when it is available

We can remove this comment now.

@dantegd
Copy link
Member Author

dantegd commented Aug 25, 2022

rerun tests

@codecov-commenter
Copy link

Codecov Report

Merging #4871 (69b01ea) into branch-22.10 (7a0ab85) will increase coverage by 0.01%.
The diff coverage is 86.00%.

@@               Coverage Diff                @@
##           branch-22.10    #4871      +/-   ##
================================================
+ Coverage         78.02%   78.04%   +0.01%     
================================================
  Files               180      180              
  Lines             11385    11421      +36     
================================================
+ Hits               8883     8913      +30     
- Misses             2502     2508       +6     
Flag Coverage Δ
dask 46.28% <68.00%> (+0.06%) ⬆️
non-dask 67.32% <86.00%> (+0.05%) ⬆️

Flags with carried forward coverage won't be shown. Click here to find out more.

Impacted Files Coverage Δ
python/cuml/common/array.py 95.10% <85.10%> (-2.88%) ⬇️
python/cuml/thirdparty_adapters/adapters.py 91.54% <100.00%> (+0.05%) ⬆️

Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here.

@dantegd
Copy link
Member Author

dantegd commented Aug 25, 2022

@gpucibot merge

@rapids-bot rapids-bot bot merged commit a825caa into rapidsai:branch-22.10 Aug 25, 2022
v22.10 Release automation moved this from PR-Reviewer approved to Done Aug 25, 2022
divyegala pushed a commit to viclafargue/cuml that referenced this pull request Sep 2, 2022
ajschmidt8 pushed a commit that referenced this pull request Feb 13, 2023
jakirkham pushed a commit to jakirkham/cuml that referenced this pull request Feb 27, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
2 - In Progress Currenty a work in progress bug Something isn't working CMake CUDA/C++ Cython / Python Cython or Python issue non-breaking Non-breaking change
Projects
No open projects
Development

Successfully merging this pull request may close these issues.

None yet

7 participants