Improve timeouts handling in conversation handlers by starry-shivam · Pull Request #2417 · python-telegram-bot/python-telegram-bot

starry-shivam · 2021-03-05T11:15:41Z

Closes #2407

Signed-off-by: starry69 <starry369126@outlook.com>

Bibo-Joshi

Besides the comment I left, I'm still missing a few things:

We should add a note that timeout with run_async may fail, if the promise takes longer than the timeout (as described in #2417)
We need a test that makes sure the timeout handlers are not triggered, if the promise resolves to CH.END. Ideally, we'd also have a test that makes sure that stuff still works if the promise raises an exception
Timeout of child conversations needs to be detected by parent conversations somehow. If we can't get that to work, the added warning needs to be reformulated and the limitation needs to be documented. picking up my idea from off-line to have check_update check if the child has timed out in the meantime: that's not stright forward, as the childs state is then None and also the child may be used in multiple parent conversations (having the same CH instance as child in multiple parents is surely somewhat crazy but we can't prevent that either …).

TBH while typing this I feel like it may just not be worth the effort and we might just want to document the limitation for now and revisit only if someone complains …

PS: most of the test fails are due to #2409 …

telegram/ext/conversationhandler.py

Signed-off-by: starry69 <starry369126@outlook.com>

Bibo-Joshi

My third bullet point remains to be resolved. Do you have a preference on that?
Test failures: Seems like TG doesn't like our test bots anymore … I had problems with the fallback bots on local tests in the last time.

tests/test_conversationhandler.py

telegram/ext/conversationhandler.py

starry-shivam · 2021-03-07T08:48:39Z

My third bullet point remains to be resolved. Do you have a preference on that?

Imo we should leave it for now as you said, and just add a note in documentation that: using timeouts with nested conversation may cause troubles. TBH I've never seen anyone complaining about timeouts in nested conversation triggering incorrectly, even when it was bugged, which tells that users rarely use timeouts in nested conversation, besides, now we also have a warning in place so ig we should be fine...

Bibo-Joshi · 2021-03-07T08:53:14Z

All right. I'd just to reformulate the note & warning in this case, to be more explicit. E.g. something like

Using conversation_timeout with nested conversations is currently not supported. You can still try to use it, but it will likely behave differntly from what you expect.

for both

Signed-off-by: starry69 <starry369126@outlook.com>

Bibo-Joshi

Thanks! I'll wait with merging, though, let's first see if we can get the tests to work again …

starry-shivam · 2021-03-07T10:24:24Z

Thanks! I'll wait with merging, though, let's first see if we can get the tests to work again …

Yep sure 👍

Poolitzer

code changes look good to me

telegram/ext/conversationhandler.py

Signed-off-by: starry69 <starry369126@outlook.com>

Poolitzer

GJ!

Bibo-Joshi · 2021-03-10T16:45:11Z

tests now look good, but coverage is not happy. @starry69 can you check why line 647 in CH is not covered?

Signed-off-by: starry69 <starry369126@outlook.com>

starry-shivam · 2021-03-11T04:19:49Z

tests now look good, but coverage is not happy. @starry69 can you check why line 647 in CH is not covered?

Fixed

Signed-off-by: starry69 <starry369126@outlook.com>

…/python-telegram-bot into conv-timeout

Signed-off-by: starry69 <starry369126@outlook.com>

github-actions

Hey! Looks like you edited README.rst or README_RAW.rst. I'm just a friendly reminder to apply relevant changes to both of those files :)

github-actions

Hey! Looks like you edited the (dev) requirements or the pre-commit hooks. I'm just a friendly reminder to keep the pre-commit hook versions in sync with the dev requirements and the additional dependencies for the hooks in sync with the requirements :)

github-actions

Hey there. Relax, I am just a little warning for the maintainers to release directly after merging your PR, otherwise we have broken examples and people might get confused :)

Bibo-Joshi

Thanks for re-working :)
Overall looks good, I left some remarks

telegram/ext/conversationhandler.py

telegram/ext/utils/promise.py

