classification
Title: Derby: Convert the zlib, _bz2 and _lzma modules to use Argument Clinic
Type: enhancement Stage: resolved
Components: Extension Modules Versions: Python 3.4
process
Status: closed Resolution: fixed
Dependencies: 20226 20294 Superseder:
Assigned To: serhiy.storchaka Nosy List: larry, meador.inge, nadeem.vawda, python-dev, serhiy.storchaka
Priority: normal Keywords: patch

Created on 2014-01-08 19:03 by serhiy.storchaka, last changed 2014-01-27 00:07 by python-dev. This issue is now closed.

Files
File name Uploaded Description Edit
zlib_clinic.patch serhiy.storchaka, 2014-01-18 11:46 review
bz2_clinic.patch serhiy.storchaka, 2014-01-18 11:47 review
lzma_clinic.patch serhiy.storchaka, 2014-01-18 11:47 review
zlib_clinic.patch serhiy.storchaka, 2014-01-22 12:59 review
bz2_clinic.patch serhiy.storchaka, 2014-01-22 12:59 review
lzma_clinic.patch serhiy.storchaka, 2014-01-22 12:59 review
lzma_clinic.patch serhiy.storchaka, 2014-01-25 10:17 review
zlib_clinic_2.patch serhiy.storchaka, 2014-01-25 12:49 review
zlib_clinic_3.patch serhiy.storchaka, 2014-01-26 12:05 review
Messages (28)
msg207696 - (view) Author: Serhiy Storchaka (serhiy.storchaka) * (Python committer) Date: 2014-01-08 19:03
In progress. These modules have similar interfaces and common maintainer.
msg207698 - (view) Author: Larry Hastings (larry) * (Python committer) Date: 2014-01-08 20:40
I'd prefer it if you use the existing Derby issues.  It's hard enough for me to keep track of what's going on as it is, and it's unhelpful of you to create a redundant issue.  If you'd like some help figuring out how to work with the existing issues please talk to me.  Please close this issue.

zlib is in Derby #13, issue #20182.
_lzma is in Derby #16, issue #20184.
_bz2 is in Derby #17, issue #20185.
msg207704 - (view) Author: Serhiy Storchaka (serhiy.storchaka) * (Python committer) Date: 2014-01-08 21:53
Here are completed patches for the _bz2 and _lzma modules (only constructors 
are not converted) and preliminary patch for the zlib module.

There are issues in the zlib module:

* Argument Clinic generates invalid code for zlib_Decompress_flush 
(issue20196).

* Argument Clinic can simulate optional positional parameters without default 
Python values only using groups, and resulting code differs from original and 
is too cumbersome (see code for zlib_compress, zlib_decompress, 
zlib_Decompress_flush).

* I have experimented in zlib_decompressobj, and I got good compact code, but 
wrong docstring ("decompressobj(wbits=None, zdict=None)"). Needed a way to 
specify optional parameters without default Python values. Correct signature 
should be something like "decompressobj([wbits], [zdict])".

* Previous approach is not applicable to compressobj because Py_buffer can't 
have default value. Seems as compressobj is not compatible with Argument 
Clinic.

* This is not specific to this patch, but pydoc produces wrong outputs for 
compressobj.

Perhaps there are other issues, this is only preliminary patch.

Larry, I don't want to scatter patches for these related modules and merge 
them with patches for totally unrelated modules. And I think it would be 
cumbersome to Nadeem Vawda to make reviews in such circumstances.
msg207707 - (view) Author: Meador Inge (meador.inge) * (Python committer) Date: 2014-01-08 22:30
> Larry, I don't want to scatter patches for these related modules and merge 
> them with patches for totally unrelated modules. And I think it would be 
> cumbersome to Nadeem Vawda to make reviews in such circumstances.

Maybe so, but it will more productive if we can all agree on a method to organize
the work so that we don't duplicate effort and it is clear which issues to contribute
to.  Larry seems to have organized that by sending out multiple mails and opening
issues.  Opening other issues because you don't agree with the way the organizer has
chosen to cut up the work is counterproductive.
msg207709 - (view) Author: Serhiy Storchaka (serhiy.storchaka) * (Python committer) Date: 2014-01-08 22:53
I didn't even know for which of the dozens of random issues Larry randomly 
assigned these modules. And this organization does not look productive.
msg207712 - (view) Author: Larry Hastings (larry) * (Python committer) Date: 2014-01-08 23:39
I did correct that "where is file X" problem; issue #20187 is a meta-issue for the whole Derby, and it has a list of all the issues and what files they map to.  I'm sorry that this is not an optimal solution for organization, but it should be workable, and it only has to last us a week or two, and I felt like I was spending enough time trying to organize the Derby as it was.
msg207713 - (view) Author: Larry Hastings (larry) * (Python committer) Date: 2014-01-08 23:56
> * I have experimented in zlib_decompressobj, and I got good compact
>   code, but wrong docstring ("decompressobj(wbits=None, zdict=None)").
>  Needed a way to specify optional parameters without default Python
>  values. Correct signature should be something like
>  "decompressobj([wbits], [zdict])".

"decompressobj([wbits], [zdict])" isn't valid Python.  "decompressobj(wbits=None, zdict=None)" is; both wbits and zdict are positional-or-keyword parameters with defaults.  If you were writing decompressobj() in Python, and you wanted to simulate its argument parsing as closely as possible, you'd write what Argument Clinic generated.

You shouldn't use a default of "None" for wbits, though (and Argument Clinic shouldn't have let you).  How about_zlib.MAX_WBITS ?
msg207717 - (view) Author: Larry Hastings (larry) * (Python committer) Date: 2014-01-09 00:02
> Previous approach is not applicable to compressobj because
> Py_buffer can't have default value. Seems as compressobj is
> not compatible with Argument Clinic.

Then perhaps we can fix Argument Clinic to be compatible.  Py_buffer doesn't like default values because they mostly don't make sense.  However, I suspect a default value of "None" is fine.  I'll sleep on it and hopefully I can come up with something that will work tomorrow.
msg207720 - (view) Author: Larry Hastings (larry) * (Python committer) Date: 2014-01-09 00:52
I just realized, you're misunderstanding what the first line of the docstring is.

When you run help(function_written_in_Python), the first line of the help is the function's signature rendered in text.  That's generated by "pydoc", which gets signature information from inspect.  Now, it used to be that this didn't work for builtins.  pydoc would look at the function, say "I can't get metadata for this function, I'll skip it".  And it'd just print the builtin's docstring.  Builtins worked around this problem by having their first line *look* like a Python signature.  So the user kind of couldn't tell the difference.

As part of adding Argument Clinic, I changed this.  Now the first line of a docstring is a machine-readable representation of the signature of the function.  Internally it gets split off from the docstring--you won't see it if you look at builtin.__doc__.  Instead this first line is provided via a new property called __text_signature__.  inspect.Signature supports builtins by parsing this string.

As it happens, the format of the string happens to look exactly like a Python function definition.  Because... it is!  inspect.Signature parses the string by passing it in to ast.parse.

"decompressobj([wbits], [zdict])" is not legal Python; if you gave that to ast.parse it would reject it.  (I tried, with PEP 457, but so far nobody has been interested.)  On the other hand, ast.parse is totally happy with "decompressobj(wbits=None, zdict=None)".
msg208396 - (view) Author: Serhiy Storchaka (serhiy.storchaka) * (Python committer) Date: 2014-01-18 11:47
Updated patches use many new Argument Clinic features.

The only issues left:

* pydoc fails with zlib due to the "unspecified" default values (issue20293).

* Constructors for compressor and decompressor objects in the bz2 and lzma 
modules don't use Argument Clinic.
msg208485 - (view) Author: Nadeem Vawda (nadeem.vawda) * (Python committer) Date: 2014-01-19 17:23
The patches for bz2 and lzma look good to me, aside from one nit for lzma.
msg208797 - (view) Author: Serhiy Storchaka (serhiy.storchaka) * (Python committer) Date: 2014-01-22 12:59
Thank you Nadeem for the review.

Here are updated patches. Addressed Nadeem's comments. Compress and decompress 
objects in bz2 and lzma modules now use Argument Clinic for __init__ and to 
generate class docstring. I think these patches are ready to commit (except 
that the name of clinic file will be changed soon).

The zlib module still suffer from a bug #20293.
msg208850 - (view) Author: Nadeem Vawda (nadeem.vawda) * (Python committer) Date: 2014-01-22 20:58
The bz2 patch looks good to me, aside from a nit with the docstring for
BZ2Compressor.__init__.

The lzma patch produces a bunch of test failures for me. It looks like
the __init__ methods for LZMACompressor and LZMADecompressor aren't
accepting keyword args:

    ☿ ./python -c 'import lzma; lzma.LZMACompressor(format=lzma.FORMAT_XZ)'                                                                                                                                                                                                                
    Traceback (most recent call last):
      File "<string>", line 1, in <module>
    TypeError: __init__ does not take keyword arguments

    ☿ ./python -c 'import lzma; lzma.LZMADecompressor(format=lzma.FORMAT_AUTO)'                                                                                                                                                                                                            
    Traceback (most recent call last):
      File "<string>", line 1, in <module>
    TypeError: __init__ does not take keyword arguments
