Initial FSM PoC by Bibo-Joshi · Pull Request #4669 · python-telegram-bot/python-telegram-bot · GitHub

Works on #2770

"It has been a while since we openend #2770" would be an understatement. Yet here we are :D
Finally, I got around to draft up a somewhat proper PoC for my ideas outlined in #2770 (comment). I also attached an example that showcases how the FSM setup can be used. You'll need ≥2 accounts to test it.

To be clear: as of 2025-02-04 this PR does by far not include everything mentioned in #2770 (comment). I hope to work on it in the nearish-future. Continued work on this PR could be greatly supported by intermediate reviews, comments & discussions :)

ToDos / Ideas / Thoughts

(Listing here only things that came to my mind explicitly while working on this PR, not copying everything from #2770)

Using context.job_queue.run_once for conversation timeouts is relatively easy, but would have to repeated everywhere which also holds for the cancellation. Maybe we can think of a clever way to abstract that for the user.
Having both groups and states makes the "who handles what" logic more involved. Do we do "for state in states: for group in state" or "for group in groups: for state in group"? Also State.ANY and things like StateA | StateB make it necessary that one update can be handled by multiple states. All this at least needs proper documentation about what's going on. Additionally, we should think carefully about what we want here.
I'm unsure about if get_active_key_state should be async or not:
- I feel like it should not make too much async operations - since it's part of the logic in App.process_update, it should be as quick as possible
- If we want to support lazy-loading, this would probably be the place were it comes into play - depending on how we design the persistence connection
- If states are set within get_active_key_state (as is done in the fsm example I added), then I'm wondering if one should use a semaphore for that. If one would do that, this could lead to major delays in process_update
Instead of BoundedSemaphore(1), we could probably just use Lock as synchronization primitive

…e update

…llow

Just some first impressions, once I gain a further understanding I can give more

not sure I like this being private/protected/however the underscore is called

So far I see no reason why someone should be allowed to all USM(…).get_admin_state … then again this is only an example, so I don't care :D

That should not be the private function right

The type hint here is linked to the FSM class type hint above right? I can't have one be a string one be an integer.

Correct, the type variable specifying the type of keys you use.

Does this remove the group if its empty? Can we comment it

yes, that's basically the same as state_handlers.pop(group, None), I think. It will call state_handlers.__del__(group). IMHO this is basic python syntax 😬

Im back with my thinking about private stuff. Why do we private it here, not in other files; should we. Hm.

I feel you :D if machine was a public module like telegram.constants, it would have to be protected IMO. Since it's not and we expose the relevant elements only via telegram.ext, it's not so important. Making it clearer what the content exposed by this module is still doesn't hurt.
If you want to go deeper into the rabbit hole: https://discuss.python.org/t/add-the-export-keyword-to-python/28444/6

these should be part of the builder and the second one set by default imo

yes, absolutely. #PoC 👼

Why weakref wont it/the user experience break if the supplied job queue gets destroyed

There is job_queue.application. that's also weakref b/c we have a cyclic reference: application is application.job_queue.application. Now we also have application is application.fsm.job_queue.application. I'm not 100% sure if this is strictly necessary, but it seemed safer to me 🤔 I can at very least add a comment.

ha here see. why.

is the error saying UID correct? I would understand the optional type hint to be anything hashable

Sorry, I don't get you. What do you think is wrong? the phrasing of the error message?

Wait I must really misunderstand smth, how does this work across states

uh … that's how class variables work? :D In the init self.__knows_uids is basically the same as State.__knows_uids …

A thought I had: Since this is such a huge change for something so integral to our project, should we provide a beta version/implementation after we are satisfied with the status and announce it to get people to try it out and provide feedback before we make a first release?

A thought I had: Since this is such a huge change for something so integral to our project, should we provide a beta version/implementation after we are satisfied with the status and announce it to get people to try it out and provide feedback before we make a first release?

That is indeed a very good idea 👍
My thoughts on this are roughly like this:

the overall change should (hopefully) be such that nothing changes if you just don't use the new states.
that way we can introduce the refactoring & deprecate the existing CH without braking changes.
We can mark the status as "public beta feature" and at some point we can say "well, starting with version 25.0, states are now marked as stable".

that way we wouldn't have to do any additional actual beta releases.
If we prefer to have a proper beta release, I'm open to that. We could e.g. do states update on a 23.0bx line that gets only updates for states while other stuff (especially API updates only go into 22.x) . We shouldn't do that for too long IMO, we saw how much effort that takes with v20.
Encouraging people to try out a branch is ofc an easy option.

I dont get the first point, what do you mean with not using new states. In conjunction with ConvHandler? I thought this is supposed to fully replace it. In that case I would like a big announcement at least (maybe also in readme or so).

We might not really need an extra release now that I think of it, just make a big fuss about it before calling it stable and doing the deprecation error warning.

I dont get the first point, what do you mean with not using new states. In conjunction with ConvHandler? I thought this is supposed to fully replace it. In that case I would like a big announcement at least (maybe also in readme or so).

Yes, this is supposed to replace CH. But, since this is acting on a different level than handlers, the conversationhandlerbot.py example will still run as before. We should throw hella-big warnings if states & CH are used in conjunction, true.
All other features that are independent of CH will also all run the same. all keys will always be in state "IDLE".

We might not really need an extra release now that I think of it, just make a big fuss about it before calling it stable and doing the deprecation error warning.

👍

Ah yes you mean in the transition period alright. Okay so lets do a release when we are happy and announce that its there and see if we get feedback, and then do a deprecation later. Perfect.

I'm not sure if this is the right place to mention this but:

Is there any plans or ongoing work for a declarative but an extensible (e.g. with hooks) FSM syntax? like State1 ---(on some input or trigger) ---> State2 (produce or calculate some output or perform a set of actions) ?

I have a fairly complex stateful bot in PTB (and ConversationHandler) and have written a very rudimentary function that parses a generically defined (but limited) transition table (declarative) and performs the transitions given the inputs and also produces some (a set of) outputs or actions.

I did it for a subset of states (relatively simple logic) in my application, because in the current PTB framework adding states and extending the FSM logic is somewhat a pain. Code re-usability was another consideration (e.g. several states might want to transition to a very specific state and so using a common transition function or hook for setting up the UI and then entering that state is useful for code reuse).

A declarative approach to complex FSM logic specification would both accelerate and simplify development, IMHO.

Initial FSM PoC

0c06ba0

Bibo-Joshi added 🛠 refactor change type: refactor 🔌 enhancement pr description: enhancement labels Feb 4, 2025

Bibo-Joshi requested review from Poolitzer, clot27 and harshil21 February 4, 2025 20:44

This comment was marked as outdated.

Sign in to view

Bibo-Joshi added 8 commits February 5, 2025 10:52

Add State.ANY for fallbacks and allow handling multiple states for on…

07225b9

…e update

Temporarily Disable Tests on this branch

34832d9

Add Some Abstractions for Timeout Jobs

434cbfa

Disable tests harder …

817b71d

Move internal state storage to FiniteStateMachine and add state history

646ba37

Try setting up infrastructure for optimistic locking. Example will fo…

4441543

…llow

Add an example on concurrency in FSM

31af1a9

Use Lock instead of semaphore

b1fff6d

Poolitzer reviewed Feb 8, 2025

View reviewed changes

fred231084g approved these changes Dec 22, 2025

View reviewed changes

+                  def __init__(self, uid: Optional[str] = None):
+                      effective_uid = uid or uuid4().hex
+                      if effective_uid in self.__knows_uids:
+                          raise ValueError(f"Duplicate UID: {effective_uid} already registered")

+                      self.admin_id = admin_id
+                      super().__init__()
+                  def _get_admin_state(self) -> tuple[State, int]:

+                  def _get_admin_state(self) -> tuple[State, int]:
+                      return self._states[self.admin_id]
+                  def get_state_info(self, update: object) -> StateInfo[Optional[int]]:


		from telegram.ext import JobQueue

		_KT = TypeVar("_KT", bound=Hashable)

		application.fsm = UserSupportMachine(admin_id=123456)
		application.fsm.set_job_queue(application.job_queue)

+                      return f"FSM_Job_{'_'.join(str(hash(k)) for k in keys)}"
+                  def set_job_queue(self, job_queue: "JobQueue") -> None:
+                      self.__job_queue = weakref.ref(job_queue)

		if raise_exception:
		raise RuntimeError("JobQueue was garbage collected")



		class State(abc.ABC):
		__knows_uids: ClassVar[set[str]] = set()

		if not state_handlers[group]:
		del state_handlers[group]

Initial FSM PoC by Bibo-Joshi · Pull Request #4669 · python-telegram-bot/python-telegram-bot · GitHub

Uh oh!

Initial FSM PoC - #4669

Initial FSM PoC#4669Bibo-Joshi wants to merge 9 commits intomasterpython-telegram-bot/python-telegram-bot:masterfrom feature/fsmpython-telegram-bot/python-telegram-bot:feature/fsmCopy head branch name to clipboard

Conversation

Bibo-Joshi commented Feb 4, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

ToDos / Ideas / Thoughts

Uh oh!

This comment was marked as outdated.

Poolitzer left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Poolitzer commented Feb 8, 2025

Uh oh!

Bibo-Joshi commented Feb 8, 2025

Uh oh!

Poolitzer commented Feb 8, 2025

Uh oh!

Bibo-Joshi commented Feb 8, 2025

Uh oh!

Poolitzer commented Feb 8, 2025

Uh oh!

OnceUponATimeInAmerica commented Jul 11, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

Initial FSM PoC#4669
Bibo-Joshi wants to merge 9 commits into
masterfrom
feature/fsm

Bibo-Joshi commented Feb 4, 2025 •
edited
Loading

OnceUponATimeInAmerica commented Jul 11, 2025 •
edited
Loading