| 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) *  |
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) * |
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) * |
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) * |
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) * |
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) * |
Date: 2016-08-11 11:01 |
| Xiang’s patch looks okay from a correctness point of view |
|
|
| msg272481 - (view) |
Author: Serhiy Storchaka (serhiy.storchaka) * |
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) * |
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) * |
Date: 2016-08-15 06:58 |
| LGTM. With 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) * |
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 . |
|
|
| msg272897 - (view) |
Author: Xiang Zhang (xiang.zhang) * |
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 (vstinner) * |
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) * |
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) * |
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) * |
Date: 2016-08-21 08:19 |
| I think it's okay to close now. |
|
|