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

Provide our own Cython declaration for make_unique #13746

Merged
merged 1 commit into from
Jul 26, 2023

Conversation

wence-
Copy link
Contributor

@wence- wence- commented Jul 25, 2023

Description

make_unique in Cython's libcpp headers is not annotated with except +. As a consequence, if the constructor throws, we do not catch it in Python. To work around this (see cython/cython#5560 for details), provide our own implementation.

Due to the way assignments occur to temporaries, we need to now explicitly wrap all calls to make_unique in move, but that is arguably preferable to not being able to catch exceptions, and will not be necessary once we move to Cython 3.

Checklist

  • I am familiar with the Contributing Guidelines.
  • New or existing tests cover these changes.
  • The documentation is up to date with these changes.

@wence- wence- requested a review from a team as a code owner July 25, 2023 14:20
@wence- wence- requested review from shwina and charlesbluca July 25, 2023 14:20
@github-actions github-actions bot added the Python Affects Python cuDF API. label Jul 25, 2023
@shwina shwina added non-breaking Non-breaking change bug Something isn't working labels Jul 25, 2023
@wence- wence- force-pushed the wence/fix/issue-13743 branch from 111ce41 to 58631f8 Compare July 25, 2023 16:31
Copy link
Contributor

@shwina shwina left a comment

Choose a reason for hiding this comment

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

This is fine until it's fixed in Cython

@shwina
Copy link
Contributor

shwina commented Jul 25, 2023

Let's re-target this to 23.10

@wence- wence- force-pushed the wence/fix/issue-13743 branch from 58631f8 to 758483d Compare July 26, 2023 08:10
@wence- wence- requested review from a team as code owners July 26, 2023 08:10
@wence- wence- requested review from harrism and PointKernel July 26, 2023 08:10
@github-actions github-actions bot added libcudf Affects libcudf (C++/CUDA) code. CMake CMake build issue conda Java Affects Java cuDF API. labels Jul 26, 2023
@wence- wence- changed the base branch from branch-23.08 to branch-23.10 July 26, 2023 08:10
@wence- wence- removed request for a team, harrism and PointKernel July 26, 2023 08:11
make_unique in Cython's libcpp headers is not annotated with `except
+`. As a consequence, if the constructor throws, we do not catch it in
Python. To work around this (see cython/cython#5560 for details),
provide our own implementation.

Due to the way assignments occur to temporaries, we need to now
explicitly wrap all calls to `make_unique` in `move`, but that is
arguably preferable to not being able to catch exceptions, and will
not be necessary once we move to Cython 3.

- Closes rapidsai#13743
@wence- wence- force-pushed the wence/fix/issue-13743 branch from 758483d to ab77c9f Compare July 26, 2023 08:53
@github-actions github-actions bot removed libcudf Affects libcudf (C++/CUDA) code. CMake CMake build issue conda Java Affects Java cuDF API. labels Jul 26, 2023
@wence-
Copy link
Contributor Author

wence- commented Jul 26, 2023

Let's re-target this to 23.10

Done.

@shwina
Copy link
Contributor

shwina commented Jul 26, 2023

/merge

@rapids-bot rapids-bot bot merged commit 427f879 into rapidsai:branch-23.10 Jul 26, 2023
@wence- wence- deleted the wence/fix/issue-13743 branch July 26, 2023 12:41
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 Non-breaking change Python Affects Python cuDF API.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[BUG] Catch and propagate rmm out of memory errors during copy operations up to Python
2 participants