unskip and fix some passing tests by mattip · Pull Request #5429 · cython/cython

mattip · 2023-05-11T07:39:18Z

PyPy3.9 passes these tests, once the sys.getrefcount part is skipped. I am working on getting test_asyncgen to pass so grpcio work with PyPY, see pypy issue 3280 and pypy issue 3935

mattip · 2023-05-11T10:27:30Z

cool, pypy3.9 tests pass on CI.

mattip · 2023-05-11T14:34:52Z

The pure-python test_asyncgen.py passes pypy -m pytest test_asyncgen.py but runtests.py is failing. There are two out-of-bound indexes errors and five badly filled in return value failures

Cython/Utility/AsyncGen.c

da-woods · 2023-05-11T18:29:35Z

Cython/Utility/AsyncGen.c


    o->ag_hooks_inited = 1;

+#if CYTHON_COMPILING_IN_CPYTHON


I'm not completely sure, but I wonder if CYTHON_USE_TYPE_SLOTS would be the more appropriate check here (and later)

I am not sure, I thought CYTHON_USE_TYPE_SLOTS is for the documented dunder slots like tp_str -> __str__ as generated by the typeslots.py code generator.

... which generates these slot functions

CYTHON_USE_TYPE_SLOTS is for code that directly uses the tp_* slot functions of types, not for attributes on the thread state. There's CYTHON_FAST_THREAD_STATE, but I don't think that applies here, either. That guard controls the way we get at the thread state, more that what we do with it (although that might still depend on how fast we can get our hands on it when we need it).

If this it PyPy specific code, why not use CYTHON_COMPILING_IN_PYPY?

mattip · 2023-05-14T20:20:49Z

There are a number of failing tests in code like

ZERO = 0

async def gen():
    yield 123
    1 / ZERO

def test_last_yield(g):
    ai = g.__aiter__()
    an = ai.__anext__()
    try:
        next(an)          #  <---- this becomes __Pyx_PyIter_Next2()
    except StopIteration as ex:
        assert ex.args[1] == 123      # <- fails when not using CYTHON_USE_TYPE_SLOTS
    else:
        assert False, "no exception raised

I think the failure is due to a difference between PyIter_Next and tp_iternext(iterator) in __Pyx_PyIter_Next2. On the last iteration, PyIter_Next returns NULL while clearing the StopIteration error, but tp_iternext(iterator) does not clear the error. The StopIteration exception is regenerated, since NULL has been returned, but clearing the error removes the yield value from the exception, and so the args tuple is empty.

scoder · 2023-05-17T08:10:10Z

I think the failure is due to a difference between PyIter_Next and tp_iternext(iterator) in __Pyx_PyIter_Next2. On the last iteration, PyIter_Next returns NULL while clearing the StopIteration error, but tp_iternext(iterator) does not clear the error. The StopIteration exception is regenerated, since NULL has been returned, but clearing the error removes the yield value from the exception, and so the args tuple is empty.

So, you're saying that calling PyIter_Next() (when not using type slots) is wrong here, because it swallows the StopIteration exception? I don't think there is a way to use this protocol without either using type slots or calling PyIter_Next()…

Does PyPy fill the tp_iternext type slot?

mattip · 2023-05-17T09:58:33Z

Does PyPy fill the tp_iternext type slot?

Yes. I added a fix in 859a9a5 to use the type slot on PyPy, with a workaround for PyPy not having _PyObject_NextNotImplemented (I think the code intention there is to avoid calling _PyObject_NextNotImplemented since it knows what the outcome will be)

scoder

Most of this is good to have. I left comments on changes that I found questionable.

scoder · 2023-05-21T09:44:34Z

Cython/Utility/AsyncGen.c


    o->ag_hooks_inited = 1;

+#if CYTHON_COMPILING_IN_CPYTHON


CYTHON_USE_TYPE_SLOTS is for code that directly uses the tp_* slot functions of types, not for attributes on the thread state. There's CYTHON_FAST_THREAD_STATE, but I don't think that applies here, either. That guard controls the way we get at the thread state, more that what we do with it (although that might still depend on how fast we can get our hands on it when we need it).

If this it PyPy specific code, why not use CYTHON_COMPILING_IN_PYPY?

scoder · 2023-05-21T09:45:18Z

Cython/Utility/AsyncGen.c

-
    finalizer = tstate->async_gen_finalizer;
+#else
+    finalizer = _PyEval_GetAsyncGenFinalizer();


These were removed from CPython a year ago: python/cpython#32017
That probably means they are not future proof.

The code-path is for non-CPython compilation. Is there a better way to work around the opaque PyThreadState fields?

The code-path is for non-CPython compilation.

Right, but the fact that CPython had them and removed them means that other implementation won't necessarily have them either. As long as only PyPy is known to have them, I'd use the PYPY guard instead of the CPYTHON guard here.

Thefunctions were removed from CPython in favour of sys.get_asyncgen_hooks(), which sounds rather annoying to call from C, but we could do that here.

Is there a better way to work around the opaque PyThreadState fields?

It's an opaque struct in the Limited API, not in regular CPython.

Cython/Utility/ObjectHandling.c

mattip · 2023-05-21T20:48:56Z

I related to the review comments. Still not resolved: using getter functions _PyEval_GetAsyncGenFinalizer and _PyEval_GetAsyncGenFirstiter rather than direct acess to the fields.

I had to reskip the test: there is too much missing in PyPy to support yield and exception handlers in coroutines.

mattip · 2023-05-22T06:05:07Z

CI is passing. I will revert using a nightly build in CI (commit fcf1695) once PyPy releases a version that supports _PyEval_GetAsyncGenFinalizer and _PyEval_GetAsyncGenFirstiter, which in turn are needed to support compilation of grpcio.

