Public API review

[asvetlov]

@jurca, it would be nice if you review the public API.

Please feel free to ask questions, including silly ones, and raise wild proposals.
Now, the API is flexible; it can be changed without any consequences.
After making the first release, any backward incompatible change is painful.

[jurca]

@asvetlov I see you've already put a lot of work and thought into this! I'm just trying to do this along my dayjob 🙂.

1. num_workers - I don't think we should allow 0 (and default to 100). With networking tasks, my previous - now 6 years old - laptop could handle ~1_000 simultaneous asyncio networking tasks in a single thread when scraping. I think letting the caller always specify the number would be a safer option.
2. I find max_pending's naming to be potentially misleading given how it's used. What do you think about max_scheduled or max_submitted?
3. How do you feel about adding a max_througput=None parameter to __init__()? This could be useful for req/s rate-limited APIs. Executor would need to keep track of the earliest_start_time of the last work item submitted into the queue and increment its next_earliest_start_time by 1 / max_througput. A worker would then just wait until given work item's earliest_start_time. Note: next_earliest_start_time needs to be updated in submit[_nowait]() to current monotonic time if it is in the past.
4. Your current design of map and amap prevents use with on-the-fly generated iterables that cannot fit into memory - this is the case for crawlers and for processing large data batches, both of which relate to my job. While this does make the implementation simpler, support for this use case was one of my primary reasons to design Executor (and the original pipeline) the way I did. Please reconsider this.
5. jsbueno had suggested subclassing asyncio.TaskGroup, which could handle worker (and feeder) tasks for us, while also adding another way to utilize this. I think it's worth considering, but you might be better qualified than me to decide on this.

P.S.: I know it's not public API, but I think the _counter can be a local variable in __init__().

[asvetlov]

Thank you for the feedback, you spotted many questionable points that I wanted to discuss, plus you've added new ones.

I'll respond to every bullet a little later.

[asvetlov]

1. num_workers - I don't think we should allow 0 (and default to 100). With networking tasks, my previous - now 6 years old - laptop could handle ~1_000 simultaneous asyncio networking tasks in a single thread when scraping. I think letting the caller always specify the number would be a safer option.

0 is for 'default'. An alternative approach is disabling 0 and using None for default. I'm good with any proposal; max_pending (or whatever) should use the same notation. 1_000 tasks as the default is probably ok; but I have a guts feeling that high default tasks number requires on-demand tasks creation. ThreadPoolExecutor already does it, we can borrow the implementation quickly.

2. I find max_pending's naming to be potentially misleading given how it's used. What do you think about max_scheduled or max_submitted?

I used this name because in aiojobs I named the similar parameter as pending. I don't know which name is better. max_scheduled and max_submitted sounds like a number of .submit() calls, but this argument limits the size of the incoming queue and the number of pending (not processed yet) work items. I'm not native English speaker though, maybe I missed something.

3. How do you feel about adding a max_througput=None parameter to __init__()? This could be useful for req/s rate-limited APIs. Executor would need to keep track of the earliest_start_time of the last work item submitted into the queue and increment its next_earliest_start_time by 1 / max_througput. A worker would then just wait until given work item's earliest_start_time. Note: next_earliest_start_time needs to be updated in submit[_nowait]() to current monotonic time if it is in the past.

Sounds good, would you work on PR?

5. Your current design of map and amap prevents use with on-the-fly generated iterables that cannot fit into memory - this is the case for crawlers and for processing large data batches, both of which relate to my job. While this does make the implementation simpler, support for this use case was one of my primary reasons to design Executor (and the original pipeline) the way I did. Please reconsider this.

Yes, the design follows Executors (both thread and process executors). It is not fixed in stone, but I'm curious about the real use case. Could you sketch a crawler that requires non-eager .amap() implementation for example?

7. jsbueno had suggested subclassing asyncio.TaskGroup, which could handle worker (and feeder) tasks for us, while also adding another way to utilize this. I think it's worth considering, but you might be better qualified than me to decide on this.>

I don't think that a task group should shutdown the whole executor if, say, the feeder was cancelled. Again, maybe I've missed something; a code example could help a lot with future discussion.

P.S.: I know it's not public API, but I think the _counter can be a local variable in __init__().

Please elaborate on what you mean.

[jurca]

1. num_workers - I don't think we should allow 0 (and default to 100). With networking tasks, my previous - now 6 years old - laptop could handle ~1_000 simultaneous asyncio networking tasks in a single thread when scraping. I think letting the caller always specify the number would be a safer option.

0 is for 'default'. An alternative approach is disabling 0 and using None for default. I'm good with any proposal; max_pending (or whatever) should use the same notation. 1_000 tasks as the default is probably ok; but I have a guts feeling that high default tasks number requires on-demand tasks creation. ThreadPoolExecutor already does it, we can borrow the implementation quickly.

I think we misunderstood each other. My argument was for not having a default value for this parameter at all, because I think finding a universally sound value is not possible. Therefore I argue for letting the creator of Executor instance always choose the task-appropriate value. The point here is to restrict parallelism to a specific level, therefore the level should be specified.

2. I find max_pending's naming to be potentially misleading given how it's used. What do you think about max_scheduled or max_submitted?

I used this name because in aiojobs I named the similar parameter as pending. I don't know which name is better. max_scheduled and max_submitted sounds like a number of .submit() calls, but this argument limits the size of the incoming queue and the number of pending (not processed yet) work items. I'm not native English speaker though, maybe I missed something.

Neither am I 🙂. If, however, the name is already used elsewhere in the library, we should keep it for sake of consistency 👍.

3. How do you feel about adding a max_througput=None parameter to __init__()? This could be useful for req/s rate-limited APIs. Executor would need to keep track of the earliest_start_time of the last work item submitted into the queue and increment its next_earliest_start_time by 1 / max_througput. A worker would then just wait until given work item's earliest_start_time. Note: next_earliest_start_time needs to be updated in submit[_nowait]() to current monotonic time if it is in the past.

Sounds good, would you work on PR?

I'll be happy to! I'll try to implement it within a couple of days, hopefully I won't hit too many issues.

5. Your current design of map and amap prevents use with on-the-fly generated iterables that cannot fit into memory - this is the case for crawlers and for processing large data batches, both of which relate to my job. While this does make the implementation simpler, support for this use case was one of my primary reasons to design Executor (and the original pipeline) the way I did. Please reconsider this.

Yes, the design follows Executors (both thread and process executors). It is not fixed in stone, but I'm curious about the real use case. Could you sketch a crawler that requires non-eager .amap() implementation for example?

Consider a crawler that starts with a set of URLs to crawl. With each scraped page it extracts all links on that page and adds the not-seen-before ones to the set of URLs to crawl. The process then continues, with the set of URLs being grown from one side and reduced from another until no new link is discovered and all URLs have been processed.

7. jsbueno had suggested subclassing asyncio.TaskGroup, which could handle worker (and feeder) tasks for us, while also adding another way to utilize this. I think it's worth considering, but you might be better qualified than me to decide on this.>

I don't think that a task group should shutdown the whole executor if, say, the feeder was cancelled. Again, maybe I've missed something; a code example could help a lot with future discussion.

You're right, and I forgot about parallel usage of a single Executor instance. Let's not do this.

P.S.: I know it's not public API, but I think the _counter can be a local variable in __init__().

Please elaborate on what you mean.

Never mind, I realized this way workers will always have unique names when multiple Executors are created. My bad, sorry.