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

Refs #510 - Models can no longer be pickled #511

Open
wants to merge 2 commits into
base: master
Choose a base branch
from

Conversation

alexhayes
Copy link

@alexhayes alexhayes commented Aug 14, 2017

See #510

Essentially MappingProxyType does not support pickle and thus models could not be pickled anymore.

I've replaced MappingProxyType with dict.

As far as I can tell the access to the only attribute that was using MappingProxyType (schematics.ModelDict._valid) is governed by the API (of course this is Python and we all need to behave as consenting adults) and thus perhaps a read-only dict is not that important?

If it's deemed that this solution is OK I'll fix up the Python 2.6/2.7 failing tests.

@alexhayes alexhayes changed the title WIP - Refs #510 - Models can no longer be pickled Refs #510 - Models can no longer be pickled Aug 14, 2017
@alexhayes
Copy link
Author

alexhayes commented Aug 15, 2017

Just to be sure that MappingProxyType is not supposed to support pickle I've raised this https://bugs.python.org/issue31209

@lkraider
Copy link
Contributor

I am thinking of instead of using different dicts, actually marking the values with metadata to know if they are valid or not. This would simplify the structure back to a single container which could expose iterators for data in each state.

@alexhayes
Copy link
Author

@lkraider given what you've said above will that be something that will be happening in the short term or is it more a mid-long term goal?

I'm just wondering if I should expect that this PR will be merged (in which case I might use my fork for the time being knowing that it will be merged) or if I should hold off upgrading from 2.0.0a1 altogether for the time being.

@lkraider
Copy link
Contributor

I want to fix this by removing the MappingProxyType yes, but I would recommend if you require pickling stick with what works for you right now. I'm updating the internals using this PR + tests, so this PR might not merge as-is afterwards.

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

Successfully merging this pull request may close these issues.

None yet

2 participants