Skip to content

Issue 332 abc data#427

Closed
emfdavid wants to merge 1 commit intouqfoundation:masterfrom
emfdavid:issue_332_abc_data
Closed

Issue 332 abc data#427
emfdavid wants to merge 1 commit intouqfoundation:masterfrom
emfdavid:issue_332_abc_data

Conversation

@emfdavid
Copy link

@emfdavid emfdavid commented Aug 31, 2021

Resolves #332

Register ABCMeta to use StockPickler. If workable, this is preferable to the cloudpickle solution that depends heavily on the internals of the ABC implementation.

This will remain broken on some versions of Python 3.7 that did not support pickling the new c implementation of ABC.

Issues:

  • Registering ABCMeta seems to exclude other registered types, for instance the module/locality solution for ClassType (see broken test)
  • Need to confirm expected behavior for ABC registry once locality issue is solved

@mmckerns
Copy link
Member

you may want to check against any of these tests that are relevant: https://github.com/cloudpipe/cloudpickle/blob/343da119685f622da2d1658ef7b3e2516a01817f/tests/cloudpickle_test.py#L1170

@emfdavid emfdavid force-pushed the issue_332_abc_data branch from 8730f98 to af11bbe Compare August 31, 2021 18:08
@mmckerns
Copy link
Member

Can you give a short summary of what you feel is done and what is left to do? I'm heading toward a release, and trying to get some nagging issues/PRs tied up. I'll help with this if possible.

@emfdavid
Copy link
Author

emfdavid commented Oct 19, 2021

@mmckerns Status update
I spent a bit more time on this today, but it is still not complete.
There seem to be three remaining issues:

  1. The implementation for <locals> is not satisfactory. The class path is not the same for the deserialized objects when they are locals rather than normal module objects.
  2. The register function does not behave as expected based on the tests and issues observed in CloudPickle.
  3. The current implementation does not work for python < 3.6. I believe the if registry is None clause of the cloud pickle implementation handles the post/pre 3.6 implementation change.

I don't think anything that previously worked would be broken by these changes, but I have not confirmed that ABC classes failed to pickle in python <= 3.6

@anivegesana
Copy link
Contributor

@mmckerns @emfdavid Take a look at https://github.com/anivegesana/dill/tree/issue_332_abc_data and see if it is going in the direction that you want it.

@mmckerns
Copy link
Member

mmckerns commented Jan 2, 2022

@mmckerns @emfdavid Take a look at https://github.com/anivegesana/dill/tree/issue_332_abc_data and see if it is going in the direction that you want it.

@anivegesana: Yeah, this is a good direction.

@anivegesana
Copy link
Contributor

@emfdavid Bump

@mmckerns
Copy link
Member

@anivegesana: feel free to assume the answer is yes, as opposed to waiting.

@emfdavid
Copy link
Author

emfdavid commented Jan 19, 2022 via email

@emfdavid
Copy link
Author

Very cool @anivegesana - 2ce1c9e looks really good.
Tests are much improved using copy instead of dump and load and the implementation is cleaner. I think I can see how you are properly handling the registries now too.
Please don't block on me from here. Thank you!

@emfdavid emfdavid closed this Jan 20, 2022
@emfdavid emfdavid deleted the issue_332_abc_data branch January 20, 2022 03:22
@mmckerns mmckerns added this to the dill-0.3.5 milestone Apr 22, 2022
anivegesana pushed a commit to anivegesana/dill that referenced this pull request May 9, 2022
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.

TypeError: can't pickle _abc_data objects

3 participants