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鈥檒l occasionally send you account related emails.

Already on GitHub? Sign in to your account

asyncio support #2

Open
vltr opened this Issue Apr 17, 2018 · 14 comments

Comments

Projects
None yet
4 participants
@vltr
Copy link

vltr commented Apr 17, 2018

Hi! I know that asyncio support is not yet implemented (as described on README), but to "get there" wouldn't be like adding some kind of contextvars task factory into the asyncio.Task.current_task using loop.set_task_factory? Thanks! 馃槂

@1st1

This comment has been minimized.

Copy link
Member

1st1 commented Apr 17, 2018

It's a bit more involved, as callbacks need to track context too (not just Tasks). I'll try to find time to implement support for it soon.

@vltr

This comment has been minimized.

Copy link
Author

vltr commented Apr 17, 2018

Thanks, @1st1 ! I thought this might be the way, just my two cents but, of course, reading the PEP and to be fully compliant, it'll need some hacking into Python ... Again, thanks a lot! I hope this comes to light soon 馃槈

@vltr vltr closed this Apr 17, 2018

@fantix

This comment has been minimized.

Copy link
Contributor

fantix commented Apr 28, 2018

Could we please keep this issue open before asyncio support lands? I'd like to reference this issue, but a closed one seems a little bit weird. 馃槇

@vltr

This comment has been minimized.

Copy link
Author

vltr commented Apr 28, 2018

@1st1 1st1 reopened this Apr 28, 2018

@fantix

This comment has been minimized.

Copy link
Contributor

fantix commented May 7, 2018

I've looked into this issue for solution, and found it tricky and requiring tradeoffs perhaps.

Approach 1

Backport python/cpython#5027 as it is, by providing a patched asyncio event loop (as well as Future and Task) in this package.

In CPython 3.7 contextvars works across different Tasks in a way that, each step of a Task is executed explicitly in the same Context owned by this Task. Therefore when multiplexing several Tasks, the current running Task always pushes its own Context into the stack first, then run the next step, then pop it out before switching back to event loop for the next Task to run. So that for the coroutines driven by Tasks, they feel like the Context is local to itself and isolated from other coroutines/Tasks. (Please correct me if my understanding was wrong)

To follow that in this backport, a patched Task would be essential to own a Context and drive steps within it. Therefore, call_soon() and friends should better follow the new API in PEP 567, that leads to providing a new event loop in this package - actually a set of patched default event loops corresponding to those in CPython asyncio. The same for Futures. As for performance, it might be better to also provide the C code version of them too. So it might just end up having almost a whole code base of asyncio from CPython 3.7 here in a backport package, in order to achieve 100% compatibility. Code copying smells, I'm eager to learn about any possibility to achieve the same thing by reusing the code, not copying it.

Approach 2

So I thought about what context actually means for asyncio:

  1. Context keeps the same values for a coroutine/Task, even if it is physically executed cooperatively with other coroutines alternatively.
  2. Context inherits, when creating new Tasks from a running Task.

This does not cover 100% of PEP-567 for asyncio, but I think it is the most important part. The idea was from PEP-550, we had some discussion in fantix/gino#84, and it led to aiocontextvars. Approach 2 is very much the same:

  1. Instead of tweaking Task._step(), Approach 2 modifies contextvars._get_context() to honor asyncio.current_task().
  2. Provides a Task factory that handles Context inheritance only.

It has the minimal impact, while providing the key feature. However, the defects are also obvious and fatal:

  1. The asyncio support is no longer a backport - no PEP-567 API, call_soon(), add_done_callback() and friends are not compatible with PEP-567.
  2. It requires additional effort to allow custom event loops like uvloop to use CPython 3.7 and this backport at the same time.
  3. It adds complexity to non-asyncio users of this backport.

Therefore I think Approach 2 is definitely a no-go, and listed here just for inspiration.

Approach 3

I'd love to see a different third approach here. Please shed some light on this, and I'm willing to contribute PoC/PR on it.

References:

@vltr

This comment has been minimized.

Copy link
Author

vltr commented May 10, 2018

@fantix , I know I'm just a spectator here, but as I understood from your ideas and some thoughts about:

Approach 1: We would end up with "patched" versions of asyncio, Future, Task and everything needed inside this package, something (roughly) like:

try:
    from contextvars import asyncio, ContextVar  # for Python 3.6 and 3.5
except ImportError:
    import asyncio
    from contextvars import ContextVar

Is that correct? IMO this works just fine, since I'm already hooking some task factories to manage some kind of context variables (Python 3.5 and 3.6). Even made a simpler approach to use with sanic-jwt on some specific occasions, so developers worried with this backport may already know they need to get their hands a little dirty ...

