Skip to content

Conversation

@Bibo-Joshi
Copy link
Member

@Bibo-Joshi Bibo-Joshi commented Nov 25, 2018

As reported in #1297, the use of shallow copies in PicklePersistence leads to data not being persisted. Same holds for DictPersistence A fix would be the use of copy.deepcopy, which however may become time consuming for large data sets.

I therefore implemented an additional argument on_update for Pickle/DictPersistence that let's you choose wether to check for updates before dumping or not.
If on_update = False, checking is obmitted and one can skip time consuming deepcopies (i.e. just pass the actual data). If on_update = True, deepcopies are used.

@jsmnbom jsmnbom requested a review from Eldinnie January 4, 2019 19:29
@jsmnbom jsmnbom added the 📋 pending-review work status: pending-review label Jan 4, 2019
@Bibo-Joshi
Copy link
Member Author

If this is merged, #1325 needs to be revisited. Similar adjustments have to be made for bot_data.

@jsmnbom
Copy link
Member

jsmnbom commented Feb 13, 2019

I would like to merge this for the V12 beta that I'm working on.
But I'm gonna hold of for now because of the following (please correct me if I'm wrong :))

  • Tests are updated but don't actually include a regression test for PicklePersistence of user_data set within conversations #1297.
  • I still don't 100% understand why we need to copy the data at all?
  • And I'm not really sure if on_update is a descriptive enough name for the parameter if we do need it.
  • It also feels the parameter should be on BasePersistence and not only subclasses of it?

@jsmnbom
Copy link
Member

jsmnbom commented Feb 13, 2019

Oh and feel free to reach out to me on telegram @jsmnbom if you wanna help convince and fix these things together so we can get it v12 beta ready :)

@jsmnbom jsmnbom added this to the V12 milestone Feb 13, 2019
@jsmnbom jsmnbom mentioned this pull request Feb 13, 2019
@jsmnbom
Copy link
Member

jsmnbom commented Feb 13, 2019

Superseeded by #1342. We choose to go a simpler route and always deepcopy the data. Thanks a lot for both finding the issue and taking the time to make the PR :D

@jsmnbom jsmnbom closed this Feb 13, 2019
@Bibo-Joshi Bibo-Joshi deleted the pp-deep-copy branch February 14, 2019 12:32
@github-actions github-actions bot locked and limited conversation to collaborators Aug 19, 2020
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Labels

📋 pending-review work status: pending-review

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants