Aiohttp request by Poolitzer · Pull Request #126 · python-telegram-bot/ptbcontrib

Poolitzer · 2025-03-26T19:29:31Z

This is my take on solving python-telegram-bot/python-telegram-bot#4560. I think the implementation is correct, I copied all appropriate httpx tests over.

There are open points I need feedback on:
Implementation:

read_timeout property needs to be implemented, but this doesn't make sense since aiohttp doesn't have that timeout. Why do even have this property do we actually need it? Should I just pass the most fitting timeout like I do?
same, do_request needs all the httpx timeout params. They dont make sense at all with aiohttp, should I raise an error? I just ignore them anyway lol

Tests:

the two init tests are skipped because I dont get at all which params are selected in the httpx test suit to be passed here and which aren't, can someone explain?
the last test is skipped for now because I want to understand if I have to build my own bot instance or not
the failing test test_multiple_init_cycles I don't understand at all why it fails. Because the async with block should (as implemented and kinda tested above) properly shut down and initialize the instance, why doesn't it
and the timeout test raise makes the text xfail which is a) really funny and b) also kinda a successful test in its own, right?

Bibo-Joshi

Hi. Thanks for taking the time to PR for that :) I hope the authors of python-telegram-bot/python-telegram-bot#4560 appreciate it :)

read_timeout property needs to be implemented, but this doesn't make sense since aiohttp doesn't have that timeout. Why do even have this property do we actually need it? Should I just pass the most fitting timeout like I do?

This was introduced in python-telegram-bot/python-telegram-bot#3963 and enables Bot.get_updates to use the default read timeout specified in the request object rather than hard-coding a custom default value.

same, do_request needs all the httpx timeout params. They dont make sense at all with aiohttp, should I raise an error? I just ignore them anyway lol

IMO you should ensure that aiohttprequest.do_request(…, read_timeout=aiohttprequest.read_timeout) behaves the same as aiohttprequest.do_request(…). If aiohttp has a different set of timeout settings, then you should map our parameters to them as good as possible.
If you do not use all of them, I'd personally recommend to raise a warning (in case the passed value differs from DEFAULT_NONE) but not raise an exception.

I did not look at the tests yet

ptbcontrib/aiohttp_request/README.md

Bibo-Joshi · 2025-03-26T19:52:35Z

ptbcontrib/aiohttp_request/aiohttprequest.py

+from telegram._utils.logging import get_logger
+from telegram._utils.types import ODVInput


please do not use ptb internals

I have to, otherwise I can't check if the parameter is a default instance, or?

we have https://docs.python-telegram-bot.org/en/stable/telegram.request.baserequest.html#telegram.request.BaseRequest.DEFAULT_NONE in place. for the typing. I would suggest to copy-paste from PTB code if that helps …

I tried that but then the isinstance call breaks because its not an instance of the copied class, it is of PTB.

no, I mean coyping the type annotation, not copying the DefaultValue class. for checking if the argument is DEFAULT_NONE, you shoul use if parameter is (not) BaseRequest.DEFAULT_NONE as described in the docs IMO :)

Bibo-Joshi · 2025-03-26T19:53:36Z

ptbcontrib/aiohttp_request/aiohttprequest.py

+from typing import Any, Optional, Union
+
+import aiohttp
+import yarl


this is not listed in requirements.txt

No aiohttp depends on it, it is how they type hint an URL in their library.

Do I need to explicitly add it? Or can I rely on it implicitly, since when they drop it we would realize it in unit tests...

I'd personally add it as requirement, yes, since you explicitly use it. but on ptbcontrib I'd be okay with skippen, then

Bibo-Joshi · 2025-03-26T19:54:48Z

ptbcontrib/aiohttp_request/aiohttprequest.py

+                This parameter is intended for advanced users that want to fine-tune the behavior
+                of the underlying ``aiohttp`` clientSession. The values passed here will override
+                all the defaults set by ``python-telegram-bot`` and all other parameters passed to
+                :class:`ClientSession`. The only exception is the :paramref:`media_write_timeout`
+                parameter, which is not passed to the client constructor.
+                No runtime warnings will be issued about parameters that are overridden in this
+                way.


please update this. AiohttpRequest is no PTB class and there is also no parameter media_write_timeout here.

What do you mean with no PTB class? PTB will still pass defaults to this class, and passing this param will override these defaults

not a class provided through the PTB package. so it's not used in the ApplicationBuilder and the defaults are not set by PTB, but by ptbcontrib :)

well if I add it like in the ReadMe, applicationbuilder still adds its default timeouts

No, ApplicationBuilder will not build instances of AiohttpRequest, it doesn't know how to. you'd have to call https://docs.python-telegram-bot.org/en/stable/telegram.ext.applicationbuilder.html#telegram.ext.ApplicationBuilder.request and get_updates_request, passing a ready-build instance.

yeah okay. Though get_updates does kinda change the timeouts by default, which is a bit unfitting for this module, but it doesn't really matter since it just increases the timeout.

Bibo-Joshi · 2025-03-26T19:55:30Z

ptbcontrib/aiohttp_request/aiohttprequest.py

+        # this is needed because there are errors if one uses async with or a normal def
+        # with ApplicationBuilder, apparently. I am confused. But it works.


what is "this" here? I am confused about what you're confused about.

The try except part around which loop function to use, you need one with the async def situation, and one for with ApplicationBuilder.

maybe this is related to python-telegram-bot/python-telegram-bot#4875?

yeah same loop setup there

ptbcontrib/aiohttp_request/aiohttprequest.py

Bibo-Joshi · 2025-05-03T18:12:36Z

@Poolitzer are you still interested in working on this?

Poolitzer · 2025-05-03T21:09:28Z

yes just got busy

Poolitzer · 2025-07-27T09:37:52Z

If aiohttp has a different set of timeout settings, then you should map our parameters to them as good as possible.

Well the issue is basically httpx (and thus PTB usage where someone sets custom timeouts) expects read, write, connect, pool timeout to basically add together to an overall timeout.

Aiohttp otoh has a total timeout for everything (hence by default I only set that, and to 15, which is the 5 default timeouts httpx implementation has summed up). Then it has further limits for some other parts of the connection process (for reverence).

I will implement it now as you said with a best effort match basis, but for the future it would make more sense to have one "timeout" param and each implementation can decide what they want to pass there in what form.

Co-authored-by: Bibo-Joshi <22366557+Bibo-Joshi@users.noreply.github.com>

Bibo-Joshi

I only looked at the diff since my last review. Two minor findings

ptbcontrib/aiohttp_request/aiohttprequest.py

Co-authored-by: Bibo-Joshi <22366557+Bibo-Joshi@users.noreply.github.com>

Bibo-Joshi · 2025-08-02T19:36:56Z

Thank you very much for the contribution!

Poolitzer added 4 commits March 25, 2025 19:29

Feat: aiohttp request implementation

c04f0d5

Fix: Wrong Networkerror class name debugging

540aee2

Feat: Added tests

4eab8f3

Fix: Remove httpx comments

8810db5

Bibo-Joshi requested changes Mar 26, 2025

View reviewed changes

Poolitzer and others added 3 commits July 27, 2025 14:55

Applying review comments

ceb584c

Apply suggestions from Bibo

e947f24

Co-authored-by: Bibo-Joshi <22366557+Bibo-Joshi@users.noreply.github.com>

Fix: Improve docstring of client_kwargs

df40094

Bibo-Joshi self-requested a review August 2, 2025 15:05

Bibo-Joshi requested changes Aug 2, 2025

View reviewed changes

ptbcontrib/aiohttp_request/aiohttprequest.py Outdated Show resolved Hide resolved

ptbcontrib/aiohttp_request/aiohttprequest.py Outdated Show resolved Hide resolved

Apply suggestions from hinrich

d258e15

Co-authored-by: Bibo-Joshi <22366557+Bibo-Joshi@users.noreply.github.com>

Bibo-Joshi approved these changes Aug 2, 2025

View reviewed changes

Merge branch 'main' into aiohttpRequest

8292c52

Bibo-Joshi merged commit 7dec4c1 into main Aug 2, 2025
10 of 11 checks passed

Bibo-Joshi deleted the aiohttpRequest branch August 2, 2025 19:36

		from telegram._utils.logging import get_logger
		from telegram._utils.types import ODVInput

		# this is needed because there are errors if one uses async with or a normal def
		# with ApplicationBuilder, apparently. I am confused. But it works.

Conversation

Poolitzer commented Mar 26, 2025

Uh oh!

Bibo-Joshi left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Poolitzer Jul 27, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Bibo-Joshi commented May 3, 2025

Uh oh!

Poolitzer commented May 3, 2025

Uh oh!

Poolitzer commented Jul 27, 2025

Uh oh!

Bibo-Joshi left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Bibo-Joshi commented Aug 2, 2025

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

Poolitzer Jul 27, 2025 •

edited

Loading