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

Make zlib accept keyword-arguments #60968

Closed
ebfe mannequin opened this issue Dec 24, 2012 · 21 comments
Closed

Make zlib accept keyword-arguments #60968

ebfe mannequin opened this issue Dec 24, 2012 · 21 comments
Labels
extension-modules C modules in the Modules dir type-feature A feature request or enhancement

Comments

@ebfe
Copy link
Mannequin

ebfe mannequin commented Dec 24, 2012

BPO 16764
Nosy @vstinner, @ezio-melotti, @vadmium, @serhiy-storchaka, @matrixise, @zhangyangyu
Files
  • zlib_keywords.patch
  • zlib_tests_pep8.patch: PEP8/PyFlakes-fixes for test_zlib.py
  • zlib_keyword_argument.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 = None
    closed_at = <Date 2016-09-04.04:17:30.905>
    created_at = <Date 2012-12-24.12:45:08.618>
    labels = ['extension-modules', 'type-feature']
    title = 'Make zlib accept keyword-arguments'
    updated_at = <Date 2016-09-04.04:17:30.904>
    user = 'https://bugs.python.org/ebfe'

    bugs.python.org fields:

    activity = <Date 2016-09-04.04:17:30.904>
    actor = 'martin.panter'
    assignee = 'none'
    closed = True
    closed_date = <Date 2016-09-04.04:17:30.905>
    closer = 'martin.panter'
    components = ['Extension Modules']
    creation = <Date 2012-12-24.12:45:08.618>
    creator = 'ebfe'
    dependencies = []
    files = ['28418', '28419', '44074']
    hgrepos = []
    issue_num = 16764
    keywords = ['patch']
    message_count = 21.0
    messages = ['178053', '178054', '178058', '178059', '178063', '259453', '272350', '272357', '272404', '272437', '272481', '272491', '272722', '272723', '272731', '272897', '272920', '273014', '273015', '273271', '273279']
    nosy_count = 8.0
    nosy_names = ['vstinner', 'ezio.melotti', 'ebfe', 'python-dev', 'martin.panter', 'serhiy.storchaka', 'matrixise', 'xiang.zhang']
    pr_nums = []
    priority = 'normal'
    resolution = 'fixed'
    stage = 'resolved'
    status = 'closed'
    superseder = None
    type = 'enhancement'
    url = 'https://bugs.python.org/issue16764'
    versions = ['Python 3.6']

    @ebfe
    Copy link
    Mannequin Author

    ebfe mannequin commented Dec 24, 2012

    The patch "zlib_keywords.patch" makes zlib's classes and functions accept keyword arguments as documented. It also fixes two cases in which the docstring differ from the documentation (decompress(data) vs. decompress(string) and compressobj(memlevel) vs. compressobj(memLevel)). Additional tests are provided.

    @ebfe ebfe mannequin added the stdlib Python modules in the Lib dir label Dec 24, 2012
    @ebfe
    Copy link
    Mannequin Author

    ebfe mannequin commented Dec 24, 2012

    Attaching a patch to fix all pep8/pyflakes warnings and errors in test_zlib.py

    @serhiy-storchaka
    Copy link
    Member

    serhiy-storchaka commented Dec 24, 2012

    I don't think it worth to add support of keyword for the first mandatory argument. This can freeze the poor and inconsistent () names. For example compress()/decompress() methods of bz2 and lzma objects doesn't support keyword arguments. And why you use "string" name for decompress() argument?

    Renaming of "memLevel" argument to "memlevel" is not backward compatible and can break third-part code (if anyone use it). This may require starting of deprecation process. Difference between a code and a documentation is a bug and should be fixed for all Python versions.

    Note, that calling a function with keyword arguments is a little slower (a lot of slower for fast functions) than calling a function with positional-only arguments.

    @serhiy-storchaka serhiy-storchaka added extension-modules C modules in the Modules dir type-feature A feature request or enhancement and removed stdlib Python modules in the Lib dir labels Dec 24, 2012
    @ebfe
    Copy link
    Mannequin Author

    ebfe mannequin commented Dec 24, 2012

    Nothing of what you mention is a problem of this patch.

    The memLevel-keyword was not supported as of now, only the docstring ("memLevel") and the documentation ("memlevel") mentioned it. There is no third-party code that could have used it.

    The current docstring says that a "string"-keyword should be used with decompress(), the documentation talks about a "data"-keyword. Both are not supported, the patch adds support for a "data"-keyword and fixes the docstring.

    @ebfe ebfe mannequin added stdlib Python modules in the Lib dir and removed extension-modules C modules in the Modules dir type-feature A feature request or enhancement labels Dec 24, 2012
    @serhiy-storchaka
    Copy link
    Member

    serhiy-storchaka commented Dec 24, 2012

    Sorry, I missed, not decompress(), but compress(). Except this small error all of what I said *is* a problem of this patch.

    @serhiy-storchaka serhiy-storchaka added extension-modules C modules in the Modules dir type-feature A feature request or enhancement and removed stdlib Python modules in the Lib dir labels Dec 24, 2012
    @vadmium
    Copy link
    Member

    vadmium commented Feb 3, 2016

    See bpo-8706 discussing adding keyword support to library functions in general.

    Functions the first patch affects:

    • zlib.compress(data, level=...): still relevant; see bpo-26243
    • compressobj(memlevel=...): fixed in documentation in revision fdb5d84f9948 instead
    • compressobj.compress(data): still relevant, but little benefit IMO
    • compressobj.flush(mode=...): still relevant; little benefit
    • zlib.decompress(data, wbits=..., bufsize=...): still relevant
    • decompressobj.decompress(data, max_length=...): still relevant
    • decompressobj.flush(length=...): still relevant, but not worthwhile IMO; see bpo-23200

    Most of the PEP-8 patch looks like pointless noise, and a lot of the remaining bits have probably already been addressed.

    @matrixise
    Copy link
    Member

    matrixise commented Aug 10, 2016

    martin or serhiy,

    can we move this issue to 3.6 ?

    @serhiy-storchaka
    Copy link
    Member

    serhiy-storchaka commented Aug 10, 2016

    PyArg_ParseTupleAndKeywords() and Argument Clinic now support positional-only arguments. Thus this my objection can be resolved. But performance argument still needs an investigation.

    In any case the patch needs updating, since the zlib module was rewritten for using Argument Clinic.

    @zhangyangyu
    Copy link
    Member

    zhangyangyu commented Aug 11, 2016

    I agree with Martin and suggest make the following changes:

    1. Make wbits and bufsize of zlib.decompress keyword arguments.
    2. Make max_length of decompressobj.decompress keyword argument.

    I'd prefer others to stay position-only as now.

    Attach a patch doing the above 2 changes.

    @vadmium
    Copy link
    Member

    vadmium commented Aug 11, 2016

    Xiang’s patch looks okay from a correctness point of view

    @serhiy-storchaka
    Copy link
    Member

    serhiy-storchaka commented Aug 11, 2016

    Xiang Zhang, could you please provide results of benchmarking zlib.decompress and decompressobj.decompress in simplest case with and without the patch? PyArg_ParseTupleAndKeywords can be slower than PyArg_ParseTuple even for positional arguments, and we should know how much.

    @zhangyangyu
    Copy link
    Member

    zhangyangyu commented Aug 12, 2016

    OK. Simplest test with positional arguments.

    Without patch:

    ./python -m timeit -s 'import zlib; a = zlib.compress(b"abcdefghijklmnopqrstuvwxyz")' 'zlib.decompress(a, 15, 16384)'
    1000000 loops, best of 3: 0.841 usec per loop

    ./python -m timeit -s 'import zlib; a = zlib.compress(b"abcdefghijklmnopqrstuvwxyz"); do = zlib.decompressobj()' 'do.decompress(a, 100)'
    10000 loops, best of 3: 16 usec per loop

    With patch:

    ./python -m timeit -s 'import zlib; a = zlib.compress(b"abcdefghijklmnopqrstuvwxyz")' 'zlib.decompress(a, 15, 16384)'
    1000000 loops, best of 3: 0.843 usec per loop

    ./python -m timeit -s 'import zlib; a = zlib.compress(b"abcdefghijklmnopqrstuvwxyz"); do = zlib.decompressobj()' 'do.decompress(a, 100)'
    10000 loops, best of 3: 16.1 usec per loop

    But, with keyword specified, there is a degrade.

    ./python -m timeit -s 'import zlib; a = zlib.compress(b"abcdefghijklmnopqrstuvwxyz")' 'zlib.decompress(a, wbits=15, bufsize=16384)'
    1000000 loops, best of 3: 1.26 usec per loop

    ./python -m timeit -s 'import zlib; a = zlib.compress(b"abcdefghijklmnopqrstuvwxyz"); do = zlib.decompressobj()' 'do.decompress(a, max_length=100)'
    10000 loops, best of 3: 16.8 usec per loop

    But with large data, the difference is gone:

    ./python -m timeit -s 'import zlib; a = zlib.compress(b"abcdefghijklmnopqrstuvwxyz"*10000)' 'zlib.decompress(a, 15, 16384)'
    1000 loops, best of 3: 252 usec per loop # without patch

    ./python -m timeit -s 'import zlib; a = zlib.compress(b"abcdefghijklmnopqrstuvwxyz"*10000)' 'zlib.decompress(a, wbits=15, bufsize=16384)'
    1000 loops, best of 3: 252 usec per loop # with patch

    So I think it's OK for this change. There seems no performance degrade to old code. And considering that zlib usually does time consuming tasks (I don't think it's common to decompress such small data), the small slower down seems affordable.

    @serhiy-storchaka
    Copy link
    Member

    serhiy-storchaka commented Aug 15, 2016

    LGTM. With bpo-27574 the overhead is even smaller.

    @serhiy-storchaka serhiy-storchaka self-assigned this Aug 15, 2016
    @python-dev
    Copy link
    Mannequin

    python-dev mannequin commented Aug 15, 2016

    New changeset a4101218364e by Serhiy Storchaka in branch 'default':
    Issue bpo-16764: Support keyword arguments to zlib.decompress(). Patch by
    https://hg.python.org/cpython/rev/a4101218364e

    @zhangyangyu
    Copy link
    Member

    zhangyangyu commented Aug 15, 2016

    Oops! I am on the way regenerating the CA output to catch up with hg tip, but after a meeting you have done it.

    Thanks for your work, Serhiy. And excellent job as for bpo-27574.

    @serhiy-storchaka serhiy-storchaka removed their assignment Aug 15, 2016
    @zhangyangyu
    Copy link
    Member

    zhangyangyu commented Aug 17, 2016

    Serhiy, the message added in Misc/NEWS should be in Library section not Core and Builtins section.

    @vstinner
    Copy link
    Member

    vstinner commented Aug 17, 2016

    Oops, I reviewed the patch before seeing that Serhiy already pushed it :-) Ignore my comment.

    @python-dev
    Copy link
    Mannequin

    python-dev mannequin commented Aug 18, 2016

    New changeset ab0039b8a80e by Serhiy Storchaka in branch 'default':
    Issue bpo-16764: Move NEWS entry to correct section and remove too strict test.
    https://hg.python.org/cpython/rev/ab0039b8a80e

    @serhiy-storchaka
    Copy link
    Member

    serhiy-storchaka commented Aug 18, 2016

    Serhiy, the message added in Misc/NEWS should be in Library section not Core and Builtins section.

    Done. And addressed Victor's reasonable comment. Thanks!

    @vadmium
    Copy link
    Member

    vadmium commented Aug 21, 2016

    All the interesting keyword arguments seem to work now (checking against my notes from earlier). Is there anything else anyone wants to do, or can we close this now?

    @zhangyangyu
    Copy link
    Member

    zhangyangyu commented Aug 21, 2016

    I think it's okay to close now.

    @vadmium vadmium closed this as completed Sep 4, 2016
    @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

    5 participants