Skip to content
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

Refactor MessageQueue #2139

Open
Bibo-Joshi opened this issue Oct 12, 2020 · 4 comments
Open

Refactor MessageQueue #2139

Bibo-Joshi opened this issue Oct 12, 2020 · 4 comments

Comments

@Bibo-Joshi
Copy link
Member

@Bibo-Joshi Bibo-Joshi commented Oct 12, 2020

TL;DR:

Known bugs are #2015, #2012, #2035 and #2392

Long version:

Using MessageQueue currently requires quite some manual work and there also some bugs (see #2015, #2012, #2035, #2392). Time to live up to the wikis statement

We plan to tightly couple it with telegram.Bot so that it could be used more conveniently (and even implicitly unless other specified).

So here is what my idea for a refactoring is. I'd be happy for feedback before getting started :)

Refactoring of MessageQueue and DelayQueue

  • DQ.exc_route: I'd like to rename to error_handler in accordance with Dispatcher.add_error_handler.
  • If a Dispatcher is used, it would make sense to have the execution of the promises to behave like the refactored Dipsatcher._pooled, which also includes proper error handling. That can be easily achieved by passing the Dispatcher to either the Bot or the MessageQueue. Not sure yet, what makes more sense …
  • I think I'd like to repace the __call__ methods with process or whatever. That's more explicit and easier to read in the code.
  • Add MQ.add/remove_delay_queue so that users can add custom delay queues in addition to the default & the group one. One should be able to specify a parent (defaulting to the default DQ) for new_dq, such that everything going throguh new_dq will also go through the parent. MQ.add_remove_delay_queue sohuld also start/stop the DQ in case we're adding them at runtime. Edit: This will in fact close #1369 , as I just notice
  • Add constants for the default & the group DQ
  • Handle the stopping of MQ correctly, some hints are already given in #2012

Integration into Bot

Add a MessageQueue to Bot by default and rebuild all methods in the fashion

def badly_sketched_messagequeue_wrapper(func, *args, **kwargs):
    delay_queue = kwargs.pop('delay_queue')
    if delay_queue == self.message_queue.DEFAULT_DQ and int(kwargs.get('chat_id')) < 0: 
        return self.message_queue.process(func, self.message_queue.GROUP_DQ)
    elif delay_queue:
        return self.message_queue.process(func, delay_queue)
    return func(*args, **kwargs)

@badly_sketched_messagequeue_wrapper
def send_message(…, delay_queue=None):
    ...

such that delay_queue allows to select the delay queue the method is passed through, replacing the current queue and isgroup kwargs

  • Together with the changes to Bot.__new__ as in discussed #2015 , this should resolve #2015
  • for cases where one only needs Bot, without Dispatcher or Updater, one could do something like
def badly_sketched_messagequeue_wrapper(func, *args, **kwargs):
    if self.message_queue.is_running:
        # see above
    return func(*args, **kwargs)

so users can easily opt-into MessageQueue with bot.mq.start/stop()

Integration with Defaults

Add Defaults.delay_queue defaulting to None (I'm not in favor of implicitly always using the MessageQueue)

Handling context managers

I think the approach detailed in #2035 is feasible (build InputFiles already in Promise.__init__). I can't think of any use case, where a context manager would be used for anything else that we'd to handle …

Edit: Now I can. See #2160

Backwards compatibility

All of above changes should be doable without breaking changes.
Hower, given that MessageQueue is essentially still in the same quite rough state as when introduced in #537 3.5 years ago, IMHO it would be okay to removethe current usage pattern (i.e. the @queuedmessage decorator) in the next major release (which probably will be mostly a "delete all the stuff" release anyway …)

@Bibo-Joshi
Copy link
Member Author

@Bibo-Joshi Bibo-Joshi commented Jan 2, 2021

After some internal discussion, We'll postpone this to v14 (or later minor releases) when we switch to asyncio. This will allow us to make the bot methods use something like return Message.de_json(await self._post(…)) instead of returning a future/promise. There is also the idea of moving the delay logic to utils.Request which would allow for a higher level of customization. Still many ideas above still apply.

PS: I thinks it's important to allow users to customize the delay logic both on a global level (see #1369) and on a per-call level (see #2265)

@jagub2
Copy link

@jagub2 jagub2 commented Feb 23, 2021

I updated this Python module and got deprecation warning. Sure, I know I should fix this on my own. However, looks like examples on wiki are out of date: https://github.com/python-telegram-bot/python-telegram-bot/wiki/Avoiding-flood-limits#using-mq-with-queuedmessage-decorator.

@Bibo-Joshi
Copy link
Member Author

@Bibo-Joshi Bibo-Joshi commented Feb 23, 2021

The wiki also comes with two big notes about the deprecation ;)

Also you are not expected to fix anything an your own - the deprecation notes simply state that this very issue here lists known bugs (see the very first line) and you should be aware of them if you use MessageQueue ;)

@jagub2
Copy link

@jagub2 jagub2 commented Feb 23, 2021

Whoops, I went "tl;dr" too much. I just checked if the example changed. Got it, thank you.

@Bibo-Joshi Bibo-Joshi added this to Stage 3: Things building upon asyncio changes in v14 May 7, 2021
@Bibo-Joshi Bibo-Joshi mentioned this issue May 11, 2021
22 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
v14
Stage 3: Stuff here depends on how we...
Linked pull requests

Successfully merging a pull request may close this issue.

2 participants