classification
Title: Make zlib accept keyword-arguments
Type: enhancement Stage: resolved
Components: Extension Modules Versions: Python 3.6
process
Status: closed Resolution: fixed
Dependencies: Superseder:
Assigned To: Nosy List: ebfe, ezio.melotti, haypo, martin.panter, matrixise, python-dev, serhiy.storchaka, xiang.zhang
Priority: normal Keywords: patch

Created on 2012-12-24 12:45 by ebfe, last changed 2016-09-04 04:17 by martin.panter. This issue is now closed.

Files
File name Uploaded Description Edit
zlib_keywords.patch ebfe, 2012-12-24 12:45 review
zlib_tests_pep8.patch ebfe, 2012-12-24 12:46 PEP8/PyFlakes-fixes for test_zlib.py
zlib_keyword_argument.patch xiang.zhang, 2016-08-11 06:58 review
Messages (21)
msg178053 - (view) Author: Lukas Lueg (ebfe) Date: 2012-12-24 12:45
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.
msg178054 - (view) Author: Lukas Lueg (ebfe) Date: 2012-12-24 12:46
Attaching a patch to fix all pep8/pyflakes warnings and errors in test_zlib.py
msg178058 - (view) Author: Serhiy Storchaka (serhiy.storchaka) * (Python committer) Date: 2012-12-24 13:39
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.
msg178059 - (view) Author: Lukas Lueg (ebfe) Date: 2012-12-24 14:31
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.
msg178063 - (view) Author: Serhiy Storchaka (serhiy.storchaka) * (Python committer) Date: 2012-12-24 14:58
Sorry, I missed, not decompress(), but compress(). Except this small error all of what I said *is* a problem of this patch.
msg259453 - (view) Author: Martin Panter (martin.panter) * (Python committer) Date: 2016-02-03 03:00
See Issue 8706 discussing adding keyword support to library functions in general.

Functions the first patch affects:

* zlib.compress(data, level=...): still relevant; see Issue 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 Issue 23200

Most of the pep8 patch looks like pointless noise, and a lot of the remaining bits have probably already been addressed.
msg272350 - (view) Author: Stéphane Wirtel (matrixise) * Date: 2016-08-10 17:32
martin or serhiy,

can we move this issue to 3.6 ?
msg272357 - (view) Author: Serhiy Storchaka (serhiy.storchaka) * (Python committer) Date: 2016-08-10 18:19
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.
msg272404 - (view) Author: Xiang Zhang (xiang.zhang) * (Python committer) Date: 2016-08-11 06:58
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.
msg272437 - (view) Author: Martin Panter (martin.panter) * (Python committer) Date: 2016-08-11 11:01
Xiang’s patch looks okay from a correctness point of view
msg272481 - (view) Author: Serhiy Storchaka (serhiy.storchaka) * (Python committer) Date: 2016-08-11 20:23
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.
msg272491 - (view) Author: Xiang Zhang (xiang.zhang) * (Python committer) Date: 2016-08-12 03:14
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.
msg272722 - (view) Author: Serhiy Storchaka (serhiy.storchaka) * (Python committer) Date: 2016-08-15 06:58
LGTM. With issue27574 the overhead is even smaller.
msg272723 - (view) Author: Roundup Robot (python-dev) Date: 2016-08-15 07:06
New changeset a4101218364e by Serhiy Storchaka in branch 'default':
Issue #16764: Support keyword arguments to zlib.decompress().  Patch by
https://hg.python.org/cpython/rev/a4101218364e
msg272731 - (view) Author: Xiang Zhang (xiang.zhang) * (Python committer) Date: 2016-08-15 07:47
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 issue27574.
msg272897 - (view) Author: Xiang Zhang (xiang.zhang) * (Python committer) Date: 2016-08-17 03:03
Serhiy, the message added in Misc/NEWS should be in Library section not Core and Builtins section.
msg272920 - (view) Author: STINNER Victor (haypo) * (Python committer) Date: 2016-08-17 11:48
Oops, I reviewed the patch before seeing that Serhiy already pushed it :-) Ignore my comment.
msg273014 - (view) Author: Roundup Robot (python-dev) Date: 2016-08-18 06:15
New changeset ab0039b8a80e by Serhiy Storchaka in branch 'default':
Issue #16764: Move NEWS entry to correct section and remove too strict test.
https://hg.python.org/cpython/rev/ab0039b8a80e
msg273015 - (view) Author: Serhiy Storchaka (serhiy.storchaka) * (Python committer) Date: 2016-08-18 06:17
> 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!
msg273271 - (view) Author: Martin Panter (martin.panter) * (Python committer) Date: 2016-08-21 06:42
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?
msg273279 - (view) Author: Xiang Zhang (xiang.zhang) * (Python committer) Date: 2016-08-21 08:19
I think it's okay to close now.
History
Date User Action Args
2016-09-04 04:17:30martin.pantersetstatus: open -> closed
resolution: fixed
2016-08-21 08:19:24xiang.zhangsetmessages: + msg273279
2016-08-21 06:42:22martin.pantersetmessages: + msg273271
stage: commit review -> resolved
2016-08-18 06:17:18serhiy.storchakasetmessages: + msg273015
2016-08-18 06:15:23python-devsetmessages: + msg273014
2016-08-17 11:48:50hayposetnosy: + haypo
messages: + msg272920
2016-08-17 03:03:55xiang.zhangsetmessages: + msg272897
2016-08-15 08:55:00serhiy.storchakasetassignee: serhiy.storchaka ->
2016-08-15 07:47:22xiang.zhangsetmessages: + msg272731
2016-08-15 07:06:34python-devsetnosy: + python-dev
messages: + msg272723
2016-08-15 06:58:49serhiy.storchakasetassignee: serhiy.storchaka
messages: + msg272722
stage: patch review -> commit review
2016-08-12 03:14:18xiang.zhangsetmessages: + msg272491
2016-08-11 20:23:56serhiy.storchakasetmessages: + msg272481
2016-08-11 11:01:36martin.pantersetmessages: + msg272437
2016-08-11 06:58:28xiang.zhangsetfiles: + zlib_keyword_argument.patch

messages: + msg272404
2016-08-11 04:11:52xiang.zhangsetnosy: + xiang.zhang
2016-08-10 18:19:02serhiy.storchakasetmessages: + msg272357
versions: + Python 3.6, - Python 3.4
2016-08-10 17:32:55matrixisesetnosy: + matrixise
messages: + msg272350
2016-02-03 03:00:18martin.pantersetnosy: + martin.panter
messages: + msg259453
2012-12-24 14:58:05serhiy.storchakasettype: enhancement
messages: + msg178063
components: + Extension Modules, - Library (Lib)
2012-12-24 14:31:45ebfesettype: enhancement -> (no value)
messages: + msg178059
components: + Library (Lib), - Extension Modules
2012-12-24 13:39:05serhiy.storchakasetnosy: + serhiy.storchaka
messages: + msg178058

components: + Extension Modules, - Library (Lib)
type: enhancement
stage: patch review
2012-12-24 12:54:10ezio.melottisetnosy: + ezio.melotti
2012-12-24 12:46:58ebfesetfiles: + zlib_tests_pep8.patch

messages: + msg178054
2012-12-24 12:45:08ebfecreate