[Discussion:] ConversationHandler

[Bibo-Joshi]

Several ideas for changes and/or additions have been gathered in the v14 project, mentioned in the user group or issues or dwelled in the back of my head. So I'm writing this issue to keep track of them better.

The following list of ideas should be discussed before release of v14 because at least some of them are easier to implement if we do them non-backwards compatible. I'd like to emphasize that most of these are very rough first thoughts and I'm fully aware that we may scrape many of them.

Related: #2802

---

• Make CH.fallbacks optional. I've seen a lot of people (including me) setting at to an empty list for some use cases … Poolitzer already expressed support for this idea
• ConverstanionHandler + run_async: Problem is that we can't persist futures/promises. But we should be able to guarantee that all futures are done on (clean) shutdown. One could add something like BasePersistence.resolve_conversation_promises that persistence classes can call before flushing on shutdown. This already get's handled on v20
• [FEATURE] ConversationHandler: Allow special handlers/callbacks for entering states #2390 (closed just for better overview, not actually implemented)
• add a "prefallbacks" list, i.e. the state-handlers only trigger, if the "prefallback" handlers don't trigger. The current alternative is to add those handlers to the fallbacks list, which requires to make all state handlers ignore updates that should be handled by the fallbacks. see also Added impementation of "prefallbacks" list. #2764 #2764; Implemented "prefallbacks" feature #2960
• Include entry_points, fallbacks (and eventual "prefallbacks") into the states dict. We already have special states for TIMEOUT and WAITING, so I think it would make the interface cleaner if we used special states (PRE)FALLBACK and END as keys for these lists rather than making them standalone kwargs of __init__
• Think about "conversation related data", e.g. context.conversation_data. It should be an object unique per conversation key and ConversationHandler instance. It may even be automatically deleted when the conversation ends . See also [FEATURE] Have per-conversation data (or at least an identifier) in ConversationHandler #4136
• Rethink which parts of ConversationHandler we consider public.
  • e.g. the conversations is currently public, although undocumented and basically part of the internals. If we make it private, we can rework the setters of persistence and conversations to something like get_persisted_data(persistence) or initialize_persistence(persistence)
  • We also rather regularly get questions like "how do I access the current state of the conversation" and "how do I check if a user has an active conversation". Making the logic of how the keys are build and how conversations works public may help with some of these and also with the above context.conversation_data
  • One could take this one step further and let the user customize how the key is generated. This could be useful e.g. in situations where one wants to handle 3rd-party updates (e.g. login into an external service) within the conversation, which are not of type telegram.Update
• think about conversation_timeout:
  • Is it worth to allow timeout handlers to put the conversation in a non-end state?
  • it it worth to try & allow a different timeout duration per state?
  • Can we make them work for nested conversations?
  • Can we ensure that the timeout jobs can be persisted?
• [FEATURE] class-based nested conversations #2827 to make CH be based more on a class-like structure
• nested conversations:
  • Check behavior of the WATING state, especially with nested conversations
  • Check behavior of async handlers with nested conversations
  • Once a child conversation has started one may want the next update to be processed exclusively by that child conversation, even if it's not the first handler in the state. Maybe this can be made possible. See also https://stackoverflow.com/questions/78143788
• Think about a mechanism to lazy-load persisted conversation states to allow to reduce load on startup. would be especially useful for stateless server designs. See here for some initial discussion
• When the conversation has ended, pass CH.END to persistence.update_conversation instead of None
• We currently issue a warning for per_message=True, per_chat=False - this is not necessarily valid in case inline_message_ids are used
• entry_points should be checked after the states are checked - that way, state handlers may trigger before any entry_points trigger via allow_reentry. See https://t.me/pythontelegrambotgroup/606782?thread=606781 for a question on that. Edit: On second thought, the allow_reentry may not be needed at all - the user can just repeat the entry_points in the fallbacks. If "prefallbacks" (see above) are added, that even allows the user to customize which of state handlers and entry_points are checked first
• A commonly requested feature is to track a "conversation" between two (or more) users who each chat with the bot in the respective private chat. If we could make something like this feasible, that would be awesome.
• discuss possibility to make job timeouts persistent: [FEATURE] Persist timeouts of ConversationHandler #3522
• Improve type hinting where possible - see Sub-Optiomal type hints in ConversationHandler #3665 (closed only to track this here)
• Errors in the timeout state should be handled like errors in handlers, not like errors in jobs - see https://t.me/pythontelegrambotgroup/675226
• In case built-in states are used, they should either not be easy to reproduce or we should warn if the user tries to use them manually. See [FEATURE] warn if ConversationHandler.states has a key -1 #3834
• Rethink if we want to allow for some error handling on CH or even state level. See From error state to other state on a Conversational bot #2277 for some discussion

[kirsanium]

I request for a tree-like behaviour on reentering nested ConversationHandlers - it seems reasonable to ConversationHandler.END all child handlers when you reenter the root one. Found no intended way to do it and my current solution is extremely hacky.

[Poolitzer]

The grammy dev recently contacted me and pointed towards their implementation of conversation handlers. Since they are pure async as well, this might be interesting to consider. May be too much for either V20 or PTB in general and rather ptbcontrib, just wanted to write it down.

https://t.me/grammyjs_news/36

[Bibo-Joshi]

Interesting! Thanks for mentioning. A similar idea has come up in #3042 before. While my comments on that PR still hold, one can ofc still keep the concept in mind. Redesigning CH as a whole can probably come to many different results.

[Bibo-Joshi]

The grammy dev recently contacted me and pointed towards their implementation of conversation handlers. Since they are pure async as well, this might be interesting to consider. May be too much for either V20 or PTB in general and rather ptbcontrib, just wanted to write it down.

https://t.me/grammyjs_news/36

I've read through the grammy docs a bit and have a few comments on what I learned:

Most important IMO is the observation, that the async conversation-function is not just simply executed. Instead on every await conversation.wait(), the function is basically ended and on the next incoming update the function is re-run up to that line. This is e.g. important for persistency. This mechanism makes it necessary to

• wrap calls to external apis and random functions in special functions in order to not call those again on the re-runs and use conversation-specific functions to deal with non-obvious side-effects
• store all updates and other variables somewhere in order to use them in the re-runs
• apparently use complicated code setups to make the exit&rerun setup work
• re-run also some other things that one wouldn't necessarily expect to re-run

TL;DR: While this approach allows you to write easy-to-read code in a straightforward way at first glance, it's not quite so easy on second glance and the implementation is everything but easy.

A second thing that I want to point out is the usage of conversation.wait/waitFor(…) and the usage of filter quries in waitFor. As far as I see by quickly skimming through the grammy docs, these filter queries are grammys analogue of BaseHandler.check_update (while in PTB filters are used exclusively for Message-handlers). With this background, wait(For) is a vary natural way to specify what update to wait for in the conversation. In PTB, the background is different and a similar approach would probably mean to eather abandon the BaseHandler setup or duplicate it into a filter-like setup.

---

Grammys approach definitely has several perks, surely works well in grammys ecosystem and I have huge respect for whoever built it. But my summary is that grammys approach is not systematically superior to a classic FSM.

[KnorpelSenf]

Glad to see that these ideas are spreading! It took a fair bit of research and many iterations and brainstorming to come up with the concept. And yes, it was a bit challenging to build, too :)

In case you're going to consider something similar for PTB, note that you have control over the event loop in Python (in contrast to JS), which greatly simplifies the way how conversation builder functions are run. You can simply stop running the loop, while grammY conversations sort of have to implicitly intercept its execution via a special promise and then hack ourselves back into control.

On the question why I personally dislike FSMs and similar abstractions, there's a good high-level summary of the ambition behind conversations in this interview starting at 26:34 min: https://podrocket.logrocket.com/grammy

Looking forward to seeing what you'll come up with! Good luck!

[Bibo-Joshi]

@KnorpelSenf thanks for your comment! I did a bit more reading and thinking on the topic, but haven't made any significant progress so far :D TBH I also don't quite understand your comment on controlling (and stopping) the event loop in Python. If I stop the event loop, then how will I process new updates?
If you're interested you can also contact me on Telegram. I'd be happy to have a more in-depth discussion in private chat :)

[Bibo-Joshi]

A few insights into the grammyjs conversation setup/how something like this could look like in python from offline discussion with KnorpelSenf:

• If you use var = await conversation.external(some_function_that_must_not_be_replayed(…)), the return value of some_function_that_must_not_be_replayed must be persisted for the replay-mechanism to work properly. This can be an issue with general objects, e.g. sychronization primitives like locks are generally not serializable.
• Object-identity can become a problem: If a variable leaves the scope of the conversation and the conversation is replayed, the variables inside the conversation and outside the conversation are no longer the same object. MWE in grammyjs, problem sketch in pseudo-python-code. This problem can probably be alliviated if the data is kept in memory as long as the bot is running, though there may be some edge cases where it stil appears
• In Python, the necessity to call await conversation.external(some_coroutine_function(…)) can be removed by implementing a custom event loop. I.e. await some_coroutine_function(…) automatically takes care of storing the return value and the replay-logic. See here for a reference. However, this still only works for coroutine functions and for other functions that must only be run once (e.g. print(…), random), one would still need something like conversation.external

