Skip to content

fix: use freethreading-supported _PySet_NextItemRef where possible #682

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
Merged
cpcloud merged 8 commits into from
Jan 6, 2026
Merged

fix: use freethreading-supported _PySet_NextItemRef where possible #682

cpcloud merged 8 commits into from
Jan 6, 2026

Conversation

@cpcloud
Copy link
Contributor

@cpcloud cpcloud commented Dec 19, 2025

Small change to the _typeof.cpp code to use a freethreading-supported API for getting the first item from a set in C.

@copy-pr-bot
Copy link

copy-pr-bot bot commented Dec 19, 2025

Auto-sync is disabled for ready for review pull requests in this repository. Workflows must be run manually.

Contributors can view more details about this message here.

@cpcloud
Copy link
Contributor Author

cpcloud commented Dec 19, 2025

/ok to test

@greptile-apps
Copy link
Contributor

greptile-apps bot commented Dec 19, 2025 •
edited
Loading

Greptile Summary

This PR upgrades the codebase to use Python 3.13+'s freethreading-compatible _PySet_NextEntryRef API for iterating over sets. The implementation correctly handles the key differences between the old and new APIs:

Key Changes:

  • Switches to _PySet_NextEntryRef for Python 3.13+ (returns new reference) vs _PySet_NextEntry for older versions (returns borrowed reference)
  • Adds critical sections (Py_BEGIN_CRITICAL_SECTION/Py_END_CRITICAL_SECTION) around set access for thread safety in freethreading builds
  • Implements proper reference counting with Py_XDECREF(item) on both success and error paths for Python 3.13+
  • Refactors error handling using goto labels for cleaner cleanup logic
  • Adds documentation in pythonapi.py explaining the borrowed reference behavior of the old API

Implementation Quality:
The reference counting is correct: the new reference from _PySet_NextEntryRef is properly released on all code paths (success and error). The critical section is correctly scoped to only protect the set access operation, allowing the owned reference to be used safely outside the critical section.

Confidence Score: 5/5

  • This PR is safe to merge with proper Python 3.13+ freethreading support
  • The implementation correctly handles the API transition with proper reference counting, thread safety via critical sections, and appropriate preprocessor conditionals. All code paths properly manage the new reference returned by _PySet_NextEntryRef, and the changes are well-isolated with clear version guards
  • No files require special attention

Important Files Changed

Filename Overview
numba_cuda/numba/cuda/cext/_typeof.cpp Correctly implements freethreading-safe set iteration with proper reference counting and critical sections for Python 3.13+
numba_cuda/numba/cuda/core/pythonapi.py Adds documentation clarifying that _PySet_NextEntry returns borrowed reference

@cpcloud cpcloud marked this pull request as draft December 19, 2025 20:15
@copy-pr-bot
Copy link

copy-pr-bot bot commented Dec 19, 2025

Auto-sync is disabled for draft pull requests in this repository. Workflows must be run manually.

Contributors can view more details about this message here.

@cpcloud cpcloud marked this pull request as ready for review December 19, 2025 20:38
@cpcloud
Copy link
Contributor Author

cpcloud commented Dec 19, 2025

/ok to test

@cpcloud
Copy link
Contributor Author

cpcloud commented Jan 5, 2026

/ok to test

@greptile-apps
Copy link
Contributor

greptile-apps bot commented Jan 5, 2026

Greptile's behavior is changing!

From now on, if a review finishes with no comments, we will not post an additional "statistics" comment to confirm that our review found nothing to comment on. However, you can confirm that we reviewed your changes in the status check section.

This feature can be toggled off in your Code Review Settings by deselecting "Create a status check for each PR".

@cpcloud
Copy link
Contributor Author

cpcloud commented Jan 5, 2026

/ok to test

@kkraus14
Copy link
Contributor

kkraus14 commented Jan 6, 2026

Is there any reasonable way to test this?

@cpcloud cpcloud force-pushed the use-threadsafe-set-next-item-when-possible branch from 899ab45 to 3aa3d5c Compare January 6, 2026 12:09
@cpcloud
Copy link
Contributor Author