msg209183 - (view) Author: Roundup Robot (python-dev) (Python triager) Date: 2014-01-25 10:08
New changeset 0a3e02251bd6 by Serhiy Storchaka in branch 'default':
Issue #20193: The _bz2 module now uses Argument Clinic.
http://hg.python.org/cpython/rev/0a3e02251bd6
msg209184 - (view) Author: Serhiy Storchaka (serhiy.storchaka) * (Python committer) Date: 2014-01-25 10:17
Thank you Nadeem. In committed patch fixed the docstring for
BZ2Compressor.__init__, and renamed module name "bz2" in clinic declaration is renamed to "_bz2".

Here is updated patch for the _lzma module which addresses Nadeem's comment and renames the "lzma" module name to "_lzma".

Unfortunately this patch is incompatible with current Argument Clinic because it uses "unspecified" default value. Default value of the "check" parameter in LZMACompressor.__init__ depends on the value of other parameter.

"""For FORMAT_XZ, the default is CHECK_CRC64.  FORMAT_ALONE and FORMAT_RAW do not suport integrity checks; for these formats, check must be omitted, or be CHECK_NONE."""
msg209189 - (view) Author: Roundup Robot (python-dev) (Python triager) Date: 2014-01-25 12:02
New changeset 7ba9642fc800 by Serhiy Storchaka in branch 'default':
Issue #20193: The _lzma module now uses Argument Clinic.
http://hg.python.org/cpython/rev/7ba9642fc800
msg209191 - (view) Author: Serhiy Storchaka (serhiy.storchaka) * (Python committer) Date: 2014-01-25 12:34
Patch for _lzma was applied without converting LZMACompressor.__init__.

Here is a patch for zlib, which doesn't use neither optional groups, nor "unspecified" defaults.  Three builtins, decompress(), decompressobj() and Decompress.flush() are left not converted.
msg209194 - (view) Author: Serhiy Storchaka (serhiy.storchaka) * (Python committer) Date: 2014-01-25 12:49
Sorry, previous patch didn't contain generated file.
msg209242 - (view) Author: Serhiy Storchaka (serhiy.storchaka) * (Python committer) Date: 2014-01-25 21:57
Updated patch for zlib reverts reverted decompress(), decompressobj() and Decompress.flush(). It get rids from DEF_WBITS and use MAX_WBITS instead, exposes DEF_BUF_SIZE (former DEFAULTALLOC) to Python level.
msg209243 - (view) Author: Serhiy Storchaka (serhiy.storchaka) * (Python committer) Date: 2014-01-25 21:59
I suppose that zdict=b'' have same effect as not specifying zdict. Am I right?
msg209287 - (view) Author: Serhiy Storchaka (serhiy.storchaka) * (Python committer) Date: 2014-01-26 08:07
Synchronized with recent Argument Clinic changes.
msg209294 - (view) Author: Nadeem Vawda (nadeem.vawda) * (Python committer) Date: 2014-01-26 11:28
The latest patch for zlib seems to be missing Modules/zlibmodule.clinic.c

> I suppose that zdict=b'' have same effect as not specifying zdict. Am I right?

Probably, but to be on the safe side I'd prefer that we preserve the behavior of
not calling deflateSetDictionary/inflateSetDictionary unless the caller
explicitly provides zdict. If you need to give a Python default value, rather
use None than b''.
msg209296 - (view) Author: Larry Hastings (larry) * (Python committer) Date: 2014-01-26 11:32
We're not allowing changes in semantics for Argument Clinic conversion for 3.4.  If it doesn't currently accept None, we can't add it right now, and we'll have to save it for 3.5.
msg209298 - (view) Author: Serhiy Storchaka (serhiy.storchaka) * (Python committer) Date: 2014-01-26 12:05
Ah, sorry, here is full patch. Generated file is now named 
Modules/clinic/zlibmodule.c.h.

The behavior is preserved. This case is exact analogue of _sha1.sha1(). No one 
additional function called when the parameter is not specified, but if it is 
specified as b'', the function behaves identically to not passing in that 
parameter.
msg209349 - (view) Author: Nadeem Vawda (nadeem.vawda) * (Python committer) Date: 2014-01-26 20:13
The patch for zlib looks good to me. Thanks for working on this, Serhiy.


> We're not allowing changes in semantics for Argument Clinic conversion for 3.4. If it doesn't currently accept None, we can't add it right now, and we'll have to save it for 3.5.

Fair enough.


> The behavior is preserved. This case is exact analogue of _sha1.sha1(). No one 
> additional function called when the parameter is not specified, but if it is
> specified as b'', the function behaves identically to not passing in that
> parameter.

