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

Support pickling objects with __new__ with keyword arguments with protocol 2+ #68352

Closed
serhiy-storchaka opened this issue May 11, 2015 · 19 comments
Assignees
Labels
extension-modules C modules in the Modules dir interpreter-core Interpreter core (Objects, Python, Grammar, and Parser dirs) stdlib Python modules in the Lib dir type-feature A feature request or enhancement

Comments

@serhiy-storchaka
Copy link
Member

serhiy-storchaka commented May 11, 2015

BPO 24164
Nosy @pitrou, @vstinner, @avassalotti, @serhiy-storchaka
Files
  • pickle_new_ex_protocol_2.patch
  • pickle_new_ex_protocol_2_doc.patch
  • pickle_new_ex_protocol_2_doc_2.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/serhiy-storchaka'
    closed_at = <Date 2015-10-13.18:29:17.151>
    created_at = <Date 2015-05-11.10:49:50.457>
    labels = ['extension-modules', 'interpreter-core', 'type-feature', 'library']
    title = 'Support pickling objects with __new__ with keyword arguments with protocol 2+'
    updated_at = <Date 2015-11-17.11:15:19.259>
    user = 'https://github.com/serhiy-storchaka'

    bugs.python.org fields:

    activity = <Date 2015-11-17.11:15:19.259>
    actor = 'python-dev'
    assignee = 'serhiy.storchaka'
    closed = True
    closed_date = <Date 2015-10-13.18:29:17.151>
    closer = 'serhiy.storchaka'
    components = ['Extension Modules', 'Interpreter Core', 'Library (Lib)']
    creation = <Date 2015-05-11.10:49:50.457>
    creator = 'serhiy.storchaka'
    dependencies = []
    files = ['39341', '40751', '40771']
    hgrepos = []
    issue_num = 24164
    keywords = ['patch']
    message_count = 19.0
    messages = ['242890', '251830', '251857', '252743', '252744', '252773', '252809', '252811', '252854', '252855', '252856', '252897', '252916', '252924', '252945', '252946', '252950', '252951', '254793']
    nosy_count = 6.0
    nosy_names = ['pitrou', 'vstinner', 'alexandre.vassalotti', 'Arfrever', 'python-dev', 'serhiy.storchaka']
    pr_nums = []
    priority = 'normal'
    resolution = 'fixed'
    stage = 'resolved'
    status = 'closed'
    superseder = None
    type = 'enhancement'
    url = 'https://bugs.python.org/issue24164'
    versions = ['Python 3.6']

    @serhiy-storchaka
    Copy link
    Member Author

    serhiy-storchaka commented May 11, 2015

    Pickling of objects of classes whose __new__ mandates the use of keyword-only arguments is supported with protocol 4 (using a new opcode NEWOBJ_EX). But it is possible to implement this feature with protocol 2+ (less efficiently than with NEWOBJ_EX). __new_ex__ is pickled as partial(cls.__new__, cls, *args, **kwargs). Pickled data is compatible with older Python releases up to 2.7 (bpo-5228).

    Proposed patch adds support of __new__ with keyword arguments with protocols 2 and 3.

    @serhiy-storchaka serhiy-storchaka added extension-modules C modules in the Modules dir interpreter-core Interpreter core (Objects, Python, Grammar, and Parser dirs) stdlib Python modules in the Lib dir type-feature A feature request or enhancement labels May 11, 2015
    @vstinner
    Copy link
    Member

    vstinner commented Sep 29, 2015

    "Pickling of objects of classes whose __new__ mandates the use of keyword-only arguments is supported with protocol 4 (using a new opcode NEWOBJ_EX)."

    Hum, can you please write a short example of such class which can only be pickled by the protocol 4 currently? Just for my information.

    I understand that some objects cannot be serialized by pickle with protocol lower than 4, whereas your change makes possible to serialize them on Python 3, and it will be possible to deserialize them on Python 2 and Python 3.

    If I understood correctly, the change makes sense.

    I reviewed the patch, it looks good to me.

    @serhiy-storchaka
    Copy link
    Member Author

    serhiy-storchaka commented Sep 29, 2015

    Hum, can you please write a short example of such class which can only be pickled by the protocol 4 currently? Just for my information.

    For now there are no such classes in the stdlib. No one implements __getnewargs_ex__. But an alternative implementation of pickling for methodcaller could use it (I implemented methodcaller pickling in bpo-22955 in different way, via __reduce_ex__, but used the same trick for passing keyword arguments to constructor).

    Note that multiprocessing uses default protocol 3 (bpo-23403), and this is not configurable.

    @python-dev
    Copy link
    Mannequin

    python-dev mannequin commented Oct 10, 2015

    New changeset bc5894a3a0e6 by Serhiy Storchaka in branch 'default':
    Issue bpo-24164: Objects that need calling __new__ with keyword arguments,
    https://hg.python.org/cpython/rev/bc5894a3a0e6

    @serhiy-storchaka
    Copy link
    Member Author

    serhiy-storchaka commented Oct 10, 2015

    Thank you for your review Victor.

    @vstinner
    Copy link
    Member

    vstinner commented Oct 11, 2015

    Buildbots failed.

    http://buildbot.python.org/all/builders/AMD64%20Debian%20root%203.x/builds/2807/steps/test/logs/stdio

    ======================================================================
    FAIL: test_reduce (test.test_descr.PicklingTests)
    ----------------------------------------------------------------------

    Traceback (most recent call last):
      File "/root/buildarea/3.x.angelico-debian-amd64/build/Lib/test/test_descr.py", line 4745, in test_reduce
        obj.__reduce_ex__(proto)
    AssertionError: ValueError not raised

    @vstinner vstinner reopened this Oct 11, 2015
    @python-dev
    Copy link
    Mannequin

    python-dev mannequin commented Oct 11, 2015

    New changeset df33dbbef7bb by Serhiy Storchaka in branch 'default':
    Issue bpo-24164: Fixed test_descr: __getnewargs_ex__ now is supported in protocols 2 and 3.
    https://hg.python.org/cpython/rev/df33dbbef7bb

    @serhiy-storchaka
    Copy link
    Member Author

    serhiy-storchaka commented Oct 11, 2015

    Thank you Victor. Tests are fixed.

    I think the documentation needs to be updated. Here is a patch for pickle documentation.

    @vstinner
    Copy link
    Member

    vstinner commented Oct 12, 2015

    "Thank you Victor. Tests are fixed."

    Not all of them. Failure on FreeBSD:

    (I don't know what is test_pyclbr!?)

    http://buildbot.python.org/all/builders/AMD64%20FreeBSD%2010.0%203.x/builds/3866/

    ======================================================================
    FAIL: test_others (test.test_pyclbr.PyclbrTest)
    ----------------------------------------------------------------------

    Traceback (most recent call last):
      File "/usr/home/buildbot/python/3.x.koobs-freebsd10/build/Lib/test/test_pyclbr.py", line 159, in test_others
        cm('pickle')
      File "/usr/home/buildbot/python/3.x.koobs-freebsd10/build/Lib/test/test_pyclbr.py", line 86, in checkModule
        self.assertIsInstance(py_item, (FunctionType, BuiltinFunctionType))
    AssertionError: <class 'functools.partial'> is not an instance of (<class 'function'>, <class 'builtin_function_or_method'>)

    @python-dev
    Copy link
    Mannequin

    python-dev mannequin commented Oct 12, 2015

    New changeset 288953a787ce by Victor Stinner in branch 'default':
    Issue bpo-24164: Fix test_pyclbr
    https://hg.python.org/cpython/rev/288953a787ce

    @vstinner
    Copy link
    Member

    vstinner commented Oct 12, 2015

    New changeset 288953a787ce by Victor Stinner in branch 'default':
    Issue bpo-24164: Fix test_pyclbr

    Another fix would be to accept functools.partial type, but it looks like the test wants to exclude symbols which comes from other modules.

    @vstinner
    Copy link
    Member

    vstinner commented Oct 12, 2015

    At least, test_pyclbr was fixed by my change. Most 3.x buildbots are green again.

    @serhiy-storchaka
    Copy link
    Member Author

    serhiy-storchaka commented Oct 13, 2015

    Thank you Victor for fixing test_pyclbr. test_pyclbr looks fragile and may be there are bugs in pyclbr itself. But this is different issue.

    Could you please look at proposed documentation changes?

    @vstinner
    Copy link
    Member

    vstinner commented Oct 13, 2015

    Serhiy Storchaka added the comment:

    Could you please look at proposed documentation changes?

    Sure, done.

    @serhiy-storchaka
    Copy link
    Member Author

    serhiy-storchaka commented Oct 13, 2015

    Documentation patch updated.

    @vstinner
    Copy link
    Member

    vstinner commented Oct 13, 2015

    pickle_new_ex_protocol_2_doc_2.patch looks good to me.

    @python-dev
    Copy link
    Mannequin

    python-dev mannequin commented Oct 13, 2015

    New changeset de982d8b7b15 by Serhiy Storchaka in branch 'default':
    Issue bpo-24164: Document changes to __getnewargs__ and __getnewargs_ex__.
    https://hg.python.org/cpython/rev/de982d8b7b15

    @serhiy-storchaka
    Copy link
    Member Author

    serhiy-storchaka commented Oct 13, 2015

    Thanks.

    @python-dev
    Copy link
    Mannequin

    python-dev mannequin commented Nov 17, 2015

    New changeset 7adc1d24d05b by Victor Stinner in branch 'default':
    Closes bpo-25645: Fix a reference leak introduced by change bc5894a3a0e6 of the
    https://hg.python.org/cpython/rev/7adc1d24d05b

    @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 interpreter-core Interpreter core (Objects, Python, Grammar, and Parser dirs) stdlib Python modules in the Lib dir type-feature A feature request or enhancement
    Projects
    None yet
    Development

    No branches or pull requests

    2 participants