Conversation
github-actions
bot
added
the
t-tooling
github-actions
bot
added
the
tested
vdusek
left a comment
vdusek
left a comment
There was a problem hiding this comment.
Just comments regarding the measure time util.
janbuchar
force-pushed
the
autoscaled-pool
branch
from
959af17 to
0f34f2c
Compare
Co-authored-by: Vlada Dusek <[email protected]>
Co-authored-by: Vlada Dusek <[email protected]>
vdusek
left a comment
vdusek
left a comment
There was a problem hiding this comment.
Maybe we will have a different opinion on how much we should document the code 🙂. However, I'd be glad if you could add at least some comments explaining what's going on in main functions like run(), autoscale, and worker. So that it's easier to understand it when we come back to this module. In a similar way as it is in the original TS implementation. Also, I'd like the class docstring to be extended to a similar length as it is in the TS implementation. Again, I believe you can copy the doc from there and just adjust it.
|
At first, good job 👏. I went through all of it, some parts were harder to understand, so I'd be glad if we could have more comments, as I wrote earlier, otherwise, it's great. To your question, let's discuss it on Slack... |
I filled in what seemed non-obvious. Please do point out if something is still unclear.
Honestly, the TS implementation seems overdocumented to me (can't believe that I said that 😄). It seems like a very internal thing and as such it should not need things like usage examples. Also, copying documentation between projects just feels wrong. |
vdusek
left a comment
vdusek
left a comment
There was a problem hiding this comment.
Nice! basically just formatting things, otherwise it's great
Co-authored-by: Vlada Dusek <[email protected]>
2 participants
AutoscaledPool#19The current concurrency model is "launch
desired_concurrencyworker tasks that execute the task function in a loop". Maybe we could achieve simpler and slightly more efficient code if each execution of the task function had its own asyncio task, similarly to the original implementation. Follow-up tasks would be started in thedone_callback, or in a recurring task (both of these would be needed).