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

python 3.13 support #442

Merged
merged 47 commits into from
Oct 10, 2024
Merged

python 3.13 support #442

merged 47 commits into from
Oct 10, 2024

Conversation

Xmader
Copy link
Member

@Xmader Xmader commented Sep 12, 2024

In order to make PythonMonkey support the upcoming (October 1, 2024) Python 3.13 release, the following changes/actions need to be made in the codebase:


Closes #441

`_Py_IsFinalizing` becomes a stable API in Python 3.13, and is renamed to `Py_IsFinalizing`
https://docs.python.org/3.13/c-api/init.html#c.Py_IsFinalizing
Since Python 3.13, `_PyDictView_New` function became an internal API.
Python 3.13 moved this undocumented function to private API.

See python/cpython#108607
@Xmader Xmader marked this pull request as ready for review September 20, 2024 08:43
@Xmader Xmader force-pushed the Xmader/feat/python-3.13-support branch from 56aed76 to 46909e3 Compare September 23, 2024 21:58
@Xmader Xmader force-pushed the Xmader/feat/python-3.13-support branch from 46909e3 to 623d83e Compare September 23, 2024 22:13
`_PyErr_SetKeyError` is more complex than originally thought, use the provided API when possible

See also python/cpython#101578
@zollqir zollqir self-requested a review September 24, 2024 14:36
Copy link
Collaborator

@zollqir zollqir left a comment

Choose a reason for hiding this comment

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

I noticed that only on python 3.13, the eval-test.simple test says

"Exception ignored in: <_io.BufferedReader name='/home/runner/work/PythonMonkey/PythonMonkey/tests/js/resources/eval-test.js'>"

I looked into it briefly, looks to be something to do with the file being closed more than once somehow.

Copy link
Collaborator

@zollqir zollqir left a comment

Choose a reason for hiding this comment

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

Overall looks great, just some minor changes

Comment on lines 85 to 90
# Python 3.13 dramatically changed how the namespace in `exec`/`eval` works
# See https://docs.python.org/3.13/whatsnew/3.13.html#defined-mutation-semantics-for-locals
_locals = {} # keep the local variables inside `eval`/`exec` to a dict
globalThis.python.eval = lambda x: eval(x, None, _locals)
globalThis.python.exec = lambda x: exec(x, None, _locals)
globalThis.python.getenv = os.getenv
Copy link
Collaborator

Choose a reason for hiding this comment

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

Suggested change
# Python 3.13 dramatically changed how the namespace in `exec`/`eval` works
# See https://docs.python.org/3.13/whatsnew/3.13.html#defined-mutation-semantics-for-locals
_locals = {} # keep the local variables inside `eval`/`exec` to a dict
globalThis.python.eval = lambda x: eval(x, None, _locals)
globalThis.python.exec = lambda x: exec(x, None, _locals)
globalThis.python.getenv = os.getenv
# Python 3.13 dramatically changed how the namespace in `exec`/`eval` works
# See https://docs.python.org/3.13/whatsnew/3.13.html#defined-mutation-semantics-for-locals
globalThis.python.eval = lambda x: eval(x, None, sys._getframe(1).f_locals)
globalThis.python.exec = lambda x: exec(x, None, sys._getframe(1).f_locals)

Without the above change, pythonmonkey would lose the ability to mutate parent scope variables with pm.eval. On main the following assert passes, but it currently fails on this branch. We should add a test to ensure this behaviour as well.

import pythonmonkey as pm

a = 42

pm.eval("python.exec('a = 43')")
assert a == 43

While on the subject, the following currently fails on main, but probably shouldn't. The above change fixes that as well.

import pythonmonkey as pm
def foo():
  b = 42
  pm.eval("python.exec('b = 43')")
  assert b == 43
foo()

Copy link
Member Author

Choose a reason for hiding this comment

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

https://docs.python.org/3/library/functions.html#exec

In all cases, if the optional parts are omitted, the code is executed in the current scope. If only globals is provided, it must be a dictionary (and not a subclass of dictionary), which will be used for both the global and the local variables. If globals and locals are given, they are used for the global and local variables, respectively. If provided, locals can be any mapping object. Remember that at the module level, globals and locals are the same dictionary.

https://github.com/python/cpython/blob/v3.9.20/Python/bltinmodule.c#L979-L985

if (globals == Py_None) {
        globals = PyEval_GetGlobals();
        if (locals == Py_None) {
            locals = PyEval_GetLocals();
            if (locals == NULL)
                return NULL;
        }

Copy link
Member Author

Choose a reason for hiding this comment

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

On main

In your first example, a is defined in the global scope, exec can catch it and re-assign it without any problem.

However, in your second example, b is defined in the local scope of the foo() function. Since the exec is executed inside of require.py, it can only see the variables defined in require.py as its "locals".

Copy link
Collaborator

Choose a reason for hiding this comment

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

"Without the above change, pythonmonkey would lose the ability to mutate parent scope variables with pm.eval. "

Question: is there a problem with losing that ability?

From a clean design POV, it's probably best to not let the languages mutate each other's scopes, except when those scoping objects are deliberately made available to this, i.e. in the case of globalThis.

The fact that Python modules have a scoping semantic not understand in JS-land underscores this from my POV.

If we made python.eval work more like indirect eval in JS, it would probably be a saner coding environment for our users, even though it might be a breaking change. And since the only code we really care about at this early stage is our test cases and BiFrost2, we control everything, so we can fix API and code in lock-step if necessary.

Copy link
Collaborator

Choose a reason for hiding this comment

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

i.e. to clarify: if we can change PythonMonkey to work with 3.13, and make it work the same in less than 3.13, I think that's probably okay. If a JS developer REALLY needs to mutate a Python scope, that dev can just pass in the scoping object, right?

src/IntType.cc Outdated Show resolved Hide resolved
include/pyshim.hh Outdated Show resolved Hide resolved
src/PyEventLoop.cc Show resolved Hide resolved
include/pyshim.hh Show resolved Hide resolved
include/pyshim.hh Show resolved Hide resolved
include/pyshim.hh Show resolved Hide resolved
Xmader and others added 11 commits September 25, 2024 17:41
…llOneArg` in all Python versions we support, so no need to do a shim on our own

This commit reverts `bcfcbf35ce5ae10bb3542052cdccadf48e95cdca`
…llObject(func, NULL)`, which is available in all Python versions
Python 3.13 was finally released on October 7, 2024.

GitHub Actions' `actions/setup-python` already added support for the final release version.
See https://raw.githubusercontent.com/actions/python-versions/main/versions-manifest.json
Something is double-free-ed during the final finalization:  in a debug build of Python, `./Include/object.h:602: _Py_NegativeRefcount: Assertion failed: object has negative ref count`

In non-debug build of Python, it simply segfaults at the end during finalization.
@wesgarland wesgarland dismissed zollqir’s stale review October 10, 2024 16:31

moved to other issue

Copy link
Collaborator

@wesgarland wesgarland left a comment

Choose a reason for hiding this comment

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

LGTM

@wesgarland wesgarland merged commit a799e35 into main Oct 10, 2024
40 checks passed
@wesgarland wesgarland deleted the Xmader/feat/python-3.13-support branch October 10, 2024 16:32
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.

Python 3.13 support
4 participants