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

cPickle to pickle conversion in py3k missing methods #47635

Closed
jnoller mannequin opened this issue Jul 16, 2008 · 10 comments
Closed

cPickle to pickle conversion in py3k missing methods #47635

jnoller mannequin opened this issue Jul 16, 2008 · 10 comments
Assignees
Labels
extension-modules C modules in the Modules dir type-feature A feature request or enhancement

Comments

@jnoller
Copy link
Mannequin

jnoller mannequin commented Jul 16, 2008

BPO 3385
Nosy @amauryfa, @abalkin, @avassalotti
Files
  • add_dispatch_check-0.patch
  • Note: these values reflect the state of the issue at the time it was migrated and might not reflect the current state.

    Show more details

    GitHub fields:

    assignee = 'https://github.com/avassalotti'
    closed_at = <Date 2010-06-28.21:16:40.219>
    created_at = <Date 2008-07-16.19:01:36.599>
    labels = ['extension-modules', 'type-feature']
    title = 'cPickle to pickle conversion in py3k missing methods'
    updated_at = <Date 2010-06-28.21:16:40.194>
    user = 'https://bugs.python.org/jnoller'

    bugs.python.org fields:

    activity = <Date 2010-06-28.21:16:40.194>
    actor = 'alexandre.vassalotti'
    assignee = 'alexandre.vassalotti'
    closed = True
    closed_date = <Date 2010-06-28.21:16:40.219>
    closer = 'alexandre.vassalotti'
    components = ['Extension Modules']
    creation = <Date 2008-07-16.19:01:36.599>
    creator = 'jnoller'
    dependencies = []
    files = ['11048']
    hgrepos = []
    issue_num = 3385
    keywords = ['patch']
    message_count = 10.0
    messages = ['69816', '69866', '70165', '70169', '70676', '71159', '71302', '71394', '108729', '108870']
    nosy_count = 4.0
    nosy_names = ['amaury.forgeotdarc', 'belopolsky', 'alexandre.vassalotti', 'jnoller']
    pr_nums = []
    priority = 'normal'
    resolution = 'wont fix'
    stage = 'test needed'
    status = 'closed'
    superseder = None
    type = 'enhancement'
    url = 'https://bugs.python.org/issue3385'
    versions = ['Python 3.2']

    @jnoller
    Copy link
    Mannequin Author

    jnoller mannequin commented Jul 16, 2008

    I was attempting the patch for bpo-3125 to py3k, and in it Amaury
    defines a new ForkingPickler:

    from pickle import Pickler
    class ForkingPickler(Pickler):
        dispatch = Pickler.dispatch.copy()

    This is also related to bpo-3350 I suspect. However, using the
    pickle.Pickler module under py3k, there is no dispatch() method on the
    class:

    Trunk:
    Python 2.6b1+ (trunk:65015M, Jul 16 2008, 10:15:51) 
    [GCC 4.0.1 (Apple Inc. build 5465)] on darwin
    Type "help", "copyright", "credits" or "license" for more information.
    >>> import pickle
    >>> dir(pickle.Pickler)
    ['_BATCHSIZE', '__doc__', '__init__', '__module__', '_batch_appends', 
    '_batch_setitems', 'clear_memo', 'dispatch', 'dump', 'get', 'memoize', 
    'persistent_id', 'put', 'save', 'save_bool', 'save_dict', 
    'save_empty_tuple', 'save_float', 'save_global', 'save_inst', 
    'save_int', 'save_list', 'save_long', 'save_none', 'save_pers', 
    'save_reduce', 'save_string', 'save_tuple', 'save_unicode']
    >>> 
    
    py3k:
    Python 3.0b1+ (py3k:65011M, Jul 16 2008, 11:50:11) 
    [GCC 4.0.1 (Apple Inc. build 5465)] on darwin
    Type "help", "copyright", "credits" or "license" for more information.
    >>> import pickle
    >>> dir(Pickler)
    ['__class__', '__delattr__', '__doc__', '__eq__', '__format__', 
    '__ge__', '__getattribute__', '__gt__', '__hash__', '__init__', 
    '__le__', '__lt__', '__ne__', '__new__', '__reduce__', '__reduce_ex__', 
    '__repr__', '__setattr__', '__sizeof__', '__str__', '__subclasshook__', 
    'bin', 'clear_memo', 'dump', 'fast', 'memo', 'persistent_id']

    I think the fix for 3125 resolves your complaint in 3350, but is the
    lack of the dispatch method intentional?

    @jnoller jnoller mannequin assigned avassalotti Jul 16, 2008
    @jnoller jnoller mannequin added the extension-modules C modules in the Modules dir label Jul 16, 2008
    @avassalotti
    Copy link
    Member

    The omission of the dispatch dictionary was sort of intentional. But
    now, I think it would be a good idea to add it to _pickle.Pickler. I
    will write a patch ASAP.

    @avassalotti
    Copy link
    Member

    Just in case you are wondering why I haven't submitted a patch yet, I
    want to let you know that my home computer is currently broken. So, I
    won't be able to work on this until I get my computer fixed (which
    unfortunately could take a few weeks).

    @amauryfa
    Copy link
    Member

    A use case:
    http://aspn.activestate.com/ASPN/Cookbook/Python/Recipe/572213
    shows how code can use the Pickler.dispatch dict, but also some
    Pickler.save_xxx function which should be exposed as well.

    @avassalotti
    Copy link
    Member

    I got a preliminary patch that adds the dispatch dictionary. However,
    the patch leaks and it doesn't expose the save_reduce() method (which is
    needed by ForkingPickler).

    @avassalotti
    Copy link
    Member

    I ran into a few problems while trying to fix this issue. First, does
    someone know how to add class attributes on extension types? It sounds
    like I will need either some tp_dict hacking or a Pickler subclass.

    Second, which methods of Pickler should be made public? I know
    save_reduce() is needed, but would it be worthwhile to expose more? In
    the recipe Amaury linked (which abuses Pickler IMHO), save_global(),
    save_dict(), write() and memoize() are used. Exposing all save_* methods
    is out of question for now as none were written to be used standalone.

    Third, should Pickler allows pickling support for built-in types (e.g.,
    int, dict, tuple, etc) to be overridden? Currently, pickle.py allows it.
    However, I am not sure if it is a good idea to copy this "feature" in
    _pickle.c.

    @amauryfa
    Copy link
    Member

    Maybe I missed something, but the (subclassable) python implementation
    of Pickler is still available with another name:

    from pickle import _Pickler as Pickler
    class ForkingPickler(Pickler):
        dispatch = Pickler.dispatch.copy()

    @avassalotti
    Copy link
    Member

    Yeah, the old Pickler and Unpickler classes are available by design (to
    allow testing both implementation). You could subclass _Pickler as a
    temporary fix for this issue.

    @abalkin
    Copy link
    Member

    abalkin commented Jun 26, 2010

    Do I understand correctly that the issue is that python Pickler class has dispatch attribute but C Pickler does not? The add_dispatch_check-0.patch patch does not seem to add class attribute, it adds an instance attribute instead.

    I also noticed that there are many more class attributes that only Python implementation has:

    >>> for x in dir(pickle._Pickler):
    ...    if not (x.startswith('_') or hasattr(pickle.Pickler, x)):
    ...       print(x)
    ... 
    dispatch
    get
    memoize
    put
    save
    save_bool
    save_bytes
    save_dict
    save_float
    save_global
    save_list
    save_long
    save_none
    save_pers
    save_reduce
    save_str
    save_tuple

    The save_* methods are clearly internal and should probably be renamed to begin with __ unless they are intended to be overridden by Pickler subclases

    @abalkin abalkin added the type-feature A feature request or enhancement label Jun 26, 2010
    @avassalotti
    Copy link
    Member

    Do I understand correctly that the issue is that python
    Pickler class has dispatch attribute but C Pickler does
    not?

    Yes.

    The add_dispatch_check-0.patch patch does not seem
    to add class attribute, it adds an instance attribute
    instead.

    I know. That's why I said it was a preliminary patch. :-) Also, see msg71159 about the class vs instance attribute issue.

    There's a lot of code out there that do crazy things with pickle's dispatch dictionary. For now, it is best if we leave the method names alone. If people wants to keep doing their funky stuff with pickle, then we can point them to pickle._Pickler and pickle._Unpickler, which will always point to the Python implementations.

    That said, I do not believe it's a good idea to add the dispatch attribute to _pickle anymore. But, I acknowledge there is a need for overriding the pickling mechanism for specific types. We should find a clean way to support this.

    I am closing this issue as "won't fix".

    @ezio-melotti ezio-melotti transferred this issue from another repository Apr 10, 2022
    Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
    Labels
    extension-modules C modules in the Modules dir type-feature A feature request or enhancement
    Projects
    None yet
    Development

    No branches or pull requests

    3 participants