telegram/ext/conversationhandler.py

tests/test_conversationhandler.py

tests/test_promise.py

Signed-off-by: starry69 <starry369126@outlook.com>

Bibo-Joshi

Had a look at the updates and left some minor comments. Codecov complains that the added except in Promise isn't hit …

telegram/ext/conversationhandler.py

Signed-off-by: starry69 <starry369126@outlook.com>

…/python-telegram-bot into conv-timeout

Signed-off-by: starry69 <starry369126@outlook.com>

SwayamSodha

Brilent Code it's wonderful.

Bibo-Joshi

Thanks for your patience @starry69 ! Almost done, just some minor nit picking left :)

telegram/ext/conversationhandler.py

telegram/ext/utils/promise.py

Signed-off-by: starry69 <starry369126@outlook.com>

Bibo-Joshi

I'm happy now :) @Poolitzer do you want to review again?

Poolitzer

Good QoL change!

Poolitzer · 2021-04-29T07:58:11Z

telegram/ext/conversationhandler.py

+
+            Note:
+                 Using `conversation_timeout` with nested conversations is currently not
+                 supported. You can still try to use it, but it will likely behave differently


Suggested change

supported. You can still try to use it, but it will likely behave differently

supported. You can try to use it, but it will likely behave differently

Poolitzer · 2021-04-29T07:58:30Z

telegram/ext/conversationhandler.py

+                if isinstance(handler, self.__class__):
+                    warnings.warn(
+                        "Using `conversation_timeout` with nested conversations is currently not "
+                        "supported. You can still try to use it, but it will likely behave "


Suggested change

"supported. You can still try to use it, but it will likely behave "

"supported. You can try to use it, but it will likely behave "

starry-shivam added 2 commits March 5, 2021 07:11

Handle promise states in conversation timeout

c90abef

Signed-off-by: starry69 <starry369126@outlook.com>

warn if nested conversation & timeout

e90c0ae

Signed-off-by: starry69 <starry369126@outlook.com>

Bibo-Joshi requested changes Mar 5, 2021

View reviewed changes

telegram/ext/conversationhandler.py Outdated Show resolved Hide resolved

starry-shivam added 3 commits March 6, 2021 14:03

Add notes and test for conversation_timeout

4ee4955

Signed-off-by: starry69 <starry369126@outlook.com>

Try to fix pre-commit

0c6c8f4

Signed-off-by: starry69 <starry369126@outlook.com>

Test promise exception

1d368cc

Signed-off-by: starry69 <starry369126@outlook.com>

starry-shivam requested a review from Bibo-Joshi March 7, 2021 04:02

Welp

38f5f21

Signed-off-by: starry69 <starry369126@outlook.com>

Bibo-Joshi requested changes Mar 7, 2021

View reviewed changes

tests/test_conversationhandler.py Outdated Show resolved Hide resolved

tests/test_conversationhandler.py Show resolved Hide resolved

telegram/ext/conversationhandler.py Outdated Show resolved Hide resolved

improve docs

e3cee40

Signed-off-by: starry69 <starry369126@outlook.com>

Bibo-Joshi approved these changes Mar 7, 2021

View reviewed changes

Poolitzer requested changes Mar 7, 2021

View reviewed changes

telegram/ext/conversationhandler.py Outdated Show resolved Hide resolved

telegram/ext/conversationhandler.py Outdated Show resolved Hide resolved

telegram/ext/conversationhandler.py Show resolved Hide resolved

typo

448e748

Signed-off-by: starry69 <starry369126@outlook.com>

Poolitzer approved these changes Mar 7, 2021

View reviewed changes

Merge branch 'master' into conv-timeout

a2f9b91

starry-shivam force-pushed the conv-timeout branch from f7895d1 to a2f9b91 Compare March 11, 2021 04:07

try to fix codecov

31896ac

Signed-off-by: starry69 <starry369126@outlook.com>

Bibo-Joshi added this to the v13.5 milestone Mar 11, 2021

starry-shivam added 3 commits March 20, 2021 06:51

refactor timeout logic with promise.add_done_cb

1e5984e

Signed-off-by: starry69 <starry369126@outlook.com>

Merge branch 'conv-timeout' of https://github.com/python-telegram-bot…

3015712

…/python-telegram-bot into conv-timeout

small fix

23dddfe

Signed-off-by: starry69 <starry369126@outlook.com>

github-actions bot reviewed Mar 20, 2021

View reviewed changes

Bibo-Joshi changed the base branch from master to doc-fixes March 21, 2021 09:14

Bibo-Joshi changed the base branch from doc-fixes to master March 21, 2021 09:14

Bibo-Joshi requested changes Mar 21, 2021

View reviewed changes

Address review

a1e858c

Signed-off-by: starry69 <starry369126@outlook.com>

Bibo-Joshi reviewed Mar 22, 2021

View reviewed changes

telegram/ext/conversationhandler.py Outdated Show resolved Hide resolved

telegram/ext/conversationhandler.py Outdated Show resolved Hide resolved

telegram/ext/conversationhandler.py Outdated Show resolved Hide resolved

telegram/ext/conversationhandler.py Outdated Show resolved Hide resolved

Bibo-Joshi and others added 4 commits March 24, 2021 08:11

Fix some type hinting

d8555d5

Few fixes

ef9815e

Signed-off-by: starry69 <starry369126@outlook.com>

Merge branch 'conv-timeout' of https://github.com/python-telegram-bot…

9bede76

…/python-telegram-bot into conv-timeout

fix tests

aeceab6

Signed-off-by: starry69 <starry369126@outlook.com>

SwayamSodha approved these changes Mar 24, 2021

View reviewed changes

Bibo-Joshi requested changes Mar 26, 2021

View reviewed changes

telegram/ext/conversationhandler.py Outdated Show resolved Hide resolved

telegram/ext/conversationhandler.py Show resolved Hide resolved

telegram/ext/utils/promise.py Outdated Show resolved Hide resolved

minor nitpick

9eba935

Signed-off-by: starry69 <starry369126@outlook.com>

Bibo-Joshi approved these changes Mar 27, 2021

View reviewed changes

Poolitzer approved these changes Apr 29, 2021

View reviewed changes

Bibo-Joshi merged commit 7e55458 into master Apr 30, 2021

Bibo-Joshi deleted the conv-timeout branch April 30, 2021 08:10

github-actions bot locked and limited conversation to collaborators May 1, 2021

	supported. You can still try to use it, but it will likely behave differently
	supported. You can try to use it, but it will likely behave differently

	"supported. You can still try to use it, but it will likely behave "
	"supported. You can try to use it, but it will likely behave "

Conversation

starry-shivam commented Mar 5, 2021

Uh oh!

Bibo-Joshi left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Bibo-Joshi left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

starry-shivam commented Mar 7, 2021

Uh oh!

Bibo-Joshi commented Mar 7, 2021

Uh oh!

Bibo-Joshi left a comment

Choose a reason for hiding this comment

Uh oh!

starry-shivam commented Mar 7, 2021

Uh oh!

Poolitzer left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Poolitzer left a comment

Choose a reason for hiding this comment

Uh oh!

Bibo-Joshi commented Mar 10, 2021

Uh oh!

starry-shivam commented Mar 11, 2021

Uh oh!

github-actions bot left a comment

Choose a reason for hiding this comment

Uh oh!

github-actions bot left a comment

Choose a reason for hiding this comment

Uh oh!

github-actions bot left a comment

Choose a reason for hiding this comment

Uh oh!

Bibo-Joshi left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Bibo-Joshi left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

SwayamSodha left a comment

Choose a reason for hiding this comment

Uh oh!

Bibo-Joshi left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Bibo-Joshi left a comment

Choose a reason for hiding this comment

Uh oh!

Poolitzer left a comment

Choose a reason for hiding this comment

Uh oh!

Poolitzer Apr 29, 2021

Choose a reason for hiding this comment

Bibo-Joshi left a comment •

edited

Loading