https://codereview.appspot.com/110820049/diff/20001/asyncio/base_events.py File asyncio/base_events.py (right): https://codereview.appspot.com/110820049/diff/20001/asyncio/base_events.py#newcode133 asyncio/base_events.py:133: task_factory = tasks.Task Assuming this is going to be ...
9 years, 9 months ago
(2014-07-03 17:33:09 UTC)
#1
9 years, 9 months ago
(2014-07-03 22:19:25 UTC)
#2
https://codereview.appspot.com/110820049/diff/20001/asyncio/base_events.py
File asyncio/base_events.py (right):
https://codereview.appspot.com/110820049/diff/20001/asyncio/base_events.py#ne...
asyncio/base_events.py:133: task_factory = tasks.Task
On 2014/07/03 17:33:09, GvR wrote:
> Assuming this is going to be an official, supported API, please add a comment
> explaining what it's for. Is a user supposed to monkeypatch this spot or
> subclass one of the event loop classes? Should it return a subclass of Task or
> is duck typing allowed? What is the call signature for the factory?
I would suggest to modify BaseEventLoop.task_factory before creating the first
event loop (ex: before calling get_event_loop) to affect all threads, not only
the main thread.
You might want to only modify a single event loop, in this case you can modify
the instance (loop.task_factory = MyTask). IMO it's obvious and it should not be
documented.
I would not suggest duck typing because asyncio API is not really stable (I
still added minor attributes recently) and there are many private methods and
attributes which are important.
I propose to add a comment like:
# Factory to build Task objects, subclass of the Task class.
# Modify asyncio.BaseEventLoop.task_factory to affect all threads
# (don't modify directly loop.task_factory).
# Prototype of Task constructor:
# def Task(coroutine_object, *, loop=None) -> Task object
9 years, 9 months ago
(2014-07-03 23:30:21 UTC)
#3
https://codereview.appspot.com/110820049/diff/20001/asyncio/base_events.py
File asyncio/base_events.py (right):
https://codereview.appspot.com/110820049/diff/20001/asyncio/base_events.py#ne...
asyncio/base_events.py:133: task_factory = tasks.Task
On 2014/07/03 22:19:25, haypo_gmail wrote:
> On 2014/07/03 17:33:09, GvR wrote:
> > Assuming this is going to be an official, supported API, please add a
comment
> > explaining what it's for. Is a user supposed to monkeypatch this spot or
> > subclass one of the event loop classes? Should it return a subclass of Task
or
> > is duck typing allowed? What is the call signature for the factory?
>
> I would suggest to modify BaseEventLoop.task_factory before creating the first
> event loop (ex: before calling get_event_loop) to affect all threads, not only
> the main thread.
>
> You might want to only modify a single event loop, in this case you can modify
> the instance (loop.task_factory = MyTask). IMO it's obvious and it should not
be
> documented.
>
> I would not suggest duck typing because asyncio API is not really stable (I
> still added minor attributes recently) and there are many private methods and
> attributes which are important.
>
> I propose to add a comment like:
>
> # Factory to build Task objects, subclass of the Task class.
> # Modify asyncio.BaseEventLoop.task_factory to affect all threads
> # (don't modify directly loop.task_factory).
> # Prototype of Task constructor:
> # def Task(coroutine_object, *, loop=None) -> Task object
Somehow this feels like a hack, not up to the standards of API design we've
tried to keep up for Tulip. I think we need to discuss more whether this should
affect all loops in all threads, or whether it should be allowed to vary per
loop or per thread. And then we should (perhaps) define a proper API to change
it. Maybe it even belongs in the event loop *policy*?
A separate issue is that when you assign a function object (e.g. a lambda,
perhaps used to adapt the signature) to a class variable, it will become an
instance method. IOW:
BaseEventLoop.task_factory = Task
works, but
BaseEventLoop.task_factory = lambda *a, **k: Task(*a, **k)
will break because it is missing 'self'. That could cause a fair bit of
head-scratching...
On 2014/07/03 23:30:21, GvR wrote: > Somehow this feels like a hack, not up to ...
9 years, 8 months ago
(2014-07-04 20:38:11 UTC)
#4
On 2014/07/03 23:30:21, GvR wrote:
> Somehow this feels like a hack, not up to the standards of API design we've
> tried to keep up for Tulip. I think we need to discuss more whether this
should
> affect all loops in all threads, or whether it should be allowed to vary per
> loop or per thread. And then we should (perhaps) define a proper API to
change
> it. Maybe it even belongs in the event loop *policy*?
I rewrote the patch to move task_factory to the event loop policy. It solves
different issues.
The task factory is still copied to each event loop. but the attribute is now
private: BaseEventLoop._task_factory. The event loop is always available where
we need to create tasks, caling get_event_loop_policy().get_task_factory() might
be a little bit slower.
Since the task factory is set in the event loop policy, it becomes obvious that
all event loops (in all threads) will use the same task factory.
Moreover, an event loop policy can use a default task factory differen than
Task. For example, GreenEventLoopPolicy of greenio would use GreenTask.
In my greenio pull request to support Trollius, there is a tricky question on
the import order: should we import trollius or asyncio? If the task factory is
part of the event loop policy, we can have two policies: one for Trollus, one
for asyncio. The pull request:
https://github.com/1st1/greenio/pull/5#issuecomment-48050080
> A separate issue is that when you assign a function object (e.g. a lambda,
> perhaps used to adapt the signature) to a class variable, it will become an
> instance method.
This issue is also fixed with the event loop policy design. I added an unit test
to check that. The task factory doesn't look like a method anymore, it's now set
by event_loop_policy.set_task_factory().
(Or as I wrote, a custom event loop policy may use a different default task
factory.)
I also added documentation, as requested.
On 2014/07/07 15:51:41, yselivanov wrote: > I like this patch. > > I think we ...
9 years, 8 months ago
(2014-07-07 16:25:48 UTC)
#6
On 2014/07/07 15:51:41, yselivanov wrote:
> I like this patch.
>
> I think we should also add a 'create_task()' method to EventLoop and and
promote
> it as a preferred way of creating tasks (instead of importing and using Task
> class directly).
When I use greenio and redio, how can I GreenTask and RedTask simultaneously?
On 2014/07/07 16:25:48, songofacandy wrote: > On 2014/07/07 15:51:41, yselivanov wrote: > > I like ...
9 years, 8 months ago
(2014-07-07 16:43:15 UTC)
#7
On 2014/07/07 16:25:48, songofacandy wrote:
> On 2014/07/07 15:51:41, yselivanov wrote:
> > I like this patch.
> >
> > I think we should also add a 'create_task()' method to EventLoop and and
> promote
> > it as a preferred way of creating tasks (instead of importing and using Task
> > class directly).
>
> When I use greenio and redio, how can I GreenTask and RedTask simultaneously?
Combining things like greenio, pulsar or hypothetical redio, is a pretty serious
decision. You cannot just import them and hope that they will automatically
interact with each other. You need to import their Task classes, create your own
Task derived from them (or combine their functionality some other way) and
install your custom Task class through the event loop policy.
It's not going to be a popular thing though, because it's mostly an instrument
for frameworks writers. For instance, if I create a new framework called Red,
that is built on top of asyncio & redio and supports greenlets and other
features, I may choose to have a custom Task class for it. But regular users of
the Red framework will never need to know about it.
On 2014/07/07 15:51:41, yselivanov wrote: > I like this patch. Cool. > I think we ...
9 years, 8 months ago
(2014-07-07 19:33:42 UTC)
#8
On 2014/07/07 15:51:41, yselivanov wrote:
> I like this patch.
Cool.
> I think we should also add a 'create_task()' method to EventLoop and and
promote
> it as a preferred way of creating tasks (instead of importing and using Task
> class directly).
First I didn't want to add another method creating task objects, there are
already Task() and async(). But when I read again my patch, I found
"self._loop._task_factory(res, loop=self._loop)" ugly. The loop has to be
repeated twice, it looks like a trap to pass the wrong event loop. So I added
create_task().
I updated the patch for your suggestions.
songofacandy wrote:
> When I use greenio and redio, how can I GreenTask and RedTask simultaneously?
You have to choose between greenio and redio, or build you own custom YellowTask
(green+red).
https://codereview.appspot.com/110820049/diff/60001/asyncio/base_events.py File asyncio/base_events.py (right): https://codereview.appspot.com/110820049/diff/60001/asyncio/base_events.py#newcode155 asyncio/base_events.py:155: def create_task(self, coro): You should also update AbstractEventLoop with ...
9 years, 8 months ago
(2014-07-07 20:11:28 UTC)
#9
On 2014/07/07 20:11:28, yselivanov wrote: > You should also update AbstractEventLoop with a 'create_task' api ...
9 years, 8 months ago
(2014-07-07 21:30:40 UTC)
#10
On 2014/07/07 20:11:28, yselivanov wrote:
> You should also update AbstractEventLoop with a 'create_task' api definition
Ok, fixed in new patch. By the way, I added more missing methods in
AbstractEventLoop in a different commit (that I just pushed).
On 2014/07/07 21:30:40, haypo_gmail wrote: > On 2014/07/07 20:11:28, yselivanov wrote: > > You should ...
9 years, 8 months ago
(2014-07-07 21:46:30 UTC)
#11
On 2014/07/07 21:30:40, haypo_gmail wrote:
> On 2014/07/07 20:11:28, yselivanov wrote:
> > You should also update AbstractEventLoop with a 'create_task' api definition
>
> Ok, fixed in new patch. By the way, I added more missing methods in
> AbstractEventLoop in a different commit (that I just pushed).
Thanks! Looks good to me now.
Wow, I like the latest version (just the create_task()) method a lot! However, it requires ...
9 years, 8 months ago
(2014-07-08 00:06:24 UTC)
#12
Wow, I like the latest version (just the create_task()) method a lot! However,
it requires subclassing. Maybe that's not a problem if the subclass just
overrides create_task() though.
On 2014/07/08 00:06:24, GvR wrote: > Wow, I like the latest version (just the create_task()) ...
9 years, 8 months ago
(2014-07-08 00:12:53 UTC)
#13
On 2014/07/08 00:06:24, GvR wrote:
> Wow, I like the latest version (just the create_task()) method a lot! However,
> it requires subclassing. Maybe that's not a problem if the subclass just
> overrides create_task() though.
Eh, I completely missed the fact that Victor removed 'set_task_factory()' method
from policy in his latest patch.
I think that having *both* 'set_task_factory' and 'create_task' methods makes a
lot of sense.
'create_task()' is an API to create tasks for the given loop, no need to ever
override it.
'set_task_factory()' is a simple way of injecting custom Tasks into any
compliant event loop.
Guido wrote: > However, it requires subclassing. Maybe that's not a problem if the > ...
9 years, 8 months ago
(2014-07-08 00:40:17 UTC)
#14
Guido wrote:
> However, it requires subclassing. Maybe that's not a problem if the
> subclass just overrides create_task() though.
2014-07-08 2:12 GMT+02:00 <yselivanov@gmail.com>:
> I think that having *both* 'set_task_factory' and 'create_task' methods
> makes a lot of sense.
It looks like greenio and Pulsar define their own event loop (and
their event loop policy too) using subclassing.
create_task() is enough for them.
Yury wrote:
> 'set_task_factory()' is a simple way of injecting custom Tasks into any
> compliant event loop.
I would prefer to make the API evolve step by step. If someone asks
set_task_factory() with a real use case, we can still modify the code.
Right now, I don't see any user for set_task_factory().
@Guido, @Yury: So what do you prefer? create_task() only (event loop),
or create_task() + set_task_factory() (event loop + event policy)?
Should we stop mentionning the Task constructor in asyncio doc, in
favor of create_task() to be compliant with Pulsar and greenio? Or
does it still make sense to create directly Task objects?
On 2014/07/08 00:40:17, haypo_gmail wrote: > Guido wrote: > > However, it requires subclassing. Maybe ...
9 years, 8 months ago
(2014-07-08 01:31:55 UTC)
#15
On 2014/07/08 00:40:17, haypo_gmail wrote:
> Guido wrote:
> > However, it requires subclassing. Maybe that's not a problem if the
> > subclass just overrides create_task() though.
>
> 2014-07-08 2:12 GMT+02:00 <mailto:yselivanov@gmail.com>:
> > I think that having *both* 'set_task_factory' and 'create_task' methods
> > makes a lot of sense.
>
> It looks like greenio and Pulsar define their own event loop (and
> their event loop policy too) using subclassing.
>
> create_task() is enough for them.
>
> Yury wrote:
> > 'set_task_factory()' is a simple way of injecting custom Tasks into any
> > compliant event loop.
>
> I would prefer to make the API evolve step by step. If someone asks
> set_task_factory() with a real use case, we can still modify the code.
> Right now, I don't see any user for set_task_factory().
OK, I agree. Let's keep the policy API plain and simple.
> @Guido, @Yury: So what do you prefer? create_task() only (event loop),
> or create_task() + set_task_factory() (event loop + event policy)?
>
> Should we stop mentionning the Task constructor in asyncio doc, in
> favor of create_task() to be compliant with Pulsar and greenio?
I'm +1 for 'loop.create_task()' being the only public and safe api for creating
tasks.
On 2014/07/08 01:50:53, GvR wrote: > On 2014/07/08 01:31:55, yselivanov wrote: > > I'm +1 ...
9 years, 8 months ago
(2014-07-08 02:19:03 UTC)
#17
On 2014/07/08 01:50:53, GvR wrote:
> On 2014/07/08 01:31:55, yselivanov wrote:
> > I'm +1 for 'loop.create_task()' being the only public and safe api for
> creating
> > tasks.
>
> Agreed. LGTM!
Great ;) I'm really excited about this patch!
This will make it into 3.4.x, right?
On 2014/07/08 02:19:03, yselivanov wrote: > On 2014/07/08 01:50:53, GvR wrote: > > On 2014/07/08 ...
9 years, 8 months ago
(2014-07-08 02:34:01 UTC)
#18
On 2014/07/08 02:19:03, yselivanov wrote:
> On 2014/07/08 01:50:53, GvR wrote:
> > On 2014/07/08 01:31:55, yselivanov wrote:
> > > I'm +1 for 'loop.create_task()' being the only public and safe api for
> > creating
> > > tasks.
> >
> > Agreed. LGTM!
>
> Great ;) I'm really excited about this patch!
> This will make it into 3.4.x, right?
Yup. This is exactly the kind of API adjustments that the "provisional" status
was meant for.
Issue 110820049: Add asyncio.tasks.task_factory variable
Created 9 years, 9 months ago by haypo_gmail
Modified 9 years, 8 months ago
Reviewers: GvR, yselivanov, songofacandy
Base URL:
Comments: 7