[rejedai]

I apologize for the possible duplication, I did not find this in the branch.

I think it is necessary to add a simple way to get the conversation by name and get the current state from the conversation.
Example:

main_conversation = context.application.handlers.get_by_name("main_conversation")
state = main_conversation.get_state(CHAT_ID, USER_ID)

Maybe I have bad notification design, but I don't need to send a notification when a person has passed a certain state. I used to bypass this with the help of a persist, I took data from the database, now when the persist is not realtime, I have to bypass it with the help of cyclic checking of handlers, which I think is not correct.

my current user state code:

async def get_state(context: CallbackContext, telegram_id: int):
    conversations = None
    user_state = None

    for handlers in context.application.handlers.values():
        for handler in handlers:
            if isinstance(handler, ConversationHandler) and handler.name:
                # noinspection PyProtectedMember
                conversations = handler._conversations

    if conversations is not None:
        for key, state in conversations.items():
            if telegram_id in key:
                user_state = state
    return user_state

[Bibo-Joshi]

I had more thoughts on the grammyjs conversation setup. Specifically, the following points from the description above would be hard to realize that way IMO or at least harder than with an FSM approach like we currently have:

• fallbacks and pre-fallsbacks - these would require a bunch of if-elses inside the conversation
• getting info of where in the conversation the user currently is - this would require to somehow set that info within the conversation, which would be weird IMO as it has nothing to do with the actual conversation implementation
• moving the users conversation to a different step - this would probably require to inject custom ForwardConversation updates, injecting them and handling them on every conversation.wait(…)

OTOH, the following things might be easier with the grammyjs setup:

• timeout handling - you could just use asyncio.wait_for. Although that's surely hard to get working correctly with persistence
• a WATING state - you could use standard asyncio.Task methods, but would probably have to take care of cancelling the conversatio.wait task if there is no new update coming in while the update is still being processed
• handling conversations between multiple users/groups - you could specify the expected chat_id in conversation.waitFor(…)

[KnorpelSenf]

Leaving my 2c here since I'm still subscribed to the thread:

• fallbacks and pre-fallsbacks - these would require a bunch of if-elses inside the conversation

You don't need if-else, to repeat code, you need loops. The convenience methods as well as conversation forms abstract those away most of the time.

• getting info of where in the conversation the user currently is - this would require to somehow set that info within the conversation, which would be weird IMO as it has nothing to do with the actual conversation implementation

This info is always available implicitly. Whatever you want to do at step X you can already do by writing the code at that point in the conversation.

• moving the users conversation to a different step - this would probably require to inject custom ForwardConversation updates, injecting them and handling them on every conversation.wait(…)

Those patterns are discouraged, they are like the complaint "python is bad because it does not have goto". Code will be better without jumping around.

• timeout handling - you could just use asyncio.wait_for. Although that's surely hard to get working correctly with persistence

Already implemented in grammY, but conversations are silently left once expired, which may not always be the best solution for people.

• a WATING state - you could use standard asyncio.Task methods, but would probably have to take care of cancelling the conversatio.wait task if there is no new update coming in while the update is still being processed

Concurrent update processing per chat is rarely a good idea, not sure what this is about tbh

• handling conversations between multiple users/groups - you could specify the expected chat_id in conversation.waitFor(…)

You must first synchronize updates received from different chats, as this is a race condition. Good luck with this in distributed environments, this would come with heavy perf penalties. But yes, the abstraction itself would permit this easily.

[lemontree210]

I think it is necessary to add a simple way to get the conversation by name and get the current state from the conversation.

I would also add to this the action that led to the current state. If I want to return the user to a certain state (and a certain callback), I most likely need to ask them the same question that they were asked before they got to that state. Right now I have to repeat the question manually before returning the desired state and sending the user back to the relevant part of conversation.

This can be too tricky, though, as, depending on the state of conversation, we may need to edit an inline query or send a new message or do something else. So maybe it's not realistic anyway.

[MikiEremiki]

I would also like to suggest the possibility of adapting the library for consideration. https://github.com/Tishka17/aiogram_dialog
Of course, this is a fundamentally different solution, but many users would say thank you for such a library.
I tried to analyze how to adapt it to PTB, but my knowledge was not enough.

[Poolitzer]

Recently the idea came up in chat to allow custom classes in CH, because the subclass restriction of Update potentially forces one to input a lot of dummy values in the class.

So if we ask them to e.g. subclass ConversationHandlerRequirement which only has user and/or chat id as attribute, we can decrease that dummy data amount

[Bibo-Joshi]

@Poolitzer see also the 3rd bullet point in

Rethink which parts of ConversationHandler we consider public.