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

fix: Make org.jpy.PyLib.getCurrentLocals/Globals work for Python 3.13 #164

Merged
merged 3 commits into from
Sep 23, 2024

Conversation

jmao-denver
Copy link
Contributor

Python 3.13.0rc1 includes PEP667 (https://peps.python.org/pep-0667) and it made PyEval_GetLocals() no longer behave the same way as in Python 3.12 and older, which could be a bug to report to the cpython team but probably not worth it as it is marked as deprecated together with PyEval_GetGlobals().

Copy link
Member

@devinrsmith devinrsmith left a comment

Choose a reason for hiding this comment

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

This seems like a good change. Should we prefer to instead increment locals for python <= 3.12, and always XDECREF? At least from the PEPs example of migrating PyEval_GetLocals, it seems like incrementing the ref may be the more canonical approach?

locals = PyEval_GetLocals();
if (locals == NULL) {
    goto error_handler;
}
Py_INCREF(locals);

to

locals = PyEval_GetFrameLocals();
if (locals == NULL) {
    goto error_handler;
}

@jmao-denver jmao-denver changed the title Made org.jpy.PyLib.getCurrentLocals/Globals work for Python 3.13 Make org.jpy.PyLib.getCurrentLocals/Globals work for Python 3.13 Sep 4, 2024
@jmao-denver jmao-denver changed the title Make org.jpy.PyLib.getCurrentLocals/Globals work for Python 3.13 fix: Make org.jpy.PyLib.getCurrentLocals/Globals work for Python 3.13 Sep 13, 2024
@jmao-denver
Copy link
Contributor Author

This seems like a good change. Should we prefer to instead increment locals for python <= 3.12, and always XDECREF? At least from the PEPs example of migrating PyEval_GetLocals, it seems like incrementing the ref may be the more canonical approach?

locals = PyEval_GetLocals();
if (locals == NULL) {
    goto error_handler;
}
Py_INCREF(locals);

to

locals = PyEval_GetFrameLocals();
if (locals == NULL) {
    goto error_handler;
}

I wasn't very comfortable with the existing pattern (use borrowed ref) on globals/locals either, but it wasn't critical when GIL guarantees the thread-safetly. Now with free-threaded Python is just around the corner it is time to use the correct pattern for borrowed refs.

chipkent
chipkent previously approved these changes Sep 13, 2024
src/main/c/jni/org_jpy_PyLib.c Outdated Show resolved Hide resolved
src/main/c/jni/org_jpy_PyLib.c Outdated Show resolved Hide resolved
@jmao-denver jmao-denver merged commit 222d048 into jpy-consortium:master Sep 23, 2024
52 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants