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

Hack to fix pickling #197

Closed
wants to merge 2 commits into from

Conversation

adamjstewart
Copy link
Collaborator

@adamjstewart adamjstewart commented Dec 18, 2021

This is a hack that allows Index objects to be pickled. It works by copying the entire index into memory, pickling it, then rebuilding the entire index during unpickling. I hope that there is a more efficient, less hacky way to do this, but until then this at least lets users pickle rtree objects.

Fixes #87

@adamjstewart adamjstewart changed the title Tests: add expected failure for pickling Hack to fix pickling Dec 18, 2021
attrs = self.__dict__.copy()
del attrs["handle"]
entries = self.intersection(self.bounds, objects=True)
entries = [(item.id, item.bbox, item.object) for item in entries]
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

When interleaved=True, this seems to require item.bbox, but when interleaved=False, this seems to require item.bounds. I'm not even sure why there are two names for the same thing...

Copy link
Member

Choose a reason for hiding this comment

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

indeed that seems like some kind of bug due to refactorings. It should be tracked down all the way to figure out what's going on.

@hobu
Copy link
Member

hobu commented Feb 4, 2022

Can you provide a test for this pickle support? I don't really want to merge it until it's tested.

Also, I'm curious what would happen if the index's storage is a file instead of memory.

@adamjstewart
Copy link
Collaborator Author

Can you provide a test for this pickle support? I don't really want to merge it until it's tested.

This PR already contains testing for pickle support. We already had some testing, but we didn't actually test that the unpickled object matches the original object.

I don't actually want to merge this PR as is, as it's kind of hacky and I don't fully understand why this is necessary. I'm hoping that someone smarter than me can propose a better solution.

@hobu
Copy link
Member

hobu commented Feb 4, 2022

It would be some work, but I think you have everything needed in the libspatialindex C API to make a custom storage type for Python. That could be wired into more common Python things like buffers and such.

@adamjstewart
Copy link
Collaborator Author

I know very little about C/Cython so it might be better for someone else to do that. The goal of this PR was to push forward the discussion in #87 by proposing something so hacky that someone would investigate alternatives to avoid my current solution 😆

@hobu hobu closed this Feb 4, 2022
@adamjstewart adamjstewart deleted the tests/pickling branch February 4, 2022 17:33
@adamjstewart
Copy link
Collaborator Author

I'm wondering if we should revive this PR. Something hacky and slow that works is better than something that doesn't work. Being able to pickle an index is really important for any multiprocessing code on macOS/Windows.

@hobu
Copy link
Member

hobu commented Mar 21, 2022

https://github.com/Toblerity/zope.index.rtree implemented a custom storage type for Zope's object store. You could probably revive this to wire it up to pickles.

@codeananda
Copy link
Contributor

I assume this also doesn't work for cPickle?

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.

Pickling and unpickling messes up Index
3 participants