Skip to content
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 our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Persistence #1017

Merged
merged 21 commits into from Sep 20, 2018

Conversation

@Eldinnie
Copy link
Member

Eldinnie commented Feb 22, 2018

Adding a construct for adding persistence to PTB.

  • Added Basepersistence as an interface for persistence classes
  • Added PicklePersistence for persistence using pickle
  • Added DictPersistence for persistence using dict/JSON
  • Integrating the persistence into the dispatcher, conversationhandler and updater classes
  • Added helper methods for (de)serializing conversations and user/chat_data
@Eldinnie Eldinnie changed the title [DO NOT MERGER] Persistence Construct [DO NOT MERGE] Persistence Construct Feb 22, 2018
@Eldinnie Eldinnie changed the title [DO NOT MERGE] Persistence Construct Persistence Mar 1, 2018
@jsmnbom

This comment has been minimized.

Copy link
Member

jsmnbom commented Mar 22, 2018

@Eldinnie Is this ready for review? Did we discuss it in the chat and I just forgot about it?

@JosXa

This comment has been minimized.

Copy link
Contributor

JosXa commented Apr 5, 2018

Hi @SehgalDivij, this is an implementation that will generally solve the persistence for ptb. An abstract base class was provided that will allow users to implement their own serialization interfaces and pass them on to the library, effectively giving you the ability to serialize almost every collection in the library.

Copy link
Member

jsmnbom left a comment

Adding some comments, mostly relating to the overall implementation. I have not yet read the code through thoroughly :)

Attributes:
store_user_data (:obj:`bool`): Optional, Whether user_data should be saved by this
persistence class.
store_chat_data (:obj:`bool`): Optional. Whether user_data should be saved by this

This comment has been minimized.

Copy link
@jsmnbom

jsmnbom Apr 30, 2018

Member

chat_data*

Args:
store_user_data (:obj:`bool`, optional): Whether user_data should be saved by this
persistence class. Default is ``True``.
store_chat_data (:obj:`bool`, optional): Whether user_data should be saved by this

This comment has been minimized.

Copy link
@jsmnbom

jsmnbom Apr 30, 2018

Member

chat_data*

