Thriftpy / thriftpy2

Pure python approach of Apache Thrift.
MIT License
572 stars 91 forks source link

"@coroutine" decorator is deprecated since Python 3.8, use "async def" instead #144

Closed pearu closed 6 months ago

pearu commented 3 years ago

When importing thriftpy2.contrib.aio modules with Python 3.8 or newer, a large number of deprecation warnings is trigger.

Reproducer:

Python 3.8.6 | packaged by conda-forge | (default, Oct  7 2020, 19:08:05) 
[GCC 7.5.0] on linux
Type "help", "copyright", "credits" or "license" for more information.
>>> import warnings
>>> warnings.simplefilter("always")  # this appears to be default setting when using pytest
>>> import thriftpy2.contrib.aio.rpc
/home/pearu/miniconda3/envs/rbc-new-numba/lib/python3.8/site-packages/thriftpy2/contrib/aio/processor.py:13: DeprecationWarning: "@coroutine" decorator is deprecated since Python 3.8, use "async def" instead
  def process_in(self, iprot):
<snip about 60 DeprecationWarning messages>

It is possible that replacing asyncio.coroutine with types.coroutine when using 3.8+ will fix this issue [untested].

Thanks for thriftpy2!

ethe commented 3 years ago

I am not sure that replacing asyncio.coroutine with types.coroutine would not break the compatibility of Python 3.4 or lower version. I will test it later.

aisk commented 3 years ago

Python3.5 and former versions is already dead: https://python-release-cycle.glitch.me/ , so I think it's ok to migrate to using async def.

pearu commented 3 years ago

I am not sure that replacing asyncio.coroutine with types.coroutine would not break the compatibility of Python 3.4 or lower version. I will test it later.

It will likely break the compatibility, hence one can use the following idiom:

if sys.version_info[:2] < (3, 8):
    from asyncio import coroutine
else:
    from types import coroutine

@coroutine
def foo(...):
    ...
aisk commented 3 years ago

@pearu Hi I saw some answer on stackoverflow, said they have a little different behaviors: https://stackoverflow.com/a/49477233

I think migrate to async def is a good choice since Python 3.4 and former version is already dead, what do you think? @ethe

ethe commented 3 years ago

https://pypistats.org/packages/thriftpy2 shows that there are some users still use 3.4 in current, if we migrate to async def, I think we should release a major version.

aiudirog commented 3 years ago

@ethe I can handle the conversion to async/await if you decide to go for it. I'm already very familiar with the async implementation from my previous work.

ethe commented 3 years ago

@aiudirog thank you so much, i am glad to see your help.

aiudirog commented 3 years ago

@ethe I don't think its necessary to have a major version release. The asyncio tests aren't even run on 3.4, so it's not like there was actually a guarantee the code worked for them.

I've almost got a PR ready for conversion. I've also gone ahead and added 3.8, 3.9, and PyPy3 to the test environments to ensure we're not breaking anything in the future. My last task is cleaning up some of the warnings from the previous timeout deprecation.

Edit: I made the rookie mistake of pulling from my fork instead of upstream so I didn't see that you had already added testing environments for 3.8 and 3.9. I have still added PyPy3 though.

aiudirog commented 3 years ago

I also just noticed that flake8 is only being run on the tests, would you like me to get everything else passing as well?

ethe commented 3 years ago

@aiudirog it looks good to me, thanks