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

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

Merged
merged 24 commits into from
Nov 7, 2024
Merged
Show file tree
Hide file tree
Changes from 2 commits
Commits
Show all changes
24 commits
Select commit Hold shift + click to select a range
f335468
Rebase to master
jmao-denver Sep 23, 2024
a1fb456
More thread-safety change/refactoring
jmao-denver Sep 23, 2024
e93774a
Fix a race between PO cleanup thread and Python
jmao-denver Sep 27, 2024
e65f8ff
Add free-threaded build in GH action
jmao-denver Oct 21, 2024
6a0f8fe
Fix dll names for FT/debug builds
jmao-denver Oct 24, 2024
2cb3314
Cleanup and more comments
jmao-denver Oct 24, 2024
fe97888
Rollback temp change and add clarifying comments
jmao-denver Oct 24, 2024
ab38dcb
Complete release action for FT builds
jmao-denver Oct 24, 2024
16ba89a
Merge ft builds into build.yml
jmao-denver Oct 24, 2024
40ef816
Use GITHUB_PATH
jmao-denver Oct 29, 2024
a5718c1
Squash windows-t build, add Java ver in matrix
jmao-denver Oct 29, 2024
9aa8250
Fix for invalid syntax for powershell
jmao-denver Oct 29, 2024
752a0a7
Add upload action for JVM core/log
jmao-denver Oct 29, 2024
e1f841b
debug build.yml
jmao-denver Oct 29, 2024
0165921
Apply suggestions from code review
jmao-denver Oct 29, 2024
31b9010
Restore a block of code deleted by mistake
jmao-denver Oct 29, 2024
ccc3253
debug Windows FT build
jmao-denver Oct 29, 2024
d972d96
Add comments, use version directives
jmao-denver Oct 30, 2024
4058638
Code cleanup
jmao-denver Oct 31, 2024
173592d
Fix a bug in convert() for java array type
jmao-denver Nov 2, 2024
eea87e7
Remove duplicate comment
jmao-denver Nov 4, 2024
c9d7bfa
Add clarifying comment
jmao-denver Nov 4, 2024
1c51917
Respond to review comments
jmao-denver Nov 6, 2024
99f130a
Naming change to be more applicable in FT mode
jmao-denver Nov 6, 2024
File filter

Filter by extension

Filter by extension


Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 1 addition & 1 deletion .github/workflows/build.yml
Original file line number Diff line number Diff line change
Expand Up @@ -169,7 +169,7 @@ jobs:
- { machine: 'ubuntu-20.04', python: '3.13t', java: '8', arch: 'amd64', cmd: '.github/env/Linux/bdist-wheel.sh' }
- { machine: 'windows-2022', python: '3.13t', java: '8', arch: 'amd64', cmd: '.\.github\env\Windows\bdist-wheel.ps1' }
- { machine: 'macos-13', python: '3.13t', java: '8', arch: 'amd64', cmd: '.github/env/macOS/bdist-wheel.sh' }
- { machine: 'macos-latest', python: '3.13t', java: '11', arch: 'arm64', cmd: '.github/env/macOS/bdist-wheel.sh' }
- { machine: 'macos-14', python: '3.13t', java: '11', arch: 'arm64', cmd: '.github/env/macOS/bdist-wheel.sh' }

steps:
- uses: actions/checkout@v4
Expand Down
2 changes: 1 addition & 1 deletion .github/workflows/check.yml
Original file line number Diff line number Diff line change
Expand Up @@ -61,7 +61,7 @@ jobs:
source .venv/bin/activate
uv pip install pip
echo $JAVA_HOME
echo PATH=$PATH >> $GITHUB_ENV
echo PATH=$PATH >> $GITHUB_PATH

- run: pip install "setuptools < 72"

Expand Down
7 changes: 2 additions & 5 deletions jpyutil.py
devinrsmith marked this conversation as resolved.
Show resolved Hide resolved
Original file line number Diff line number Diff line change
Expand Up @@ -331,11 +331,8 @@ def _find_python_dll_file(fail=False):
# Prepare list of possible library file names

# account for Python debug builds
try:
sys.gettotalrefcount()
debug_build = True
except AttributeError:
debug_build = False

debug_build = sysconfig.get_config_var('Py_DEBUG')
Comment on lines +334 to +335
Copy link
Member

Choose a reason for hiding this comment

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

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:

Copy link
Member

Choose a reason for hiding this comment

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

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?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

image

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

image

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

Copy link
Member

Choose a reason for hiding this comment

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

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


# account for Python 3.13+ with GIL disabled
dll_suffix = ''
Expand Down
4 changes: 2 additions & 2 deletions src/main/c/jni/org_jpy_PyLib.c
Original file line number Diff line number Diff line change
Expand Up @@ -499,7 +499,7 @@ void dumpDict(const char* dictName, PyObject* dict)

size = PyDict_Size(dict);
printf(">> dumpDict: %s.size = %ld\n", dictName, size);
#ifdef Py_GIL_DISABLED
#if PY_VERSION_HEX >= 0x030D0000 // >=3.13
// PyDict_Next is not thread-safe, so we need to protect it with a critical section
// https://docs.python.org/3/howto/free-threading-extensions.html#pydict-next
Py_BEGIN_CRITICAL_SECTION(dict);
Expand All @@ -510,7 +510,7 @@ void dumpDict(const char* dictName, PyObject* dict)
printf(">> dumpDict: %s[%ld].name = '%s'\n", dictName, i, name);
i++;
}
#ifdef Py_GIL_DISABLED
#if PY_VERSION_HEX >= 0x030D0000 // >=3.13
Py_END_CRITICAL_SECTION();
#endif
}
Expand Down
6 changes: 3 additions & 3 deletions src/main/java/org/jpy/PyObject.java
Original file line number Diff line number Diff line change
Expand Up @@ -58,7 +58,7 @@ private static void startCleanupThread() {
}

public static int cleanup() {
return REFERENCES.asProxy().cleanupOnlyUseFromGIL();
return REFERENCES.asProxy().threadSafeCleanup();
}

private final PyObjectState state;
Expand All @@ -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()) {
jmao-denver marked this conversation as resolved.
Show resolved Hide resolved
REFERENCES.cleanupOnlyUseFromGIL(); // only performs *one* cleanup
if (CLEANUP_ON_INIT) {
Copy link
Member

Choose a reason for hiding this comment

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

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?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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

REFERENCES.threadSafeCleanup(); // only performs *one* cleanup
}
if (CLEANUP_ON_THREAD) {
// ensures that we've only started after python has been started, and we know there is something to cleanup
Expand Down
2 changes: 1 addition & 1 deletion src/main/java/org/jpy/PyObjectCleanup.java
Original file line number Diff line number Diff line change
@@ -1,5 +1,5 @@
package org.jpy;

interface PyObjectCleanup {
int cleanupOnlyUseFromGIL();
int threadSafeCleanup();
}
8 changes: 4 additions & 4 deletions src/main/java/org/jpy/PyObjectReferences.java
Original file line number Diff line number Diff line change
Expand Up @@ -76,11 +76,11 @@ private Reference<PyObject> asRef(PyObject pyObject) {
/**
* This should *only* be invoked through the proxy, or when we *know* we have the GIL.
*/
public int cleanupOnlyUseFromGIL() {
return cleanupOnlyUseFromGIL(buffer);
public int threadSafeCleanup() {
return threadSafeCleanup(buffer);
}

private int cleanupOnlyUseFromGIL(long[] buffer) {
private int threadSafeCleanup(long[] buffer) {
return PyLib.ensureGil(() -> {
int index = 0;
while (index < buffer.length) {
Expand Down Expand Up @@ -164,7 +164,7 @@ def cleanup(references):
// any fairness guarantees. As such, we need to be mindful of other python users/code,
// and ensure we don't overly acquire the GIL causing starvation issues, especially when
// there is no cleanup work to do.
final int size = proxy.cleanupOnlyUseFromGIL();
final int size = proxy.threadSafeCleanup();


// Although, it *does* make sense to potentially take the GIL in a tight loop when there
Expand Down
Loading