persistence a chance to finish up saving or close a database connection gracefully. If this
is not of any importance just pass will be sufficient.
"""
raise NotImplementedError

This comment has been minimized.

Copy link
@jsmnbom

jsmnbom Apr 30, 2018

Member

If this is not of importance, should it really raise NYE?

@@ -76,6 +76,10 @@ class ConversationHandler(Handler):
per_user (:obj:`bool`): Optional. If the conversationkey should contain the User's ID.
per_message (:obj:`bool`): Optional. If the conversationkey should contain the Message's
ID.
name (:obj:`str`): Optional. The name for this conversationhandler. Required for

This comment has been minimized.

Copy link
@jsmnbom

jsmnbom Apr 30, 2018

Member

Could we default to an internal counter or something?

This comment has been minimized.

Copy link
@Eldinnie

Eldinnie May 3, 2018

Author Member

That would rely on the conv_handlers keeping in the same order when the code is changing. Better to force naming IMO

@@ -76,6 +76,10 @@ class ConversationHandler(Handler):
per_user (:obj:`bool`): Optional. If the conversationkey should contain the User's ID.
per_message (:obj:`bool`): Optional. If the conversationkey should contain the Message's
ID.
name (:obj:`str`): Optional. The name for this conversationhandler. Required for
persistence
persistent (:obj:`bool`): Optional. If the conversations dict for this handler should be

This comment has been minimized.

Copy link
@jsmnbom

jsmnbom Apr 30, 2018

Member

If name is required, could we not just have a single persistent_name that turns persistence on if a string?

This comment has been minimized.

Copy link
@Eldinnie

Eldinnie May 3, 2018

Author Member

Interesting. But this way we also have the name seperate if not persistent. Might be usefull some day?

if self.persistence.store_chat_data:
self.chat_data = self.persistence.get_chat_data()
if not isinstance(self.chat_data, defaultdict):
raise ValueError("`chat_data must be of type defaultdict")

This comment has been minimized.

Copy link
@jsmnbom

jsmnbom Apr 30, 2018

Member

misplaced `

telegram/ext/dispatcher.py Outdated Show resolved Hide resolved
from telegram.ext import BasePersistence


class PicklePersistence(BasePersistence):

This comment has been minimized.

Copy link
@jsmnbom

jsmnbom Apr 30, 2018

Member

Is there ever a file where it makes sense to use single_file=False?

This comment has been minimized.

Copy link
@Eldinnie

Eldinnie May 3, 2018

Author Member

dunno, but it might

This comment has been minimized.

Copy link
@jsmnbom

jsmnbom Sep 18, 2018

Member

I'm not really a fan of having features that we aren't sure anyone can use... It's just more maintenance work for us :/

This comment has been minimized.

Copy link
@jsmnbom

jsmnbom Sep 18, 2018

Member

Also you can't choose the filetype (suffix) when using single_file=False.?

This comment has been minimized.

Copy link
@Eldinnie

Eldinnie Sep 18, 2018

Author Member

No, but I don;t really see a problem here. I think it's versatile enough this way.

telegram/ext/picklepersistence.py Show resolved Hide resolved
telegram/ext/picklepersistence.py Show resolved Hide resolved
Eldinnie added 2 commits May 3, 2018
Copy link
Member

jsmnbom left a comment

Few more comments, but otherwise I think this is probably good to go. Do we wanna merge it after or before #1100 ?

@@ -485,6 +492,8 @@ def _join_threads(self):

def signal_handler(self, signum, frame):
self.is_idle = False
if self.persistence:

This comment has been minimized.

Copy link
@jsmnbom

jsmnbom May 15, 2018

Member

This should probably be moved into the if self.running: block

This comment has been minimized.

Copy link
@Eldinnie

Eldinnie Sep 18, 2018

Author Member

why?

This comment has been minimized.

Copy link
@jsmnbom

jsmnbom Sep 18, 2018

Member

Cause otherwise we impede the ability to exit immediately? Like if the user presses CTRL-C (or whatever) twice, it shouldn't save persistence twice, it just quit ASAP imo

This comment has been minimized.

Copy link
@Eldinnie

Eldinnie Sep 18, 2018

Author Member

Good point

else:
self.persistence = None

if self.persistence:

This comment has been minimized.

Copy link
@jsmnbom

jsmnbom May 15, 2018

Member

Is there a reason this block isn't just in the if persistence: block above?

This comment has been minimized.

Copy link
@Eldinnie

Eldinnie Sep 18, 2018

Author Member

there was no good one

@Eldinnie

This comment has been minimized.

Copy link
Member Author

Eldinnie commented Sep 18, 2018

Ok for future reference (And for the future CHANGES.RST)
Beside the obvious persistence classes and construct in this PR I added 3 helper methods to telegram.utils.helpers namely:

  • enocde_conversations_to_json(conversations)
    to json enocde a conversations dict which uses tuples as keys (which give a ValueError when trying to encode as JSON. This helper JSON-serializes the tuples to before serializing the dict.
  • decode_conversations_from_json(json_string)
    To reverse the operation from the encoding. It takes a string as encoded by the previous function and returns a conversations dict as the conversationhandler expects it
  • decode_user_chat_data_from_json(data)
    When JSON-serializing user/chat_data to json the keys (which are ints) are converted to strings.
    This function tries to cast dicts keys to ints.
Copy link
Member

jsmnbom left a comment

Overall this looks brilliant!
I added a few comments with some suggestions and questions in places where I got a bit confused.

The only quib I really have implementation-wise, is that Persistence.update_* all receive the entire defaultdict, instead of only the changed part, but I'm not sure if there is really a good way to change this. The reason I'd want this btw, is that if you were to create a DB backend like mysql or redis or whatever, then that's probably what you'd want.

:meth:`update_user_data`.
* If you want to store conversation data with :class:`telegram.ext.ConversationHandler`, you
must overwrite :meth:`get_conversations` and :meth:`update_conversations`.
* :meth:`flush` will be called when the bot is shutdown, and must always be overwritten.

This comment has been minimized.

Copy link
@jsmnbom

jsmnbom Sep 18, 2018

Member

This docstring should probably be changed, since flush mustn't always be overwritten as it is implemented atm (which I think is a good thing)

telegram/ext/dictpersistence.py Show resolved Hide resolved
""":obj:`dict`: A dictionary handlers can use to store data for the chat."""
if persistence:
if not isinstance(persistence, BasePersistence):
self.logger.warning("persistence should be based on telegram.ext.BasePersistence")

This comment has been minimized.

Copy link
@jsmnbom

jsmnbom Sep 18, 2018

Member

Imo this should raise an exception

This comment has been minimized.

Copy link
@Eldinnie

Eldinnie Sep 18, 2018

Author Member

I thought about that. But if someone really wants to create their own version without basing it on Basepersistence they should be allowed

This comment has been minimized.

Copy link
@jsmnbom

This comment has been minimized.

Copy link
@Eldinnie

Eldinnie Sep 18, 2018

Author Member

ok seems legit

from telegram.ext import BasePersistence


class PicklePersistence(BasePersistence):

This comment has been minimized.

Copy link
@jsmnbom

jsmnbom Sep 18, 2018

Member

I'm not really a fan of having features that we aren't sure anyone can use... It's just more maintenance work for us :/

telegram/ext/picklepersistence.py Show resolved Hide resolved
from telegram.ext import BasePersistence


class PicklePersistence(BasePersistence):

This comment has been minimized.

Copy link
@jsmnbom

jsmnbom Sep 18, 2018

Member

Also you can't choose the filetype (suffix) when using single_file=False.?

telegram/utils/helpers.py Show resolved Hide resolved
u = Updater(bot=bot, persistence=pickle_persistence_2)
dp = u.dispatcher
dp.add_handler(h2)
dp.process_update(update)

This comment has been minimized.

Copy link
@jsmnbom

jsmnbom Sep 18, 2018

Member

Should this check to make sure that first and second even got called?

Eldinnie added 2 commits Sep 18, 2018
As discussed with @jsmnbom
@Eldinnie

This comment has been minimized.

Copy link
Member Author

Eldinnie commented Sep 18, 2018

only pypy failing on Travis.
Coverage can be checked here. Diff coverage is 94.04%
Please check if you're happy now @jsmnbom and others welcome for review (@tsnoam @jh0ker )

Copy link
Member

jsmnbom left a comment

We should probably add some help to the wiki shortly after merge.
Maybe even an example?

Overall really amazing work @Eldinnie ! :D

Eldinnie added 2 commits Sep 19, 2018
@jsmnbom jsmnbom merged commit 4397903 into master Sep 20, 2018
2 of 3 checks passed
2 of 3 checks passed
continuous-integration/travis-ci/pr The Travis CI build failed
Details
Hound No violations found. Woof!
continuous-integration/appveyor/pr AppVeyor build succeeded
Details
@Eldinnie Eldinnie deleted the presistence branch Sep 21, 2018
@owneroxxor

This comment has been minimized.

Copy link

owneroxxor commented Oct 4, 2018

Already using PTB 11.1.0 but my script gives error when trying to import BasePersistence fromtelegram.ext.
Is persistence feature already launched?

@Eldinnie

This comment has been minimized.

Copy link
Member Author

Eldinnie commented Oct 4, 2018

@owneroxxor it's merged into master, but not yet released. In the next released version it will be available.

@alisherafat01

This comment has been minimized.

Copy link

alisherafat01 commented Oct 26, 2018

@Eldinnie when do you release this feature? we need this!

@davidh-

This comment has been minimized.

Copy link

davidh- commented Oct 27, 2018

@Eldinnie when do you release this feature? we need this!

Also would like a release! I guess for the meantime we can install from master.

@jsmnbom jsmnbom mentioned this pull request Oct 29, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
6 participants
You can’t perform that action at this time.