scoder

As long as we keep this PyPy specific, it should be safe to merge. As it stands, it also impacts other implementations.

Cython/Utility/ModuleSetupCode.c

scoder · 2023-05-22T19:00:29Z

Cython/Utility/AsyncGen.c

-
    finalizer = tstate->async_gen_finalizer;
+#else
+    finalizer = _PyEval_GetAsyncGenFinalizer();


The code-path is for non-CPython compilation.

Right, but the fact that CPython had them and removed them means that other implementation won't necessarily have them either. As long as only PyPy is known to have them, I'd use the PYPY guard instead of the CPYTHON guard here.

Thefunctions were removed from CPython in favour of sys.get_asyncgen_hooks(), which sounds rather annoying to call from C, but we could do that here.

Is there a better way to work around the opaque PyThreadState fields?

It's an opaque struct in the Limited API, not in regular CPython.

mattip · 2023-05-22T20:30:37Z

Thefunctions were removed from CPython in favour of sys.get_asyncgen_hooks(), which sounds rather annoying to call from C, but we could do that here.

That is actually what the PyPy implementation does. Would it be OK if I make the code path with the pypy-specific functions pypy-only?

mattip · 2023-05-23T19:52:35Z

close/reopen to trigger CI

scoder

LGTM now. I'll wait for CI and then merge this.

Cython/Utility/ObjectHandling.c

mattip · 2023-05-24T05:58:09Z

Thanks!

mattip · 2023-06-02T08:08:26Z

As commented in pypy issue 3280

the above patch (mattip: commit 00113c3) on top of 0.29.35 allows GRPC_PYTHON_BUILD_WITH_CYTHON=1 pip install 'grpcio<1.43' --no-build-isolation to build and our rpc tests pass with this grpcio module.

Would it be OK to backport this to the 0.29.x branch?

scoder · 2023-06-03T05:58:39Z

Would it be OK to backport this to the 0.29.x branch?

Yes, that would be fine. The whole patch might not apply cleanly, but at least the test part is not strictly needed then.

Currently require a nightly PyPy build for Py3.9 to support async iteration and finalisation. Avoid PyIter_Next() and call tp_iternext() instead because PyIter_Next() swallows StopIteration exceptions and thus looses the return value. See https://foss.heptapod.net/pypy/pypy/-/issues/3280 See https://foss.heptapod.net/pypy/pypy/-/issues/3935

* Fixes for PyPy (GH-5429) Currently require a nightly PyPy build for Py3.9 to support async iteration and finalisation. Avoid PyIter_Next() and call tp_iternext() instead because PyIter_Next() swallows StopIteration exceptions and thus looses the return value. See https://foss.heptapod.net/pypy/pypy/-/issues/3280 See https://foss.heptapod.net/pypy/pypy/-/issues/3935 * change MACOSX_DEPLOYMENT_TARGET to 11.0

mattip commented May 11, 2023

View reviewed changes

Cython/Utility/AsyncGen.c Show resolved Hide resolved

da-woods reviewed May 11, 2023

View reviewed changes

mattip mentioned this pull request May 14, 2023

PyIter_Next clears a StopException, where tp_iternext does not capi-workgroup/problems#29

Open

mattip force-pushed the pypy-tests branch from 37f7fc3 to aa7a161 Compare May 16, 2023 10:58

scoder reviewed May 21, 2023

View reviewed changes

mattip added 7 commits May 21, 2023 23:43

unskip and fix some passing tests

6e34e09

get tests to run on a branch of PyPy

00113c3

hack to avoid PyIter_Next and call tp_iternext instead

4182bf4

use a nightly build that has the function needed in 28fb74a

fcf1695

use tp_finalize on PyPy

4a2c544

fix assert syntax in error_pos test

36b212e

fixes from review, reskip failing test with explaination

2034c41

mattip force-pushed the pypy-tests branch from e71fa61 to 2034c41 Compare May 21, 2023 20:43

mattip marked this pull request as ready for review May 21, 2023 20:44

scoder reviewed May 22, 2023

View reviewed changes

mattip added 2 commits May 22, 2023 23:36

fixes from review

5fa3935

limit CYTHON_USE_TP_FINALIZE to pypy 7.3.12 and above

fbc5750

mattip closed this May 23, 2023

mattip reopened this May 23, 2023

scoder approved these changes May 23, 2023

View reviewed changes

Cython/Utility/ObjectHandling.c Show resolved Hide resolved

scoder added this to the 3.0 milestone May 24, 2023

scoder added Testing PyPy labels May 24, 2023

scoder merged commit f892686 into cython:master May 24, 2023

mattip mentioned this pull request May 29, 2023

use pypy-3.9-v7.3.12rc2 instead of a nightly #5459

Merged

mattip mentioned this pull request Jun 4, 2023

Backport fixes for PyPy (GH-5429) #5465

Merged

Uh oh!

Conversation

mattip commented May 11, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

mattip commented May 11, 2023

Uh oh!

mattip commented May 11, 2023

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

mattip commented May 14, 2023

Uh oh!

scoder commented May 17, 2023

Uh oh!

mattip commented May 17, 2023

Uh oh!

scoder left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

mattip commented May 21, 2023

Uh oh!

mattip commented May 22, 2023

Uh oh!

scoder left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

mattip commented May 22, 2023

Uh oh!

mattip commented May 23, 2023

Uh oh!

scoder left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

mattip commented May 24, 2023

Uh oh!

mattip commented Jun 2, 2023

Uh oh!

scoder commented Jun 3, 2023 via email

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

mattip commented May 11, 2023 •

edited

Loading