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 to Cython 3.0.0 #1313

Merged
merged 6 commits into from
Aug 4, 2023
Merged

Conversation

vyasr
Copy link
Contributor

@vyasr vyasr commented Jul 26, 2023

Description

This PR contains the minimal set of changes to compile using Cython 3 without warnings. Future PRs can be made to take advantage of new or improved features.

The specific changes are:

  • Ensuring nogil always comes after except. except * nogil is a compile-time error in Cython 3
  • Adding noexcept or except * to any cdef functions missing them. In Cython 0.29 these would default to noexcept, which meant that exceptions would not be properly propagated. In Cython 3.0.0, these default to except *, which incurs a performance penalty for reacquiring the GIL to check the exception value even for nogil functions. Being explicit here is important.

There are a large number of outstanding warnings due to NVIDIA/cuda-python#44. cuda-python for CUDA 12 has the necessary fix, but we will need a cuda-python 11.8.* bugfix with a backport to make those warnings go away.

Checklist

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

@vyasr vyasr added 3 - Ready for review Ready for review by team Python Related to RMM Python API breaking Breaking change improvement Improvement / enhancement to an existing function labels Jul 26, 2023
@vyasr vyasr self-assigned this Jul 26, 2023
@github-actions github-actions bot added the conda label Jul 26, 2023
@vyasr
Copy link
Contributor Author

vyasr commented Jul 26, 2023

There are a couple of outstanding warnings that I'm not sure how we want to resolve:

warning: rmm/_lib/memory_resource.pyx:40:51: Unknown type declaration 'BaseException' in annotation, ignoring                                                               
warning: rmm/_lib/memory_resource.pyx:770:37: Unknown type declaration 'size_t' in annotation, ignoring

These come from type annotations, one in an argument err: BaseException and another in a return type -> size_type. These appear to not be recognized types for Cython, but I don't know if that's happening because Cython is trying to use new annotation-style syntax to treat these like cdef BaseException err. Would appreciate a look if someone has some idea there. We could always remove the annotations if we want to get rid of the warnings, I don't think there's anything critically wrong either way here though.

@wence-
Copy link
Contributor

wence- commented Jul 27, 2023

warning: rmm/_lib/memory_resource.pyx:770:37: Unknown type declaration 'size_t' in annotation, ignoring

memory_resource.pyx needs from libc.stdint cimport size_t?

@shwina
Copy link
Contributor

shwina commented Jul 27, 2023

warning: rmm/_lib/memory_resource.pyx:40:51: Unknown type declaration 'BaseException' in annotation, ignoring

For this one, it appears using builtins.BaseException or __builtins__.BaseException removes the error..

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.

Pending resolving the remaining warnings

@vyasr
Copy link
Contributor Author

vyasr commented Jul 27, 2023

Good catches! I didn't consider that Cython would need these imported since they typically work out of the box, but I guess for typing annotations it's stricter (which is good I suppose). The proposed fixes worked (with one minor edit, size_t is in libc/stddef.pxd, not libc/stdint.pxd).

@vyasr vyasr marked this pull request as ready for review August 4, 2023 15:59
@vyasr vyasr requested review from a team as code owners August 4, 2023 15:59
@vyasr
Copy link
Contributor Author

vyasr commented Aug 4, 2023

/merge

@rapids-bot rapids-bot bot merged commit 2376e08 into rapidsai:branch-23.10 Aug 4, 2023
41 checks passed
@vyasr vyasr deleted the feat/cython3 branch August 4, 2023 18:40
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
3 - Ready for review Ready for review by team breaking Breaking change conda improvement Improvement / enhancement to an existing function Python Related to RMM Python API
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants