- Notifications
You must be signed in to change notification settings - Fork5.7k
Initial FSM PoC#4669
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 ourterms of service andprivacy statement. We’ll occasionally send you account related emails.
Already on GitHub?Sign in to your account
base:master
Are you sure you want to change the base?
Initial FSM PoC#4669
Uh oh!
There was an error while loading.Please reload this page.
Conversation
This comment was marked as outdated.
This comment was marked as outdated.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
Just some first impressions, once I gain a further understanding I can give more
| self.admin_id = admin_id | ||
| super().__init__() | ||
| def _get_admin_state(self) -> tuple[State, int]: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
not sure I like this being private/protected/however the underscore is called
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
So far I see no reason why someone should be allowed to allUSM(…).get_admin_state … then again this is only an example, so I don't care :D
| super().__init__() | ||
| def _get_admin_state(self) -> tuple[State, int]: | ||
| return self._states[self.admin_id] |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
That should not be the private function right
| def _get_admin_state(self) -> tuple[State, int]: | ||
| return self._states[self.admin_id] | ||
| def get_state_info(self, update: object) -> StateInfo[Optional[int]]: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
Correct, the type variable specifying the type of keys you use.
| if not state_handlers[group]: | ||
| del state_handlers[group] |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
Does this remove the group if its empty? Can we comment it
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
yes, that's basically the same asstate_handlers.pop(group, None), I think. It will callstate_handlers.__del__(group). IMHO this is basic python syntax 😬
| from telegram.ext import JobQueue | ||
| _KT = TypeVar("_KT", bound=Hashable) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
Im back with my thinking about private stuff. Why do we private it here, not in other files; should we. Hm.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
I feel you :D ifmachine was a public module liketelegram.constants, it would have to be protected IMO. Since it's not and we expose the relevant elements only viatelegram.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
| application.fsm = UserSupportMachine(admin_id=123456) | ||
| application.fsm.set_job_queue(application.job_queue) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
these should be part of the builder and the second one set by default imo
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
yes, absolutely. #PoC 👼
| 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) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
Why weakref wont it/the user experience break if the supplied job queue gets destroyed
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
There isjob_queue.application. that's also weakref b/c we have a cyclic reference:application is application.job_queue.application. Now we also haveapplication 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.
| if raise_exception: | ||
| raise RuntimeError("JobQueue was garbage collected") |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
ha here see. why.
| 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") |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
is the error saying UID correct? I would understand the optional type hint to be anything hashable
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
Sorry, I don't get you. What do you think is wrong? the phrasing of the error message?
| class State(abc.ABC): | ||
| __knows_uids: ClassVar[set[str]] = set() |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
Wait I must really misunderstand smth, how does this work across states
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
uh … that's how class variables work? :D In the initself.__knows_uids is basically the same asState.__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? |
That is indeed a very good idea 👍
that way we wouldn't have to do any additional actual beta releases. |
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. |
Yes, this is supposed to replace CH. But, since this is acting on a different level than handlers, the
👍 |
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. |
Uh oh!
There was an error while loading.Please reload this page.
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)
context.job_queue.run_oncefor 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.State.ANYand things likeStateA | StateBmake it necessary that one update can be handled by multiple states. All this atleast needs proper documentation about what's going on. Additionally, we should think carefully about what we want here.get_active_key_stateshould be async or not:App.process_update, it should be as quick as possibleget_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 inprocess_updateBoundedSemaphore(1), we could probably just useLockas synchronization primitive