feat: free-threaded Python (3.13.0+) support #165

jmao-denver · 2024-09-17T21:45:37Z

Fixes #163

.github/workflows/build.yml

devinrsmith · 2024-10-25T17:05:29Z

src/main/c/jpy_jtype.c

+#ifdef Py_GIL_DISABLED
+typedef struct {
+    PyMutex lock;
+    PyThreadState* owner;
+    int recursion_level;
+} ReentrantLock;
+
+static void acquire_lock(ReentrantLock* self) {


Is this a pattern that you found suggested by C python, or elsewhere?

C Python doesn't offer an reentrant lock primitive. This is a pattern I found somewhere else.

src/main/c/jpy_jtype.c

src/main/java/org/jpy/PyObjectReferences.java

src/main/java/org/jpy/PyObject.java

jpyutil.py

src/main/c/jni/org_jpy_PyLib.c

src/main/c/jpy_jtype.c

cpwright · 2024-10-25T18:23:59Z

src/main/java/org/jpy/PyObject.java

@@ -71,7 +71,7 @@ public static int cleanup() {
    PyObject(long pointer, boolean fromJNI) {
        state = new PyObjectState(pointer);
        if (fromJNI) {
-            if (CLEANUP_ON_INIT && PyLib.hasGil()) {
+            if (CLEANUP_ON_INIT) {
                REFERENCES.cleanupOnlyUseFromGIL(); // only performs *one* cleanup


If we are no longer using the GIL, then we should rename this function and instead have comments on why it is correct. If we are on 3.12 mode why are we still correct?

This change is made to fix a bug based on the following two observations:

Because we now release the GIL whenever we cross into Java from Python (by @niloc132), the PyLib.hasGIL() test will always fail in Python GIL enabled mode, which results in that the cleanup method will never get called anymore. (BTW, in the GIL disabled mode, PyLib.hasGIL() returns true).

As part of the same enhancement change by @niloc132, he made sure that the cleanup is done under GIL.

private int cleanupOnlyUseFromGIL(long[] buffer) { return PyLib.ensureGil(() -> { int index = 0; while (index < buffer.length) { final Reference<? extends PyObject> reference = referenceQueue.poll(); if (reference == null) { ...

@jmao-denver is right, there are several layers of questionable here, and the latest round here is my fault.

latest stable release: hasGil is always false here, so this block never executes

prior to the "release gil while in Java" change: hasGil is always true, so this always runs.

Why did it even test to begin with? Why does it clean up all other object any time any PyObject is about to be passed in to Java (instead of just once either just before or just after calling Java)? Couldn't say. I think removing the check is correct (either just before the ft patch, or as part of it), and we should reevaluate what is even happening here separately.

Agreed on renaming cleanupOnlyUseFromGIL.

This was an oversight on my part during the "drop GIL" review.

I made the argument during the review that we might prefer to have some way to keep the GIL during "quick" java calls, or in other cases where we explicitly want to keep the GIL. I might argue that our call from JType_CreateJavaPyObject into the constructor of PyObject should keep the GIL.

The reason we need this to only be called into the context of the GIL (historically) is because we need the GIL when calling PyLib.decRef; I'm assuming in the case of free-threaded (/GIL-less) builds, Py_DECREF is thread safe. And it's not only that Py_DECREF that needs to be thread-safe, the arbitrary python deallocation code that can be executed as part of a decRef to 0 needs to be thread-safe.

In the context of free-threaded / GIL-less builds, maybe the functions need to be renamed to be something like PyLib.isThreadSafe / PyLib.ensureThreadSafe, or something like that.

chipkent · 2024-10-25T17:11:04Z

.github/workflows/check.yml

@@ -29,3 +29,32 @@ jobs:

      - name: Run Test
        run: python setup.py test
+
+  test-free-threaded:


I see windows-specific and mac-specific code, but I don't see CI checks for the platforms. There do appear to be windows runners. https://docs.github.com/en/actions/using-github-hosted-runners/using-github-hosted-runners/about-github-hosted-runners
It isn't the highest priority, but I am noting it.

src/main/c/jpy_jobj.c

chipkent · 2024-10-25T17:30:37Z

.github/workflows/build.yml

+      fail-fast: false
+      matrix:
+        info:
+          - { machine: 'ubuntu-20.04', python: '3.13t', arch: 'amd64', cmd: '.github/env/Linux/bdist-wheel.sh' }


Noting that the latest ubuntu LTS is 24.04.1. Not sure how much it matters here.

chipkent · 2024-10-25T17:32:46Z

.github/workflows/build.yml

+      fail-fast: false
+      matrix:
+        info:
+          - { machine: 'ubuntu-20.04', python: '3.13t', arch: 'aarch64', cmd: '.github/env/Linux/bdist-wheel.sh' }


see note about the latest LTS version

20.04 goes EOL 4/23/2025. We may consider bumping to at least 22.04 which.

@devinrsmith is almost certain changing to 22.04 will make the wheels not installation due to some incompatibility in the GLIBC. Will file a follow-up ticket.

We are currently targeting manylinux_2_17, which makes it compatible with GLIBC version 2.17+; afaict, the building of these artifacts needs to happen on a system w/ GLIBC version 2.17, and updating to 22.04 or later breaks this. We should have a ticket about GLIBC version support, and when we will move on past 2.17.

pypa/cibuildwheel#1772 has relevant upstream comments about the topic.

.github/workflows/build.yml

src/test/python/jpy_mt_eval_exec_test.py

src/main/c/jpy_jtype.c

Co-authored-by: Chip Kent <[email protected]>

src/main/c/jpy_module.c

src/main/c/jpy_jobj.c

src/main/c/jpy_module.c

.github/workflows/build.yml

devinrsmith · 2024-11-05T17:10:43Z

.github/workflows/check.yml

+            source .venv/bin/activate
+            uv pip install pip
+            echo $JAVA_HOME
+            echo PATH=$PATH >> $GITHUB_ENV


This looks like it is still using GITHUB_ENV?

src/main/c/jni/org_jpy_PyLib.c

jpyutil.py

devinrsmith · 2024-11-06T15:28:50Z

jpyutil.py

+
+    debug_build = sysconfig.get_config_var('Py_DEBUG')


This is not correct in the context of how you are using it. It needs to be checked if it's set to "1"; at least in my builds, it's explicitly set to "0":

python3.8 -m sysconfig | grep Py_DEBUG Py_DEBUG = "0"

After some more reading, I actually think that sys.abiflags is the more appropriate place to check, since it's specifically mentioned as being set for debug builds, https://docs.python.org/3.10/using/configure.html#python-debug-build:

Effects of a debug build:

Add d to sys.abiflags.

Sorry for my flip-flopping; it actually looks like .get_config_var is correct, since it seems to return an int when used like this. That said, maybe we should still prefer sys.abiflags since it's more explicitly documented?

Py_DEBUG is also explicitly mentioned there. Also a quick ChatGPT inquiry yields this;

I think we should be comfortable with using the value of Py_DEBUG in sysconfig

I'm not confident that Py_DEBUG and Py_REF_DEBUG macros are guaranteed to be equivalent to "config vars"; that said, it looks like sys.abiflags doesn't work on Windows, so I guess get_config_var is the best way for now.

It looks like there was a patch many years ago to add an official way to look up if it was built with debug, but it was not merged. python/cpython#69443

devinrsmith · 2024-11-06T15:38:09Z

src/main/java/org/jpy/PyObject.java

@@ -71,8 +71,8 @@ public static int cleanup() {
    PyObject(long pointer, boolean fromJNI) {
        state = new PyObjectState(pointer);
        if (fromJNI) {
-            if (CLEANUP_ON_INIT && PyLib.hasGil()) {
-                REFERENCES.cleanupOnlyUseFromGIL(); // only performs *one* cleanup
+            if (CLEANUP_ON_INIT) {


Although originally unintentional (I think), this is a change in behavior vs the current release. I might suggest Colin or I follow up with a PR, unless this is newly tested in this PR?

It isn't explicitly tested. But the code path does get executed implicitly in jpy_typeconv_test_pyobj.py.

jmao-denver added the enhancement New feature or request label Sep 17, 2024

jmao-denver requested review from chipkent, devinrsmith and cpwright September 17, 2024 21:45

jmao-denver self-assigned this Sep 17, 2024

jmao-denver force-pushed the 163-free-threaed-port branch 2 times, most recently from 404fb91 to a8008de Compare September 23, 2024 16:36

jmao-denver added 3 commits October 18, 2024 12:39

Rebase to master

f335468

More thread-safety change/refactoring

a1fb456

Fix a race between PO cleanup thread and Python

e93774a

jmao-denver force-pushed the 163-free-threaed-port branch 7 times, most recently from 0efa657 to b5b17ab Compare October 23, 2024 02:02

Add free-threaded build in GH action

e65f8ff

jmao-denver force-pushed the 163-free-threaed-port branch 2 times, most recently from 6e49f85 to 6299c87 Compare October 24, 2024 16:56

Fix dll names for FT/debug builds

6a0f8fe

jmao-denver force-pushed the 163-free-threaed-port branch from 4045833 to 6a0f8fe Compare October 24, 2024 19:36

jmao-denver added 4 commits October 24, 2024 14:02

Cleanup and more comments

2cb3314

Rollback temp change and add clarifying comments

fe97888

Complete release action for FT builds

ab38dcb

Merge ft builds into build.yml

16ba89a

jmao-denver marked this pull request as ready for review October 25, 2024 15:21

devinrsmith reviewed Oct 25, 2024

View reviewed changes

cpwright reviewed Oct 25, 2024

View reviewed changes

chipkent reviewed Oct 25, 2024

View reviewed changes

cpwright reviewed Oct 25, 2024

View reviewed changes

src/main/c/jpy_jtype.c Show resolved Hide resolved

jmao-denver and others added 11 commits October 29, 2024 13:45

Use GITHUB_PATH

40ef816

Squash windows-t build, add Java ver in matrix

a5718c1

Fix for invalid syntax for powershell

9aa8250

Add upload action for JVM core/log

752a0a7

debug build.yml

e1f841b

Apply suggestions from code review

0165921

Co-authored-by: Chip Kent <[email protected]>

Restore a block of code deleted by mistake

31b9010

debug Windows FT build

ccc3253

Add comments, use version directives

d972d96

Code cleanup

4058638

Fix a bug in convert() for java array type

173592d

jmao-denver commented Nov 2, 2024

View reviewed changes

src/main/c/jpy_module.c Show resolved Hide resolved

jmao-denver requested review from chipkent, cpwright, devinrsmith and niloc132 November 2, 2024 16:46

cpwright reviewed Nov 4, 2024

View reviewed changes

src/main/c/jpy_jobj.c Show resolved Hide resolved

src/main/c/jpy_module.c Show resolved Hide resolved

jmao-denver added 2 commits November 4, 2024 09:43

Remove duplicate comment

eea87e7

Add clarifying comment

c9d7bfa

devinrsmith reviewed Nov 5, 2024

View reviewed changes

jmao-denver added 2 commits November 5, 2024 23:19

Respond to review comments

1c51917

Naming change to be more applicable in FT mode

99f130a

chipkent approved these changes Nov 6, 2024

View reviewed changes

devinrsmith requested changes Nov 6, 2024

View reviewed changes

devinrsmith approved these changes Nov 7, 2024

View reviewed changes

jmao-denver merged commit 1d1a3b1 into jpy-consortium:master Nov 7, 2024
70 checks passed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat: free-threaded Python (3.13.0+) support #165

feat: free-threaded Python (3.13.0+) support #165

jmao-denver commented Sep 17, 2024

devinrsmith Oct 25, 2024

jmao-denver Oct 30, 2024

cpwright Oct 25, 2024

jmao-denver Oct 31, 2024 •

edited

Loading

niloc132 Oct 31, 2024

devinrsmith Nov 5, 2024

chipkent Oct 25, 2024

chipkent Oct 25, 2024

chipkent Oct 25, 2024

cpwright Oct 25, 2024

jmao-denver Oct 29, 2024 •

edited

Loading

devinrsmith Oct 29, 2024

devinrsmith Oct 29, 2024

devinrsmith Nov 5, 2024

devinrsmith Nov 6, 2024

devinrsmith Nov 6, 2024

jmao-denver Nov 7, 2024

devinrsmith Nov 7, 2024

devinrsmith Nov 6, 2024

jmao-denver Nov 7, 2024

feat: free-threaded Python (3.13.0+) support #165

feat: free-threaded Python (3.13.0+) support #165

Conversation

jmao-denver commented Sep 17, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jmao-denver Oct 31, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jmao-denver Oct 29, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jmao-denver Oct 31, 2024 •

edited

Loading

jmao-denver Oct 29, 2024 •

edited

Loading