Skip to content

Conversation

@jakirkham
Copy link
Member

@jakirkham jakirkham commented Apr 29, 2021

As inspect.iscoroutinefunction doesn't work on Cythonized async functions, use asyncio.iscoroutinefunction instead. Internally it already calls inspect.iscoroutinefunction as part of the check it performs. So there is no need to do this in addition to inspect.iscoroutinefunction as that is already handled.

TODO: Re-enable Cythonized Scheduler tests fixed by this (in particular the ones in test_failed_workers) as noted below

xref: #4764 (comment)

cc @jrbourbeau

  • Closes #xxxx
  • Tests added / passed
  • Passes black distributed / flake8 distributed / isort distributed

As `inspect.iscoroutinefunction` doesn't work on Cythonized `async`
functions, use `asyncio.iscoroutinefunction` instead. Internally it
already calls `inspect.iscoroutinefunction` as part of the check it
performs. So there is no need to do this in addition to
`inspect.iscoroutinefunction` as that is already handled.
@jakirkham
Copy link
Member Author

James, do you recall if there were particular tests affected by this?

@jrbourbeau
Copy link
Member

I think most (if not all) of the tests we xfailed in distributed/tests/test_failed_workers.py were affected by this. FWIW, I tried switching to asyncio.iscoroutinefunction locally yesterday but don't think cython/cython#3427 has been included in a cython=0.29.* release yet

@jakirkham
Copy link
Member Author

Thanks for checking James 🙂

Good point. Missed that. Asked upstream if we could backport that PR to 0.29

Would it make sense to add this anyways? Or should we hold off?

@jrbourbeau
Copy link
Member

Thanks for asking for a backport -- that would be great. I'm slightly inclined to hold off for now just so we can keep this PR around as a reminder to remove the relevant xfails when there's a cython release with cython/cython#3427. Though no strong opinion either way

@jakirkham
Copy link
Member Author

Yep sounds reasonable to me. Will go ahead and mark it as a draft then

@jakirkham jakirkham marked this pull request as draft April 29, 2021 17:24
@jakirkham jakirkham changed the title Use asyncio.iscoroutinefunction [WIP] Use asyncio.iscoroutinefunction Apr 29, 2021
@da-woods
Copy link

da-woods commented May 3, 2021

@jakirkham https://github.com/cython/cython/blob/master/CHANGES.rst#features-added-18

Maybe of interest:

  • When generators are used in a Cython module and the module imports the modules "inspect" and/or "asyncio", Cython enables interoperability by patching these modules during the import to recognise Cython's internal generator and coroutine types. This can be disabled by C compiling the module with "-D CYTHON_PATCH_ASYNCIO=0" or "-D CYTHON_PATCH_INSPECT=0"
  • When generators or coroutines are used in a Cython module, their types are registered with the Generator and Coroutine ABCs in the collections or collections.abc stdlib module at import time to enable interoperability with code that needs to detect and process Python generators/coroutines.

I haven't really looked into this too much so if you have unanswered questions then the cython-users mailing list may be more helpful than me. I think the monkey-patching applies if you import inspect/asyncio from a Cython module. But I imagine it's possible to use one or more of these features to get a workable test on the current version of Cython. The abc test is probably the most useful of these?

@jakirkham jakirkham mentioned this pull request Apr 12, 2022
3 tasks
@jakirkham jakirkham changed the title [WIP] Use asyncio.iscoroutinefunction Use asyncio.iscoroutinefunction Jun 11, 2022
@jakirkham jakirkham marked this pull request as ready for review June 11, 2022 00:39
@jakirkham jakirkham closed this Jun 11, 2022
@jakirkham jakirkham reopened this Jun 11, 2022
@jakirkham jakirkham requested a review from jrbourbeau June 11, 2022 00:40
@github-actions
Copy link
Contributor

github-actions bot commented Jun 11, 2022

Unit Test Results

See test report for an extended history of previous test failures. This is useful for diagnosing flaky tests.

       15 files  ±0         15 suites  ±0   6h 30m 21s ⏱️ + 25m 58s
  2 863 tests ±0    2 782 ✔️ +2    81 💤 +1  0  - 1 
21 210 runs   - 2  20 270 ✔️ +2  940 💤  - 1  0  - 1 

Results for commit 50a1d40. ± Comparison against base commit 058e629.

♻️ This comment has been updated with latest results.

@jakirkham jakirkham deleted the use_asyncio_iscoroutinefunction branch March 17, 2023 17: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.

4 participants