[Discussion:] ConversationHandler #2770
Comments
Bibo-Joshi
added this to Stage 4: Not really dependent on asyncion but probably better done after fixing the rest
in v20
|
I request for a tree-like behaviour on reentering nested |
|
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. |
|
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. |
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
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 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. |
|
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! |
|
@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? |
|
A few insights into the grammyjs conversation setup/how something like this could look like in python from offline discussion with KnorpelSenf:
|
|
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. 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 |
|
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:
OTOH, the following things might be easier with the grammyjs setup:
|
|
Leaving my 2c here since I'm still subscribed to the thread:
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.
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.
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.
Already implemented in grammY, but conversations are silently left once expired, which may not always be the best solution for people.
Concurrent update processing per chat is rarely a good idea, not sure what this is about tbh
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. |
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. |
|
I would also like to suggest the possibility of adapting the library for consideration. https://github.com/Tishka17/aiogram_dialog |
|
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 |
|
@Poolitzer see also the 3rd bullet point in
|
|
I've been brooding on an idea for a few weeks now and I don't think that I'll be able to publish a AbstractWe introce an absract base class MotivationIntroducing stateful logic effectively makes the complete Implementation draftHere is some pseudo-code to give you an idea of how this could look like in terms of interfaces. New FSM specific classesclass FSMKey(ABC):
"""Alternatively, we could simply use :class:`collections.Hashable`
"""
@abstractmethod
def __hash__(self) -> int:
...
@abstractmethod
@classmethod
def from_update(cls, update: object) -> FSMKey:
"""Returns the FSM key for the update.
If more than one key can be build for the update, the implementation may either chose one or raise an exception.
If the update is not supported, the implementation may return NotImplemented in which case only handlers for the state FSMState.IDLE will be used.
"""
# Not sure if we need such a method
class ExampleFSMKey(FSMKey):
chat_id: int | None
user_id: int | None
def __hash__(self) -> int:
return hash((self.chat_id, self.user_id))
@classmethod
def from_update(cls, update: object) -> FSMKey:
if not isinstance(update, Update):
return NotImplemented
return cls(chat_id=update.effective_chat.id, user_id=update.effective_user.id)
class FSMState(ABC):
"""
Represents a state, independent of the key.
Implements bitwise and, or, not, and xor to allow logical combination of states without having to define too many state objects.
Needs to ensure uniqueness of the uid via a class singleton.
UID probably not strictly necessary for FSM implementation itself, but should be useful for logging, for easy identification of bitwise combined states and also for custom persistence implementations.
"""
def __init__(self, uid: str) -> None:
self.__uid: str = uid
@property
def uid(self) -> str:
"""UID including bitwise combinations"""
def matches(self, fsm_state: FSMState) -> bool:
"""Checks if the passed state is covered by this state.
For non-combined states, this just returnes ``self is fsm_state``.
"""
# `|` and `^` check if one of the components matches
# `~` checks if the state does *not* match
# `&` acts like a non-combined state - this is mainly useful to define substates like ICE = WATER & FROZEN without having to manually defining a new object for ICE
class FSMManager(ABC):
# Probably a Generic class dependent on the FSMKey type
@abstractmethod
async def get_active_state(self, update: object) -> FSMState:
"""Returns exactly one active state for the update.
If more than one stored key applies to the update, one must chosen.
It's recommended to select the most specific one.
Example:
The state of a chat, a user or a user in a specific chat could be tracked.
For a message in that chat, the state of the user in that chat should be returned if available.
Otherwise, the state of the chat should be returned.
"""
# Example logic for ExampleFSMKey:
if (
global_state := self.states.get(ExampleFSMKey(None, None), FSMState.IDLE)) != FSMState.IDLE:
return global_state
if (chat_state := self.states.get(ExampleFSMKey(update.effective_chat.id, None),
FSMState.IDLE)) != FSMState.IDLE:
return chat_state
if (user_state := self.states.get(ExampleFSMKey(None, update.effective_user.id),
FSMState.IDLE)) != FSMState.IDLE:
return chat_state
return self.states.get(ExampleFSMKey(update.effective_chat.id, update.effective_user.id),
FSMState.IDLE))
@astractmethod
async
def do_set_state(self, key: FSMKey, state: FSMState) -> None:
"""Store the state for the specified key.
If the state is ``FSMState.IDLE``, it's recommended to drop the key from the in-memory data.
Warning:
This method should not be directly accessed by user code.
It is intended for internal use in this class only.
Instead, use :meth:`set_state`
"""
async def set_state(self, key: FSMKey, state: FSMState) -> None:
"""Store the state for the specified key."""
async with self.get_semaphore(key=key):
self.do_set_state(key=key, state=state)
def get_semaphore(self, key: FSMKey) -> asyncio.Semphore:
"""Returns a semaphore that is unique for this key at runtime.
It can be used to prevent concurrent access to resources associated to this key.
"""
# uses weakref.WeakValueDict internallyAdaptions of exitisting interfacesclass CallbackContext
"""Extend existing interface with convenience shortcuts."""
# how to set these easily?
self.fsm_key = fsm_key
self.fsm_state = fsm_state
@property
def fsm_semaphore(self) -> asyncio.Semaphore:
return self.application.fsm_manager.get_semaphore(self.fsm_key)
async def set_fsm_state(self, state: FSMState, key: FSMKey = self.fsm_key) -> None:
self.application.fsm_manager.set_state(key=key, state=FSMState)
class Application:
def add_handler(self, handler: BaseHandler, fsm_state: FSMState = FSMState.IDLE) -> None:
"""By specifying a state, it will be availble only if the update corresponnds to that state.
"""
# Alternatively, make `fsm_state` an argument of `BaseHandler` (similar to `block`)
async def process_update(self, update: object) -> None:
"""Updated to consider FSM states."""
current_fsm_state = self.fsm_manager.get_active_state(update)
for handlers in self.handlers.values():
for handler, fsm_state in self.handlers.items():
if not fsm_state.matches(current_fsm_state):
continue
# Note: for each handler group, the handlers can call `context.set_fsm_state`, overriding the
# previous setting. `current_fsm_state == context.fsm_state` will still stay the same for the
# update in all groups. May need discussion.
await handler.handle_update(update, context)Adressing known questionsHow can existing functionality be covered?
How does this solve existing problems and feature requests?
What new problems does this approach bring?
|
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
CH.fallbacksoptional. 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 ideaConverstanionHandler + 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 likeThis already get's handled on v20BasePersistence.resolve_conversation_promisesthat persistence classes can call before flushing on shutdown.fallbackslist, which requires to make all state handlers ignore updates that should be handled by thefallbacks. see also Added impementation of "prefallbacks" list. #2764 #2764; Implemented "prefallbacks" feature #2960entry_points,fallbacks(and eventual "prefallbacks") into thestatesdict. We already have special states forTIMEOUTandWAITING, so I think it would make the interface cleaner if we used special states(PRE)FALLBACKandENDas keys for these lists rather than making them standalone kwargs of__init__context.conversation_data. It should be an object unique per conversation key andConversationHandlerinstance. It may even be automatically deleted when the conversation ends . See also [FEATURE] Have per-conversation data (or at least an identifier) in ConversationHandler #4136ConversationHandlerwe consider public.conversationsis currently public, although undocumented and basically part of the internals. If we make it private, we can rework the setters ofpersistenceandconversationsto something likeget_persisted_data(persistence)orinitialize_persistence(persistence)conversationsworks public may help with some of these and also with the abovecontext.conversation_datatelegram.Updateconversation_timeout:WATINGstate, especially with nested conversationsCH.ENDtopersistence.update_conversationinstead ofNoneper_message=True, per_chat=False- this is not necessarily valid in caseinline_message_idsare usedentry_pointsshould be checked after the states are checked - that way, state handlers may trigger before anyentry_pointstrigger viaallow_reentry. See https://t.me/pythontelegrambotgroup/606782?thread=606781 for a question on that. Edit: On second thought, theallow_reentrymay not be needed at all - the user can just repeat theentry_pointsin thefallbacks. If "prefallbacks" (see above) are added, that even allows the user to customize which of state handlers andentry_pointsare checked firstConversationHandler#3665 (closed only to track this here)ConversationHandler.stateshas a key -1 #3834The text was updated successfully, but these errors were encountered: