- Notifications
You must be signed in to change notification settings - Fork5.7k
Add Bootstrapping Logic toApplication.run_*
#4673
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to ourterms of service andprivacy statement. We’ll occasionally send you account related emails.
Already on GitHub?Sign in to your account
Uh oh!
There was an error while loading.Please reload this page.
Conversation
Also changes the default value for `bootstrap_retries` on polling
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
Im just asking questions at this point
@@ -409,12 +414,14 @@ def default_error_callback(exc: TelegramError) -> None: | |||
# updates from Telegram and inserts them in the update queue of the | |||
# Application. | |||
self.__polling_task = asyncio.create_task( | |||
self._network_loop_retry( | |||
network_retry_loop( | |||
is_running=lambda: self.running, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
why is that a lambda?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
whileeffective_is_running(): |
this line expects a callable. If we simply passis_runnig=self.running
and edit above line to
whileis_running:
then we have an infinite loop, b/c settingself.running=False
withinUpdater
won't change the value in the network retry loop :) There are probably a billion other ways do to this (pass an object with arunning
attribute to check, pass a dict with a corresponding entry, pass an event …). This one seemed rather straight forward to me, but I'm open to changing it. Accepting an instance of
classHasRunning(Protocol):running:bool
would sound like the next best thing to me at first glance
@Eldinnie
I hope that helps... |
and why did i tag Eldinnie instead of@Bibo-Joshi ? it's my memory playing tricks on me. lol. |
Thanks very much for your insights@tsnoam ! The bootstrapping phase is indeed still needed. It deletes the previous webhook if you run polling, sets a new webhook if your run in webhook mode and drops pending updates if requested. Do you recall why you chose indefinite retries over doing a limited number of retries (say, 3)?. Even though we have logging, indefinite retries can get you in a situation where the process is running in a healthy way (eagerly retrying to set the webhook) but the bot doesn't react at all. That sounds undesirable to me. A limitd number of retries OTOH would be a sane effort of getting things going whil still preventing that the process get's stuck in an unusable state without explicit indication to the user. Moreover I noticed that for webhook mode, the retries were kept at "none". I suspect that this was an oversight: The docstring was updated, but not the signature:https://github.com/python-telegram-bot/python-telegram-bot/pull/1018/files#r1948035215 |
tsnoam commentedFeb 9, 2025 • edited
Loading Uh oh!
There was an error while loading.Please reload this page.
edited
Uh oh!
There was an error while loading.Please reload this page.
we chose the option of indefinite retries (and afair, with appropriate logging). that way we make sure that the bot is properly started and will be started eventually. aborting seemed like too much, after all, the user will just need to restart the bot and come to the same "problematic" part of the initialization. so what's the point in that?
I'm not sure about it. The initialization phase is the for a reason. Skipping it might lead to a limbo state in which you don't know exactly what happens.
Could be an oversight. That much I don't remember. |
I would not skip it, but rather abort if the bootstrapping phase fails. Meaning that fater n < ∞ retries you either have a bot that's able to handle updates or the process has shut down. But okay, I get your reasoning and now have to decide what to make of it 😅 Thanks for the input! |
as long as you can get to an expected state, any solution is good. |
I think a finite number of retries is more than enough as long as the delay between them is large enough to eliminate intermittent connectivity problems as the source of potential problems. Eg. in my issue report the problem was that the service was brought up before network/DNS was online. This state is usually resolved within the first 10-20 seconds after boot. Having an infinite amount of retries by default sound dangerous and may instead hide problems which the user wants the be notified about (by the service exiting) |
So far I have implemented the initial retry-interval to 0 seconds for
but I see that the bootstrapping within
Note that the retry-loop retriesimmediately on timeout errors. On other errors, the interval is increased step by step up to 30 seconds: python-telegram-bot/telegram/ext/_utils/networkloop.py Lines 114 to 145 in315a97f
Specifying the retry-interval is currently not exposed to the user in the @septatrix would this + (customizable) finite number of retries be enough for you? |
I think having a default 1s interval is better than 0s - or at least something non-zero. Depending on how far along the network stack is the connections would try instantly instead of after a timeout so adding a small delay (such as the 1s) on our side is better.
On timeout errors this should not be a problem because the timeout itself will result in some delay (though it also wouldn't hurt to wait, it's just not necessary).
I agree. As long as the default interval is non-zero it should be fine for most people.
Yes, as far as I understand how this is hooked up now this would be enough for me. Small aside while peeking at the code I found a small curiosity, nothing urgent: I noticed the slack added to |
codecovbot commentedFeb 17, 2025 • edited
Loading Uh oh!
There was an error while loading.Please reload this page.
edited
Uh oh!
There was an error while loading.Please reload this page.
❌ 2 Tests Failed:
View the top 2 failed test(s) by shortest run time
To view more test analytics, go to theTest Analytics Dashboard |
updated the tests, ready for review :) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
Copilot reviewed 4 out of 4 changed files in this pull request and generated 1 comment.
Uh oh!
There was an error while loading.Please reload this page.
b0d22ac
intomasterUh oh!
There was an error while loading.Please reload this page.
Uh oh!
There was an error while loading.Please reload this page.
When ready,closes#4657.
This also changes the default value for
bootstrap_retries
forstart/run_polling
to 0. Previously the logic was roughlyI now changed this to
I think this is saner, and I'm not even sure if indefinite retries during the bootstrapping phase were ever actually intended …
ToDo
Application