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

Adapt to C-API changes in CPython 3.13 #5767

Merged
merged 27 commits into from Oct 30, 2023
Merged

Conversation

scoder
Copy link
Contributor

@scoder scoder commented Oct 15, 2023

No description provided.

@da-woods
Copy link
Contributor

Looks OK except for running it on Github actions

@scoder
Copy link
Contributor Author

scoder commented Oct 16, 2023

except for running it on Github actions

Yeah, CPython 3.13a1 is only freshly released. It'll become available eventually. This isn't meant for 3.0.4.

@hroncok
Copy link
Contributor

hroncok commented Oct 20, 2023

For the record, we tried to use this with Python 3.13.0a1 in Fedora and we see:

  /builddir/build/BUILD/cython-3.0.4/Cython/Compiler/Parsing.c:902:55: error: implicit declaration of function ‘_PyDict_GetItem_KnownHash’ [-Werror=implicit-function-declaration]

  /builddir/build/BUILD/cython-3.0.4/Cython/Compiler/Parsing.c:2707:22: error: implicit declaration of function ‘_PyList_Extend’; did you mean ‘PyList_Append’? [-Werror=implicit-function-declaration]

  /builddir/build/BUILD/cython-3.0.4/Cython/Compiler/Parsing.c:890:65: error: implicit declaration of function ‘_PyDict_NewPresized’ [-Werror=implicit-function-declaration]

  /builddir/build/BUILD/cython-3.0.4/Cython/Compiler/Parsing.c:815:39: error: implicit declaration of function ‘_PyThreadState_UncheckedGet’; did you mean ‘PyThreadState_GetUnchecked’? [-Werror=implicit-function-declaration]

  /builddir/build/BUILD/cython-3.0.4/Cython/Compiler/Parsing.c:3000:38: error: implicit declaration of function ‘_PyDict_SetItem_KnownHash’ [-Werror=implicit-function-declaration]

  /builddir/build/BUILD/cython-3.0.4/Cython/Compiler/Parsing.c:87139:12: error: implicit declaration of function ‘__Pyx_PyObject_GetOptionalAttr’; did you mean ‘PyObject_GetOptionalAttr’? [-Werror=implicit-function-declaration]

  /builddir/build/BUILD/cython-3.0.4/Cython/Compiler/Parsing.c:87784:28: error: implicit declaration of function ‘_PyVectorcall_Function’; did you mean ‘PyVectorcall_Function’? [-Werror=implicit-function-declaration]

  /builddir/build/BUILD/cython-3.0.4/Cython/Compiler/Parsing.c:92216:16: error: implicit declaration of function ‘_PyLong_FromByteArray’ [-Werror=implicit-function-declaration]


@scoder
Copy link
Contributor Author

scoder commented Oct 20, 2023

Apparently, there was a huge CPython core initiative earlier this summer to break existing Python packages using their existing "private by convention" APIs. I'm yet to understand why this happened, why so much effort from their side was put into deliberate breakage of existing code.

Some of the API functions are probably easy to replace, but we'll have to see how it goes. I don't see a replacement for _PyLong_FromByteArray() yet. I'll ask.

@scoder
Copy link
Contributor Author

scoder commented Oct 20, 2023

I think I got rid of the removed C-API calls. That was easy because all of them were using private C-API functions for which we have compatibility replacements that I just had to enable for Py3.13+. The replacement for _PyLong_FromByteArray() in particular is awful, though, and not everything is necessarily as fast as before. But it should compile now.

@scoder scoder changed the title Adapt to new C-API features in CPython 3.13 Adapt to C-API changes in CPython 3.13 Oct 22, 2023
@hroncok
Copy link
Contributor

hroncok commented Oct 22, 2023

