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

Make the map pickable natively #1812

Conversation

BastienGauthier
Copy link
Contributor

@BastienGauthier BastienGauthier commented Oct 6, 2023

This PR needs a modification in branca (python-visualization/branca#142).
For the sake of testing in the WIP version, i repeated the modification here.

For now, the Map can be pickled and unpickled, but it is not rendered properly when I try to cope with children. If I use :

with open(f"{folder}/map_w_children_pickle.pkl",'rb') as f: map_unpickled = pickle.load(f)

I need to add :

for key in map_unpickled._children.keys(): map_unpickled._children[key]._parent = map_unpickled

So everything is fine.
For some reason, adding these lines to the end of the init() does not work.

Linked issue : #1813

@BastienGauthier BastienGauthier changed the title Make the map pickable using copyreg [WIP] Make the map pickable using copyreg Oct 6, 2023
add set_parent_for_children function needed after unpickling
Copy link
Member

@Conengmo Conengmo left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Interesting topic, to make maps pickable. I don't have much experience with custom pickling myself. I'm not opposed to this or anything, and I want to help make this work, but I do have some comments.

  • I'd rather not add more 'stuff' to the Map class, which is already pretty crowded. I'd like it if possible to have special code to make things pickable packed together.
  • Do we need to add all arguments to self? Isn't what's in the self of Map now not enough to recreate it fully?
  • Would the re-adding of children be more appropriate in the basic Element class in Branca? That's where the other logic about children is. Maybe that way pickling and unpickling becomes something generic for all possible elements.

@martinfleis
Copy link
Collaborator

Can we just use the combo of __getstate__ and __setstate__ with the two lines you mentioned above?

for key in map_unpickled._children.keys():
   map_unpickled._children[key]._parent = map_unpickled

That would feel like a natural solution to me but I haven't tested it.

@BastienGauthier
Copy link
Contributor Author

BastienGauthier commented Oct 11, 2023

I'll try to detach the pickling code from the class Map as much as possible. However, for now adding the argument avec function is the only way I found to make it picklable. The main error initially raised by pickle module is that there is an external function called in the module (but I cannot identify which one...).

PicklingError: Can't pickle <function root at 0x000001E354BBFE20> : attribute lookup root on __main__ failed

That's why I used copyreg : unfortunately using this seems to "cancel" the setstate and getstate. @martinfleis : that's why I did not succeed to make it work with the elegant solution you mentionned. If we can identify which external function raise the pickling error in the first place, we could remove copyreg and use setstate, solving the problem I guess.

I did not though of re-adding the children in Element class of branca, I will try do this to precise the issue, thanks for the idea !

@BastienGauthier
Copy link
Contributor Author

I got some results today while removing copyreg and extra arguments in init. The task is to deal with the jinja2 Template, which are not pickable (as for the Environnement).

Part of the work is in branca, as python-visualization/branca#99 made the Element without Template pickable, not all the element. I can now pickle all Elements, the challenge is now to recover the Templates afterwards for unpickling (I did some work in the __setstate__, but it needs a little more magic to work completely).

@BastienGauthier BastienGauthier changed the title [WIP] Make the map pickable using copyreg Make the map pickable natively Oct 12, 2023
@BastienGauthier
Copy link
Contributor Author

Closing this as I go with branca changes only : https://github.com/python-visualization/branca/pull/144/files#diff-6886114fc8e4fd6197705978c768e1f49469e2455fb1c4a8efda26a7b11bfa0b.
Sorry for the back and forth, but I must I missed something in the first place that misdirected me.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants