Repository navigation
Make zlib accept keyword-arguments #60968
Description
| BPO | 16764 |
|---|---|
| Nosy | @vstinner, @ezio-melotti, @vadmium, @serhiy-storchaka, @matrixise, @zhangyangyu |
| Files |
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']
Activity
ebfe commentedon 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 commentedon Dec 24, 2012
Attaching a patch to fix all pep8/pyflakes warnings and errors in test_zlib.py
serhiy-storchaka commentedon 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.
ebfe commentedon 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.
serhiy-storchaka commentedon 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.
5 remaining items
serhiy-storchaka commentedon 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 commentedon 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 commentedon Aug 15, 2016
LGTM. With bpo-27574 the overhead is even smaller.
python-dev commentedon 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 commentedon 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.
zhangyangyu commentedon Aug 17, 2016
Serhiy, the message added in Misc/NEWS should be in Library section not Core and Builtins section.
vstinner commentedon Aug 17, 2016
Oops, I reviewed the patch before seeing that Serhiy already pushed it :-) Ignore my comment.
python-dev commentedon 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 commentedon Aug 18, 2016
Done. And addressed Victor's reasonable comment. Thanks!
vadmium commentedon 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 commentedon Aug 21, 2016
I think it's okay to close now.