Conversation
Bibo-Joshi
left a comment
There was a problem hiding this comment.
Please double check the implementations of (Ext)Bot and ApplicationBuilder. I'm pretty sure that the arguments to HTTPXRequest need adaption for the get_updates_request instances and some docstrings might need updating as well. This will probably also fix the failing tests.
| @@ -0,0 +1,5 @@ | |||
| other = "Httpx timeout improvements" | |||
There was a problem hiding this comment.
please give a short explanation of the change here and explain what users need to change if they manually build HTTPXRequest objects.
I thought about this, but came to the conclusion that that shouldn't be needed. They set the connection to 1, as expected, and since there is just one connection in the pool, it shouldn't matter that max_keepalive_connections is set to 20 now, since well. There is only one over all :D |
| @@ -0,0 +1,5 @@ | |||
| other = "Default httpx max_connections/pool size set to 256, and dropping setting max_keepalive_connections to the same amount. If you manually build the httpx request, be aware that this also applies to you. If you want your own limits, you can set them via httpx_kwargs, as explained in https://www.python-httpx.org/advanced/resource-limits/" | |||
There was a problem hiding this comment.
Do we want to mention the motivation behind the change too?
python-telegram-bot/src/telegram/_bot.py Lines 341 to 344 in 6752547 Here in line 342 I expect the connection pool to be 1 for the python-telegram-bot/src/telegram/ext/_applicationbuilder.py Lines 232 to 239 in 6752547 Here in line 256 we duplicate the default value 256. We could try to avoid that by not passing |
Co-authored-by: Harshil <37377066+harshil21@users.noreply.github.com>
Co-authored-by: Bibo-Joshi <22366557+Bibo-Joshi@users.noreply.github.com>
my bad, pool, mea culpa :D
|
Thank you for your patience ❤️ |
As discussed, we think its appropriate to change the default timeout and drop the max_keepalive_connections