python-trio / trio

Trio – a friendly Python library for async concurrency and I/O
https://trio.readthedocs.io
Other
6.12k stars 335 forks source link

Add Cython to CI #2942

Closed jakkdl closed 7 months ago

jakkdl commented 8 months ago

after some wrangling I've got Cython going in the CI. Though so much of CI is ~magic I probably have something configured badly.

I vote for testing cython<3 and cython>=3 on latest python, but if @njsmith or @Zac-HD or somebody else has any reasons to test older versions, or on non-ubuntu, or on other python versions (and pypy?). Could even go so far as to add it to ci.sh and test it on everything, which I think would add something like 4-7 seconds.

test_cython.pyx could of course also be much more extensive, though grepping for Cython I'm only getting a hit for the one comment in src/trio/_core_run.py so doesn't seem like there's a lot of code that has been specially adapted for Cython. Maybe catch some exceptions? pytest does not seem to be a thing for cython, so running the full test suite or whatever I think is out of the question.

jakkdl commented 8 months ago

pypy+cython seems to sort of be a thing: https://docs.cython.org/en/latest/src/userguide/pypy.html but I'm getting a fancy error locally, so if that's something we want to support we probably want it in the matrix (after fixing the error):

python -c 'import test_cython'
hello...
 world !
  + Exception Group Traceback (most recent call last):
  |   File "<string>", line 1, in <module>
  |   File "test_cython.pyx", line 20, in init test_cython
  |     main()
  |   File "test_cython.pyx", line 18, in test_cython.main
  |     trio.run(trio_main)
  |   File "/home/h/Git/trio/cython_ci/.tox/pypy3-cython/lib/pypy3.10/site-packages/trio/_core/_run.py", line 2274, in run
  |     raise runner.main_task_outcome.error
  |   File "test_cython.pyx", line 12, in trio_main
  |     async with trio.open_nursery() as nursery:
  |   File "/home/h/Git/trio/cython_ci/.tox/pypy3-cython/lib/pypy3.10/site-packages/trio/_core/_run.py", line 958, in __aexit__
  |     raise combined_error_from_nursery
  | exceptiongroup.ExceptionGroup: Exceptions from Trio nursery (1 sub-exception)
  +-+---------------- 1 ----------------
    | Traceback (most recent call last):
    |   File "test_cython.pyx", line 13, in test_cython.trio_main
    |     nursery.start_soon(foo)
    | AttributeError: 'NoneType' object has no attribute 'start_soon'
    +------------------------------------
codecov[bot] commented 7 months ago

Codecov Report

All modified and coverable lines are covered by tests :white_check_mark:

Comparison is base (bd19635) 99.64% compared to head (7860181) 99.64%. Report is 1 commits behind head on master.

Additional details and impacted files ```diff @@ Coverage Diff @@ ## master #2942 +/- ## ======================================= Coverage 99.64% 99.64% ======================================= Files 117 117 Lines 17564 17564 Branches 3166 3166 ======================================= Hits 17502 17502 Misses 43 43 Partials 19 19 ``` | [Files](https://app.codecov.io/gh/python-trio/trio/pull/2942?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=python-trio) | Coverage Δ | | |---|---|---| | [src/trio/\_core/\_run.py](https://app.codecov.io/gh/python-trio/trio/pull/2942?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=python-trio#diff-c3JjL3RyaW8vX2NvcmUvX3J1bi5weQ==) | `99.66% <ø> (ø)` | |
jakkdl commented 7 months ago

Looking around more for cython+pytest I found https://shwina.github.io/cython-testing/ and after messing around with that for a bit I made some progress and managed to run sync test functions, so "just" need to inject our async magic somewhere in there as well and I might close in on running the full test suite.

I also somehow triggered the error from #2908 due to running against an old trio version in my messing around, despite cython being >3, so I wanna track that down and potentially report it upstream.

But we can merge this for now, and then I'll open another PR if/when I make progress on running the full test suite.