Unfortunately, I still have:

  /builddir/build/BUILD/cython-3.0.4/Cython/Compiler/Parsing.c: In function ‘__Pyx_PyInt_As_long’:
  /builddir/build/BUILD/cython-3.0.4/Cython/Compiler/Parsing.c:92478:23: error: implicit declaration of function ‘_PyLong_AsByteArray’ [-Werror=implicit-function-declaration]
  92478 |                 ret = _PyLong_AsByteArray((PyLongObject *)v,
        |                       ^~~~~~~~~~~~~~~~~~~

The rest is gone.

@hroncok
Copy link
Contributor

hroncok commented Oct 22, 2023

I see there is a pull request open by you to get it back. I can wait, just wanted to report back.

@hroncok
Copy link
Contributor

hroncok commented Oct 22, 2023

  /builddir/build/BUILD/cython-3.0.4/Cython/Compiler/Code.c:105843:13: error: implicit declaration of function ‘_PyGen_SetStopIterationValue’; did you mean ‘__Pyx_PyGen__FetchStopIterationValue’? [-Werror=implicit-function-declaration]
  105843 |             _PyGen_SetStopIterationValue(result);
         |             ^~~~~~~~~~~~~~~~~~~~~~~~~~~~
         |             __Pyx_PyGen__FetchStopIterationValue

@hroncok
Copy link
Contributor

hroncok commented Oct 24, 2023

  /builddir/build/BUILD/cython-3.0.4/Cython/Compiler/FlowControl.c: In function ‘__Pyx_set_iter_next’:
  /builddir/build/BUILD/cython-3.0.4/Cython/Compiler/FlowControl.c:55703:19: error: implicit declaration of function ‘_PySet_NextEntry’ [-Werror=implicit-function-declaration]
  55703 |         int ret = _PySet_NextEntry(iter_obj, ppos, value, &hash);
        |                   ^~~~~~~~~~~~~~~~

Also fix a test output check in Py3.13+: the error message starts with a capital letter in CPython.
@hroncok
Copy link
Contributor

hroncok commented Oct 27, 2023

I still get SystemError: Cannot modify a string currently used when trying to use this to build other things (e.g. lxml).

@scoder
Copy link
Contributor Author

scoder commented Oct 27, 2023

I still get SystemError: Cannot modify a string currently used when trying to use this to build other things (e.g. lxml).

Ok, I'll try it myself. Or do you have a quick indication where this happens? A stacktrace would help.

@hroncok
Copy link
Contributor

hroncok commented Oct 27, 2023

I have this traceback:

Traceback (most recent call last):
  ...
  File "/builddir/build/BUILD/lxml-4.9.3/setupinfo.py", line 180, in ext_modules
    result = cythonize(result, compiler_directives=cythonize_directives)
             ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
  File "/usr/lib64/python3.13/site-packages/Cython/Build/Dependencies.py", line 1154, in cythonize
    cythonize_one(*args)
  File "/usr/lib64/python3.13/site-packages/Cython/Build/Dependencies.py", line 1300, in cythonize_one
    result = compile_single(pyx_file, options, full_module_name=full_module_name)
             ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
  File "/usr/lib64/python3.13/site-packages/Cython/Compiler/Main.py", line 615, in compile_single
    return run_pipeline(source, options, full_module_name)
           ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
  File "/usr/lib64/python3.13/site-packages/Cython/Compiler/Main.py", line 539, in run_pipeline
    err, enddata = Pipeline.run_pipeline(pipeline, source)
                   ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
  File "/usr/lib64/python3.13/site-packages/Cython/Compiler/Pipeline.py", line 398, in run_pipeline
    data = run(phase, data)
           ^^^^^^^^^^^^^^^^
  File "/usr/lib64/python3.13/site-packages/Cython/Compiler/Pipeline.py", line 375, in run
    return phase(data)
           ^^^^^^^^^^^
  File "/usr/lib64/python3.13/site-packages/Cython/Compiler/Pipeline.py", line 34, in parse
    tree = context.parse(source_desc, scope, pxd = 0, full_module_name = full_module_name)
           ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
  File "/usr/lib64/python3.13/site-packages/Cython/Compiler/Main.py", line 381, in parse
    tree = Parsing.p_module(s, pxd, full_module_name)
           ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
  File "Cython/Compiler/Parsing.py", line 3912, in Cython.Compiler.Parsing.p_module
  File "Cython/Compiler/Parsing.py", line 3931, in Cython.Compiler.Parsing.p_module
  File "Cython/Compiler/Parsing.py", line 2460, in Cython.Compiler.Parsing.p_statement_list
  File "Cython/Compiler/Parsing.py", line 2409, in Cython.Compiler.Parsing.p_statement
  File "Cython/Compiler/Parsing.py", line 3611, in Cython.Compiler.Parsing.p_def_statement
  File "Cython/Compiler/Parsing.py", line 2485, in Cython.Compiler.Parsing.p_suite_with_docstring
  File "Cython/Compiler/Parsing.py", line 2460, in Cython.Compiler.Parsing.p_statement_list
  File "Cython/Compiler/Parsing.py", line 2429, in Cython.Compiler.Parsing.p_statement
  File "Cython/Compiler/Parsing.py", line 1908, in Cython.Compiler.Parsing.p_if_statement
  File "Cython/Compiler/Parsing.py", line 1922, in Cython.Compiler.Parsing.p_else_clause
  File "Cython/Compiler/Parsing.py", line 2474, in Cython.Compiler.Parsing.p_suite
  File "Cython/Compiler/Parsing.py", line 2481, in Cython.Compiler.Parsing.p_suite_with_docstring
  File "Cython/Compiler/Scanning.py", line 447, in Cython.Compiler.Scanning.PyrexScanner.next
  File "Cython/Plex/Scanners.py", line 126, in Cython.Plex.Scanners.Scanner.read
  File "Cython/Plex/Scanners.py", line 155, in Cython.Plex.Scanners.Scanner.scan_a_token
  File "Cython/Plex/Scanners.py", line 222, in Cython.Plex.Scanners.Scanner.run_machine_inlined
SystemError: Cannot modify a string currently used

@scoder
Copy link
Contributor Author

scoder commented Oct 27, 2023

It looks like the CPython devs also tightened the C-API checks, and while doing that, might have made a couple of … interesting decisions. I'll investigate, but I'm really annoyed by this.

@hroncok
Copy link
Contributor

hroncok commented Oct 27, 2023

I've tried to use the pure Python version of Cython to build lxml instead.

This is what I get now:

ImportError: /builddir/build/BUILD/lxml-4.9.3/src/lxml/etree.cpython-313-x86_64-linux-gnu.so: undefined symbol: _PyBytes_Join

@da-woods
Copy link
Contributor

da-woods commented Oct 27, 2023

SystemError: Cannot modify a string currently used

The missing check in unicode_modifiable (https://github.com/python/cpython/blob/main/Objects/unicodeobject.c#L1622) that isn't in our __Pyx_unicode_modifiable is

if (_PyUnicode_HASH(unicode) != -1)
        return 0;

Git blame says that this was added in 2011 so it isn't a new check, but I guess they're generating hashes more eagerly.

The associated comment says

// copied directly from unicode_object.c "unicode_modifiable
// removing _PyUnicode_HASH since it's a macro we don't have
//  - this is OK because trying PyUnicode_Resize on a non-modifyable
//  object will still work, it just won't happen in place

I believe that's still true. I think what failing is

__Pyx_INCREF(*p_left);
// copy 'right' into the newly allocated area of 'left'
#if PY_VERSION_HEX >= 0x030D0000
if (unlikely(PyUnicode_CopyCharacters(*p_left, left_len, right, 0, right_len) < 0)) return NULL;

The failure is:

  1. PyUnicode_CopyCharacters checks modifiable
  2. The "incref" will give it a refcount of 2 which will make it unmodifiable.

So that seems like a reasonable failure. I think we just need to move the incref after. Although actually I'm beginning to doubt the incref and I wonder if it should be a gotref No - it's right - it accounts for the reference we return. I think it should be last thing

@@ -3066,7 +3075,7 @@ static CYTHON_INLINE PyObject *__Pyx_PyUnicode_ConcatInPlaceImpl(PyObject **p_le
__Pyx_INCREF(*p_left);

// copy 'right' into the newly allocated area of 'left'
#if PY_VERSION_HEX >= 0x030D0000
#if PY_VERSION_HEX >= 0x030d0000
if (unlikely(PyUnicode_CopyCharacters(*p_left, left_len, right, 0, right_len) < 0)) return NULL;
#else
_PyUnicode_FastCopyCharacters(*p_left, left_len, right, 0, right_len);
Copy link
Contributor

@da-woods da-woods Oct 27, 2023

Choose a reason for hiding this comment

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

Can't propose a change because it goes outside what github considers the edited region, but this block should be:

        }

        // copy 'right' into the newly allocated area of 'left'
        #if PY_VERSION_HEX >= 0x030d0000
        if (unlikely(PyUnicode_CopyCharacters(*p_left, left_len, right, 0, right_len) < 0)) return NULL;
        #else
        _PyUnicode_FastCopyCharacters(*p_left, left_len, right, 0, right_len);
        #endif
        __Pyx_INCREF(*p_left);  // Incref accounts for the returned reference to *p_left
        return *p_left;

The incref shouldn't happen on the return NULL error path I believe.

…reference counting of the result and to make sure we only resize and copy into strings to which we own the only reference.

Background is that PyUnicode_CopyCharacters() gained an additional check in Py3.13a1 that conflicts with our previous safety net of early incref-ing the resized string.

See cython#5767 (comment)
@hroncok
Copy link
Contributor

hroncok commented Oct 29, 2023

I was able to build pyyaml (tests passed) and python-lxml (tests errored due to unrelated Python 3.13 incompatibility) with the pure Python version of Cython from this PR.

@hroncok
Copy link
Contributor

hroncok commented Oct 29, 2023

I was also able to build Cython itself.

@hroncok
Copy link
Contributor

hroncok commented Oct 29, 2023

And with the self-compiled Cython, I've successfully built a couple of other packages.

@hroncok
Copy link
Contributor

hroncok commented Oct 29, 2023

ImportError: /builddir/build/BUILD/lxml-4.9.3/src/lxml/objectify.cpython-313-x86_64-linux-gnu.so: undefined symbol: _PyStack_AsDict

@scoder scoder modified the milestones: 3.0.x, 3.0.5 Oct 29, 2023
@scoder
Copy link
Contributor Author

scoder commented Oct 30, 2023

I think this gets close enough to looking good that we should include it in 3.0.5. The test suite builds with some test failures. Given that Py3.13 is only in it's earliest alpha stage, I think it's important to unblock dependent projects by having a Cython release that allows them to build and test.

I'll merge this and fix the remaining issues later.

@scoder scoder merged commit fd96443 into cython:master Oct 30, 2023
83 checks passed
@scoder scoder deleted the py313_capi branch October 30, 2023 09:16
@hroncok
Copy link
Contributor

hroncok commented Oct 30, 2023

Thank you so much for getting this into a buildable state despite all the hurdles.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants