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

Allow all sources and caches to be pickled #302

Merged
merged 4 commits into from
Mar 12, 2021

Conversation

nsmith-
Copy link
Collaborator

@nsmith- nsmith- commented Mar 12, 2021

Cutting off at the resource acquisition.
The only negative I can think of is it might take people by surprise that the pickle does not completely contain the original data. But I also hope they do not expect it to behave like that.. probably worth a warning in the docs?

@nsmith- nsmith- marked this pull request as draft March 12, 2021 03:49
@nsmith- nsmith- marked this pull request as ready for review March 12, 2021 04:40
Copy link
Member

@jpivarski jpivarski left a comment

Choose a reason for hiding this comment

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

Some time ago (#58), I detached file objects from ROOT objects that didn't need them—anything that can't initiate reading (which is everything except for ReadOnlyDirectory and HasBranches)—so that they could be pickled.

This PR extends pickleability to those last few cases by reopening the file from a pickled state. It won't work if the file has been moved or the pickled object is sent to another computer or something, but that shouldn't be surprising. I think it's not bad to have both solutions, since detaching the file is a cleaner solution when it's possible. (Those unattached objects can be fully used when sent somewhere remote.)

I don't think we have to document the fact that pickling empties caches and that a pickled TTree sent somewhere where the file can't be found would break file-reading. That's such an oddball case that bringing it up might cause more confusion than not mentioning it.

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.

2 participants