Skip to content

[FEATURE] Customizing concurrent handling of updates #3509

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 our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Closed
Bibo-Joshi opened this issue Jan 19, 2023 · 5 comments · Fixed by #3654
Closed

[FEATURE] Customizing concurrent handling of updates #3509

Bibo-Joshi opened this issue Jan 19, 2023 · 5 comments · Fixed by #3654
Assignees
Labels
ℹ️ needs-wiki-update information: needs-wiki-update
Milestone

Comments

@Bibo-Joshi
Copy link
Member

What kind of feature are you missing? Where do you notice a shortcoming of PTB?

Setting ApplicationBuilder.concurrent_updates(True) allows to process all updates concurrently (up to a specified number at a time). What you can't do is e.g. processing updates from all users & chats concurrently while processing the updates for each user/chat sequentially. Having the possibility for such a mechanism would e.g. allow to

  • handle updates concurrently except those that are revelant for stateful handlers such as ConversationHandler
  • apply a rate-limiting mechanism for users by delaying processing of their updates
  • prioritizing certain types of updates for handling, e.g. inline/callback queries over regular messages

Describe the solution you'd like

  1. Introduce an interface class BaseThrottler (other naming suggestions are welcome!). This class should
    1. accept an integer that specifies the maximum overall number of updates that may be processed concurrently
    2. have a single abstract method async def process_update(update, coroutine) whose task it is to await at an appropriate time, that may be determined depending on the update
    3. have a single non-abstract method asyne def do_process_update that calls process_update after applying a global semaphore, which enforces the limit in a.
  2. Introduce a class SimpleThrottler(BaseThrottler) (other naming suggestions are welcome) for which process_update immediately awaits the coroutine, i.e. does not apply any additional throttling
  3. The behavior of Application.concurrent_updates(…) should be as follows:
    1. passing an int will make use of the SimpleThrottler with the respective max overall number of concurrent updates
    2. passing True will behave as a. with the current default number specified for that case
    3. passing an instace of BaseThrottler will use that instance for handling concurrency
  4. Application.concurrent_updates should return the max overall number of concurrent updates as specified for the throttler in use. The throttler itself may be made available via an additional attribute/property

Describe alternatives you've considered

  • Instead of an interface class, one could directly provide the SimpleThrottler and encourage users to override process_update
  • One could make SimpleThrottler a bit more elaborate by e.g. allowing to sequencify the processing of updates per chat id. In this case, I would vote to go for the interface-class approach as the implementation would surely require more elaborate internals than that of SimpleThrottler.

Additional context

This idea was already discussed a bit in the early stages of the v20 architecture design but was put on hold, since we first wanted to get the basic structure ready before introducing additional functionality.

Before an attempt on an implementation for this is made, I'd like to get feedback on the idea :)

@Bibo-Joshi
Copy link
Member Author

Some more notes:

  • Harshil suggested a way to inspect the number of updates that are currently being processed. This could be achieved via an abstract property
  • It might be good to also add (abstract) methods initialize and shutdown such that resources can be cleans allocated and freed
  • If we do that, we can make the class also available as context Manager like we do for Bot, request and application
  • In any case, it should not be the new classes responsibility to wait for all updates being processed when the application stops running. That's already taken care of in the Application currently and I see no reason to burden a user implementation with that

@Bibo-Joshi
Copy link
Member Author

Bibo-Joshi commented Feb 2, 2023

  • More naming suggestions by Harshil: {Base, Simple}Processor (in accordance with process_update. (+1 from me)
  • Hashil also prefers an abstract base class
  • The architecture diagram should be updated after this issue is closed. One could think about using the chance to rebuilt it as mermaid diagram, which has the upside that GitHub can render those natively

@karfly
Copy link

karfly commented Mar 6, 2023

Hi, @Bibo-Joshi @clot27
I'm from #3575 issue. Do you have any progress in this feature?

@Bibo-Joshi
Copy link
Member Author

The issue is in the backlog and clot will send a PR once they have the time for it. If you like to, you can send a PR yourself. Please leave a short comment in that case so that we can avoid doubled work 🙂

@Bibo-Joshi
Copy link
Member Author

Reopening just as a reminder that the wiki pages on orvall architectire and concurrency still need to be updated :)

@Bibo-Joshi Bibo-Joshi reopened this Jun 2, 2023
@Bibo-Joshi Bibo-Joshi added this to the v20.4 milestone Jun 2, 2023
@harshil21 harshil21 added the ℹ️ needs-wiki-update information: needs-wiki-update label Jun 20, 2023
@github-actions github-actions bot locked and limited conversation to collaborators Jul 14, 2023
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
ℹ️ needs-wiki-update information: needs-wiki-update
Projects
None yet
Development

Successfully merging a pull request may close this issue.

4 participants