Approach 2: as you said in the end, a "no-go". But I think that aiocontextvars is pretty much usable (and works with custom event loops, doesn't it?). So, my question would be: why not merge them and provide additional documentation on how to use them? I have a footnote[1] explaining myself a little bit.

Approach 3: Well, I think I already made some thoughts on the first two approaches. Even a mashup of both would be interesting to see - if they provide this functionality, of course! 馃槄

1 I'm starting to think - @1st1 and @fantix, correct me if I'm wrong - that this backport of contextvars may be more complicated than initially thought (based also in your work, feedback, links provided and the actual implementation for Python 3.7). But, nevertheless, a nice feature to have. I wouldn't bother the need of some hacking to make it work instead of a simple drop-in 馃槈 Anyway, just my two cents ...

@fantix

This comment has been minimized.

Copy link
Contributor

fantix commented May 10, 2018

Thank you for the comments, @vltr! I鈥檒l reply in detail once I get to my keyboard later. While I guess Yury might be busy with PyCon and other stuff, let鈥檚 wait for his reply a bit later 馃槂


Approach 1: Yeah I think it has to be like that if we want to support something like this in Python < 3.7 with this backport:

fut = asyncio.Future()
fut.add_done_callbacks(some_cb, context=some_ctx)

Alternatively, I have two branch thoughts:

  1. One step forward - monkey patch asyncio, so that you don't need to worry about ImportError. But this might be a terrible idea. Also mentioned in python-trio/trio#420.
  2. Or one step backward - leave asyncio.Future or asyncio.Task as they were on module level, provide only an alternative PEP-567-compatible event loop. Because loop.create_task() or asyncio.ensure_future() should be preferred over using asyncio.Task directly in the first place.

Approach 2: Yes that is what worried me - there might be no clean way out. I'll be glad to use immutables in aiocontextvars or vice versa, but it is just a task-local workaround rather than a backport. Therefore I'd prefer to listen to some better ideas and directions here before making further moves. But yeah I agree with you that Python 3.5 and 3.6 deserve a de facto solution to deal with asyncio task locals.

@vltr

This comment has been minimized.

Copy link
Author

vltr commented May 11, 2018

Thanks for your reply, @fantix ! I'll add some more comments on that, I hope you don't mind 馃槈

One step forward - monkey patch asyncio, so that you don't need to worry about ImportError.

I may have a very strong opinion against monkey patching. When greenlet first appeared, I was reluctant enough to never use it until recently, just for the performance boost it gives to psycopg2 - I'm still a huge user of SQLAlchemy and have no intention on dropping it out soon - no offense here, I really see GINO with good eyes! The only monkey patching I really use is to implement a __json__method to objects because of ujson.

Or one step backward - leave asyncio.Future or asyncio.Task as they were on module level, provide only an alternative PEP-567-compatible event loop. Because loop.create_task() or asyncio.ensure_future() should be preferred over using asyncio.Task directly in the first place.

Please, for me this just seems right. I don't think that taking a step backward is bad for a a backport of Python 3.7 contextvars module 馃槃

Therefore I'd prefer to listen to some better ideas and directions here before making further moves.

Couldn't agree more, hence my comments here. Just to be clear, some of them are just personal opinions and have the sole purpose of giving the point of view of a developer that already use variables bound to tasks within asyncio - even if my approach looks stupidly simple 馃槄

@fantix

This comment has been minimized.

Copy link
Contributor

fantix commented May 11, 2018

Yeah I prefer your alternative asyncio module over monkey patching too, if asyncio.Future needs to be taken care of. Please forgive my non-native English, by "forward" and "backward" I mean "aggressive" and "less-intrusive" 馃槄 No worries your voice means a lot, keep up with the good work!

@vltr

This comment has been minimized.

Copy link
Author

vltr commented May 11, 2018

Please forgive my non-native English, by "forward" and "backward" I mean "aggressive" and "less-intrusive"

Don't worry about it, I understood what you meant perfectly! I'm not a native English speaker either ... In fact, I was just playing with these words 馃槅

@Suor

This comment has been minimized.

Copy link

Suor commented Dec 11, 2018

My two cents. There is no smell in copying code here - yes, we will have two different copies, but they are for different pythons and their asyncio versions are different anyway. This will probably require maintenance in the form of copying any security fixes if there will be any. This is trivial though. So if code could just be copied as is and won't require adaptation for older python this is a way to go.

Monkey patching is fine too because our targets python 3.5/3.6 asyncio won't change. This will require some hacking and would be heavier on docs - we would have old asyncio with some new features unlike simply new asyncio. Overall looks worse than just copying code.

P.S. asyncio should not have been included into standard lib so early.

@1st1

This comment has been minimized.

Copy link
Member

1st1 commented Dec 11, 2018

Yeah, we just need to copy a lot of code -- event loop & Task/Future implementations (both in Python and C). At this point I think it's easier to just use Python 3.7 (or we can make uvloop work with contextvars on 3.6 & 3.5 if someone is interested to work on the PR).

@Suor

This comment has been minimized.

Copy link

Suor commented Dec 11, 2018

This issue is of the kind of ones that solve themselves ;) Sample resolution: wait till Dec 2021, close it.

@vltr

This comment has been minimized.

Copy link
Author

vltr commented Dec 11, 2018

@Suor probably that would be the case 馃槈

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