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

Cannot unpickle AtomGroup onto nonexistant Universe #878

Open
kain88-de opened this issue Jun 12, 2016 · 13 comments
Open

Cannot unpickle AtomGroup onto nonexistant Universe #878

kain88-de opened this issue Jun 12, 2016 · 13 comments

Comments

@kain88-de
Copy link
Member

@kain88-de kain88-de commented Jun 12, 2016

Expected behaviour

I can just unpickle an AtomGroup without recreating a universe for it first. Unpickling should create a whole new universe object. we already save the filename of the topology. If we also save the trajectory filename this could be done.

Actual behaviour

tells me I can't unpickle it. There isn't a suitable loaded universe.
This weird behavior requires us to keep weak references to universes that aren't deleted.
So making this work out of box without loading universes separately would fix the too many files open errors we see on oxs.

Code to reproduce the behaviour

import pickle

with open('filename') as f:
    u = pickle.load(f)

Currently version of MDAnalysis:

(run python -c "import MDAnalysis as mda; print(mda.__version__)")

0.15.0

@kain88-de
Copy link
Member Author

@kain88-de kain88-de commented Jun 12, 2016

I get that the current behavior is there to share atomgroup selections between different people for the same universes. Does the pickle also store the current positions when they have been moved or does it use just the positions currently in the universe we load to (which is what it does looking at the code).

@kain88-de
Copy link
Member Author

@kain88-de kain88-de commented Jun 12, 2016

Maybe we could export MDAnalysis selections as well. Similar to the vmd/gromas selection writers we have already. That would serve the same purpose and also be very readable for a researcher.

@mnmelo
Copy link
Member

@mnmelo mnmelo commented Jun 12, 2016

I think the problem here is that the universe being unpickled isn't deemed appropriate to unpickle the underlying AtomGroups onto (u.atoms), as it doesn't really exist at the time.

Additionally, we want to make sure that those AtomGroups unpickle specifically onto the universe being unpickled (in case a compatible universe already exists).

@mnmelo mnmelo self-assigned this Jun 12, 2016
@kain88-de
Copy link
Member Author

@kain88-de kain88-de commented Jun 12, 2016

@dotsdl what do you need from the serialization protocol for MDSynthesis

@kain88-de
Copy link
Member Author

@kain88-de kain88-de commented Jun 12, 2016

@mnmelo what are you saying? I don't really understand it.

As far as I can see the pickle object only stores filenames and atom indices. Currently we also can't really decide unto which universe object a pickle is loaded to, at least from a user perspective.

@jbarnoud
Copy link
Contributor

@jbarnoud jbarnoud commented Jun 12, 2016

Won't pickling become obsolete with PR #831?

@mnmelo
Copy link
Member

@mnmelo mnmelo commented Jun 13, 2016

@kain88-de, I'm away from a computer, so I might be wrong, but if you are getting the "no suitable universe" error I believe it means python is trying to unpickle an AtomGroup.

I imagine that happens because a universe contains AtomGroup references, which get pickled along.

Anyway, soon I'll be able to check it properly and comment further.

@mnmelo
Copy link
Member

@mnmelo mnmelo commented Jun 13, 2016

Yup,
Universes lack __setstate__ and __getstate__ implementations. The default pickle/unpickle machinery will then hierarchically process references in the universe, and stall at the u.atoms AtomGroup.

I'll address this by setting up a naive pickle/unpickle pair that just stores the absolute path of topology and trajectory(ies), and recreates the universe from them.

@kain88-de
Copy link
Member Author

@kain88-de kain88-de commented Jun 13, 2016

Sorry about the title. I actually meant pickling AtomGroups.

@kain88-de
Copy link
Member Author

@kain88-de kain88-de commented Jun 13, 2016

Also @mnmelo your current approach with the weakref cache is problematic. It keeps universes from being garbage collected leaving a lot of open file handles around. See discussion in #853 and #874.

@dotsdl
Copy link
Member

@dotsdl dotsdl commented Jun 13, 2016

I don't see a way for pickling/unpickling of AtomGroup objects to ever have a clean solution, since an AtomGroup makes no sense without a Universe. Should we just drop AtomGroup as something that's meaningfully pickleable and only focus on making Universe objects pickleable?

The only use case I see for pickling anything in MDAnalysis is to use multiprocessing of any kind. Are there use cases beyond this we're trying to support?

@dotsdl
Copy link
Member

@dotsdl dotsdl commented Jun 13, 2016

@jbarnoud #831 goes a long way toward making Universe pickling cleaner, since it allows full preservation of the Topology without needing to shoehorn into existing formats. In #363 we currently don't support this AtomGroup pickling behavior at all, because honestly I don't see how we get around the issues discussed here and elsewhere. I think it's much better (and deterministic) to only support pickling of Universe objects.

@mnmelo
Copy link
Member

@mnmelo mnmelo commented Aug 25, 2016

Looking back on this discussion, my take on it is:

  • currently, it makes sense that you fail to unpickle AtomGroups if you don't have the right Universe already loaded. We could consider a machinery to automatically load a Universe when needed in these cases, but I think for now that's best left under user control;
  • in the future, let's wait for #363 and rethink the pickling/unpickling strategy;
  • as @dotsdl rightly put it, my only use for it so far comes when multiprocessing, but I imagine other state-preservation applications might be warranted.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Linked pull requests

Successfully merging a pull request may close this issue.

None yet
6 participants
You can’t perform that action at this time.