cpcloud commented Jan 6, 2026

/ok to test

@cpcloud
Copy link
Contributor Author

cpcloud commented Jan 6, 2026

Is there any reasonable way to test this?

AFAICT, there isn't.

@cpcloud
Copy link
Contributor Author

cpcloud commented Jan 6, 2026

/ok to test

@cpcloud cpcloud merged commit 2291482 into NVIDIA:main Jan 6, 2026
130 checks passed
@cpcloud cpcloud deleted the use-threadsafe-set-next-item-when-possible branch January 6, 2026 17:25
gmarkall added a commit to gmarkall/numba-cuda that referenced this pull request Jan 12, 2026
@gmarkall
Bump version to 0.24.0
b8fc6c7 
- Add arch specific target support (NVIDIA#549)
- chore: disable `locked` flag to bypass prefix-dev/pixi#5256 (NVIDIA#714)
- ci: relock pixi (NVIDIA#712)
- ci: remove redundant conda build in ci (NVIDIA#711)
- chore(deps): bump numba-cuda version and relock pixi (NVIDIA#707)
- Dropping bits in the old CI & Propagating recent changes from cuda-python (NVIDIA#683)
- Fix `test_wheel_deps_wheels.sh` to actually uninstall `nvvm` and `nvrtc` packages for CUDA 13 (NVIDIA#701)
- perf: remove some exception control flow and buffer-exception penalization for arrays (NVIDIA#700)
- perf: let CAI fall through instead of calling from_cuda_array_interface (NVIDIA#694)
- chore: perf lint (NVIDIA#697)
- chore(deps): bump deps in pixi lockfile (NVIDIA#693)
- fix: use freethreading-supported `_PySet_NextItemRef` where possible (NVIDIA#682)
- Support python `3.14` (NVIDIA#599)
- Remove customized address space tracking and address class emission in debug info (NVIDIA#669)
- Drop `experimental` from cuda.core namespace imports (NVIDIA#676)
- Remove dangling references to NUMBA_CUDA_ENABLE_MINOR_VERSION_COMPATIBILITY (NVIDIA#675)
- Use `rapidsai/sccache` in CI (NVIDIA#674)
- chore(dev-deps): remove ipython and pyinstrument (NVIDIA#670)
- Set up a new VM-based CI infrastructure  (NVIDIA#604)
@gmarkall gmarkall mentioned this pull request Jan 12, 2026
Merged
gmarkall added a commit that referenced this pull request Jan 12, 2026
@gmarkall
Bump version to 0.24.0 (#716)
c82ff48 
- Add arch specific target support (#549)
- chore: disable `locked` flag to bypass
prefix-dev/pixi#5256 (#714)
- ci: relock pixi (#712)
- ci: remove redundant conda build in ci (#711)
- chore(deps): bump numba-cuda version and relock pixi (#707)
- Dropping bits in the old CI & Propagating recent changes from
cuda-python (#683)
- Fix `test_wheel_deps_wheels.sh` to actually uninstall `nvvm` and
`nvrtc` packages for CUDA 13 (#701)
- perf: remove some exception control flow and buffer-exception
penalization for arrays (#700)
- perf: let CAI fall through instead of calling
from_cuda_array_interface (#694)
- chore: perf lint (#697)
- chore(deps): bump deps in pixi lockfile (#693)
- fix: use freethreading-supported `_PySet_NextItemRef` where possible
(#682)
- Support python `3.14` (#599)
- Remove customized address space tracking and address class emission in
debug info (#669)
- Drop `experimental` from cuda.core namespace imports (#676)
- Remove dangling references to
NUMBA_CUDA_ENABLE_MINOR_VERSION_COMPATIBILITY (#675)
- Use `rapidsai/sccache` in CI (#674)
- chore(dev-deps): remove ipython and pyinstrument (#670)
- Set up a new VM-based CI infrastructure  (#604)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@kkraus14 kkraus14 kkraus14 approved these changes

@brandon-b-miller brandon-b-miller brandon-b-miller approved these changes

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

3 participants

@cpcloud @kkraus14 @brandon-b-miller