Ah OK, I misunderstood the Argument Clinic input code when I first read it.
Having actually read the docs, it makes sense.
msg209356 - (view) Author: Roundup Robot (python-dev) (Python triager) Date: 2014-01-26 22:04
New changeset 6f217456b9ba by Serhiy Storchaka in branch 'default':
Issue #20193: The zlib module now uses Argument Clinic.
http://hg.python.org/cpython/rev/6f217456b9ba
msg209357 - (view) Author: Serhiy Storchaka (serhiy.storchaka) * (Python committer) Date: 2014-01-26 22:05
Thank you for reviews, Nadeem, Larry.
msg209370 - (view) Author: Roundup Robot (python-dev) (Python triager) Date: 2014-01-27 00:07
New changeset c2e425a05d35 by Christian Heimes in branch 'default':
Issue #20193: Fix commit r6f217456b9ba by including clinic/zlibmodule.c.h instead
http://hg.python.org/cpython/rev/c2e425a05d35
History
Date User Action Args
2014-01-27 00:07:06python-devsetmessages: + msg209370
2014-01-26 22:05:28serhiy.storchakasetstatus: open -> closed
messages: + msg209357

dependencies: - pydoc fails with the "unspecified" default value
resolution: fixed
stage: patch review -> resolved
2014-01-26 22:04:02python-devsetmessages: + msg209356
2014-01-26 20:13:11nadeem.vawdasetmessages: + msg209349
2014-01-26 12:07:39serhiy.storchakasetfiles: - zlib_clinic_3.patch
2014-01-26 12:05:23serhiy.storchakasetfiles: + zlib_clinic_3.patch

messages: + msg209298
2014-01-26 11:32:02larrysetmessages: + msg209296
2014-01-26 11:28:26nadeem.vawdasetmessages: + msg209294
2014-01-26 08:07:11serhiy.storchakasetfiles: + zlib_clinic_3.patch

messages: + msg209287
2014-01-26 08:06:20serhiy.storchakasetfiles: - zlib_clinic_3.patch
2014-01-25 21:59:06serhiy.storchakasetmessages: + msg209243
2014-01-25 21:57:04serhiy.storchakasetfiles: + zlib_clinic_3.patch

messages: + msg209242
2014-01-25 12:49:34serhiy.storchakasetfiles: + zlib_clinic_2.patch

messages: + msg209194
2014-01-25 12:48:44serhiy.storchakasetfiles: - zlib_clinic_2.patch
2014-01-25 12:34:04serhiy.storchakasetfiles: + zlib_clinic_2.patch

messages: + msg209191
stage: patch review
2014-01-25 12:02:36python-devsetmessages: + msg209189
2014-01-25 10:17:35serhiy.storchakasetfiles: + lzma_clinic.patch

messages: + msg209184
2014-01-25 10:08:02python-devsetnosy: + python-dev
messages: + msg209183
2014-01-22 20:58:18nadeem.vawdasetmessages: + msg208850
2014-01-22 12:59:10serhiy.storchakasetfiles: + zlib_clinic.patch, bz2_clinic.patch, lzma_clinic.patch

messages: + msg208797
2014-01-19 17:23:38nadeem.vawdasetmessages: + msg208485
2014-01-18 11:48:24serhiy.storchakasetfiles: - zlib_clinic.patch
2014-01-18 11:48:16serhiy.storchakasetfiles: - lzma_clinic.patch
2014-01-18 11:48:04serhiy.storchakasetfiles: - bz2_clinic.patch
2014-01-18 11:47:48serhiy.storchakasetdependencies: + pydoc fails with the "unspecified" default value, Argument Clinic: add support for __init__
2014-01-18 11:47:03serhiy.storchakasetfiles: + zlib_clinic.patch, bz2_clinic.patch, lzma_clinic.patch

messages: + msg208396
2014-01-15 20:47:31serhiy.storchakasetdependencies: + Argument Clinic: support for simple expressions?
2014-01-09 00:52:46larrysetmessages: + msg207720
2014-01-09 00:02:14larrysetmessages: + msg207717
2014-01-08 23:56:05larrysetmessages: + msg207713
2014-01-08 23:39:30larrysetmessages: + msg207712
2014-01-08 22:53:14serhiy.storchakasetmessages: + msg207709
2014-01-08 22:30:12meador.ingesetnosy: + meador.inge
messages: + msg207707
2014-01-08 21:54:14serhiy.storchakasetfiles: + zlib_clinic.patch
2014-01-08 21:53:53serhiy.storchakasetfiles: + lzma_clinic.patch
2014-01-08 21:53:43serhiy.storchakasetfiles: + bz2_clinic.patch
keywords: + patch
2014-01-08 21:53:07serhiy.storchakasetmessages: + msg207704
2014-01-08 20:40:00larrysetmessages: + msg207698
2014-01-08 19:03:44serhiy.storchakasetnosy: + larry
2014-01-08 19:03:31serhiy.storchakacreate