classification
Title: inspect.signature: inaccuracies for partial functions
Type: behavior Stage: needs patch
Components: Library (Lib) Versions: Python 3.5, Python 3.4
process
Status: closed Resolution: fixed
Dependencies: Superseder:
Assigned To: yselivanov Nosy List: brett.cannon, larry, ncoghlan, python-dev, r.david.murray, yselivanov
Priority: normal Keywords: patch

Created on 2014-03-31 20:30 by yselivanov, last changed 2014-04-08 15:32 by yselivanov. This issue is now closed.

Files
File name Uploaded Description Edit
signature_partial_fix_01.patch yselivanov, 2014-04-02 21:08 review
Messages (23)
msg215265 - (view) Author: Yury Selivanov (yselivanov) * (Python committer) Date: 2014-03-31 20:30
There is a small detail in the current Signature class implementation, in regards to how partial signatures are treated. Consider the following example:

   def foo(a, b): pass
   foo_partial = functools.partial(foo, 'spam')

Now, the signature of 'foo_partial' is '(a="spam", b)', which (strictly speaking) is not a valid python function signature. For cases like that, we have a private "Parameter._partial_kwarg" attribute. When this attribute is set to True, it means, that this parameter's default came from partial (or alike) function. Parameter instances with '_partial_kwarg=True' are treated a bit differently during signature validation and binding.

A small and nasty detail: Parameter.__eq__ ignores value of '_partial_kwarg'. Because, for the following code:

   def bar(a, b=10): pass
   def baz(a, b): pass
   baz2 = functools.partial(baz, b=10)

signature of 'bar' is equal to 'baz2'.

In light of making signatures hashable, the obvious question was raised: should __hash__ account for '_partial_kwarg' value or not.  I think it should.  But in this case, it should be really obvious, if parameter was modified by partial or not.

Hence, I propose to add two more classes:

- PartialSignature(Signature)
- PartialParameter(Parameter)

Results of signature(functools.partial(..)) and Signature.bind_partial(..) will be instances of PartialSignature. It will be OK if some PartialSignature is equal to Signature, but they will have different __hash__es.

What do you think?
msg215269 - (view) Author: R. David Murray (r.david.murray) * (Python committer) Date: 2014-03-31 21:28
I believe it is a python invariant that a == b implies hash(a) == hash(b).  I don't see why the two signatures should be equal.  I'm not even sure why the bound argument shows up in the signature of the partial.  That surprises me.
msg215270 - (view) Author: Nick Coghlan (ncoghlan) * (Python committer) Date: 2014-03-31 21:36
The already bound arguments should be treated as additional keyword-only
arguments, and already bound positional arguments hidden completely.

I'm +0 on new types to clean that up if necessary, but would prefer it if
we could just improve the translation to ordinary signature objects instead.
msg215271 - (view) Author: Nick Coghlan (ncoghlan) * (Python committer) Date: 2014-03-31 21:37
On 1 Apr 2014 07:36, "Nick Coghlan" <report@bugs.python.org> wrote:
>
>
> Nick Coghlan added the comment:
>
> The already bound arguments should be treated as additional keyword-only
> arguments, and already bound positional arguments hidden completely.

Oops: already bound positional-*only* arguments should be hidden.
msg215276 - (view) Author: Yury Selivanov (yselivanov) * (Python committer) Date: 2014-03-31 22:17
@Nick:

> Oops: already bound positional-*only* arguments should be hidden.
Hm, good catch. I'm not sure we currently do this. I'll check if this needs to be fixed (in 3.4.1 too).

> I'm +0 on new types to clean that up if necessary, but would prefer it if
we could just improve the translation to ordinary signature objects instead.

I'm not sure I understand what you mean by "translation to ordinary signature objects". Could you please elaborate on this?


@R. David:
> I believe it is a python invariant that a == b implies hash(a) == hash(b).  

I think that 'hash(a) == hash(b)' means that 'a == b' (strongly). But not reverse.  Or am I wrong?

> I don't see why the two signatures should be equal. I'm not even sure why the bound argument shows up in the signature of the partial.

Well, why shouldn't they be equal? They have same parameters, same default values. Quoting pep 362: """...two signatures are equal only when their corresponding parameters are equal and have the exact same names...""". Moreover, this behaviour is implemented since 3.3.
 
But their hashes shouldn't be equal, that's something I can agree on.
msg215277 - (view) Author: Nick Coghlan (ncoghlan) * (Python committer) Date: 2014-03-31 22:29
On 1 Apr 2014 08:17, "Yury Selivanov" <report@bugs.python.org> wrote:
>
> > I'm +0 on new types to clean that up if necessary, but would prefer it
if
> we could just improve the translation to ordinary signature objects
instead.
>
> I'm not sure I understand what you mean by "translation to ordinary
signature objects". Could you please elaborate on this?

If arguments bound by position disappear from the signature, and those
bound by keyword are mapped to keyword-only parameters with a default, we
should get a valid and accurate signature. That's a 3.4.1 bug fix, rather
than a 3.5 feature.
msg215286 - (view) Author: Yury Selivanov (yselivanov) * (Python committer) Date: 2014-04-01 00:01
@Nick:

Agreed on positional-only stuff.

> If arguments bound by position disappear from the signature, and those
> bound by keyword are mapped to keyword-only parameters with a default, we
> should get a valid and accurate signature.

But what about my example from the first message:

   def foo(a, b): pass
   foo_partial = functools.partial(foo, 'spam')

'foo_partial' now has the following signature: (a='spam', b); where 'a' is a positional_or_keyword parameter, i.e. you can still do 'foo_partial(10, 20)', or 'foo_partial(b=20, a=10)'.

(a='spam',  b) is not a valid python pure function signature, but is a perfectly valid signature of partial function. And since its arguments aren't positional-only, we shouldn't hide anything.

That's why I have this private '_partial_kwarg' attribute, which I don't like and want to remove by adding a PartialParameter subclass. Otherwise, we have something that is hidden and non-documented, but affects Parameter.__hash__ and some other logic.
msg215290 - (view) Author: Yury Selivanov (yselivanov) * (Python committer) Date: 2014-04-01 01:53
@Nick: oh, it took me some time to realize that your suggestion to transform positional-or-keyword to keyword-only parameters is correct. If we do this, we no longer need '_partial_kwarg' hack. I'll work on the patch. Thank you.
msg215295 - (view) Author: Nick Coghlan (ncoghlan) * (Python committer) Date: 2014-04-01 05:38
I believe Yury already figured out what I meant, but to make it entirely clear, after the change, this example:

   def foo(a, b): pass
   foo_partial = functools.partial(foo, 'spam')
   foo_partial2 = functools.partial(foo, a='spam')

Should lead to the following signature for both "foo_partial" and "foo_partial2":

    (b, *, a='spam')

That accurately indicates that "a" is now effectively a keyword only parameter - the first supplied positional argument will map to "b", and attempting to supply *two* positional arguments will fail.

Correctly handing *args may get a little interesting, but should be feasible.
msg215317 - (view) Author: R. David Murray (r.david.murray) * (Python committer) Date: 2014-04-01 13:59
OK, I didn't even realize that was possible with partial.  Now I understand Yuri's original point.  His example is wrong:

>>> def foo(a, b):
...    print(a, b)
>>> x2 = partial(foo, 'x')
>>> str(inspect.signature(x2))
'(b)'

This is the correct example:

>>> x = partial(foo, a='x')
>>> x('b')
Traceback (most recent call last):
  File "<stdin>", line 1, in <module>
TypeError: foo() got multiple values for argument 'a'

The current signature for this is the one Yuri gave:

>>> str(inspect.signature(x))
"(a='x', b)"

Which is about as close as one can come to the rather non-intuitve (non-pythonic?) behavior that partial has here.  Perhaps this a bug in partial?  If so it is unfortunately one with ugly backward compatibility implications.
msg215318 - (view) Author: R. David Murray (r.david.murray) * (Python committer) Date: 2014-04-01 14:02
By "didn't know that was possible", I mean binding a positional argument as a keyword argument in the partial.  If nobody else thought that was possible, maybe can just fix it :)
msg215323 - (view) Author: Yury Selivanov (yselivanov) * (Python committer) Date: 2014-04-01 15:14
@Nick:

Ouch... I'm halfway through the implementation, and it seems like your idea isn't going to work.

Example (from unittest):

    def foo(a=1, b=2, c=3): pass
    _foo = partial(foo, a=10, c=13)

Now, the signature for "_foo", with your logic applied, will be:

    (b=2, *, a=10, c=13)

If, however, you try to do the following call:

    _foo(11)

It will fail with a TypeError "got multiple values for argument 'a'", because 'partial' will actually do this call:

    foo(11, a=10, c=13)

I now remember this obstacle, that's why I have '_partial_kwarg'. So unfortunately, why I really like your idea, I don't think we can make it work.

Now, I still want to get rid the '_partial_kwarg' attribute. Are you guys OK, if I introduce PartialParameter & PartialSignature classes?
msg215324 - (view) Author: Yury Selivanov (yselivanov) * (Python committer) Date: 2014-04-01 15:16
@R. David:

Yes, thank you, David. Too bad I haven't seen your last messages before I started working on the patch...
msg215325 - (view) Author: R. David Murray (r.david.murray) * (Python committer) Date: 2014-04-01 15:32
We'll have to wait for Nick to chime in, but I'll make a couple of comments.  

First, I think this is a bug in partial, so I think we need to decide what, if anything, to do about that first, before we decide if signature needs to compensate for it or not.  

The other comment is about ==/__hash__, in case that is involved in the solution: two objects may very well have the same __hash__ and *not* be equal, but they cannot have different __hash__es and *be* equal.  The equality comparison algorithm uses __hash__ as a shortcut.  If two objects have different hashes, they are not equal and we return False.  If they have the same hash, then a full equality comparison is done before the result of __eq__ is returned.  You will understand that this must be the case if you think about the nature of hashing: it throws away information.  Cryptographic hashes used for identification are constructed such that the *probability* of a collision is very low, but it is still not zero, so how they are used is just as important as how they work.  Our __hash__es are generally not that strong, so collision is likely.
msg215327 - (view) Author: Yury Selivanov (yselivanov) * (Python committer) Date: 2014-04-01 15:50
> First, I think this is a bug in partial, so I think we need to decide what, if anything, to do about that first, before we decide if signature needs to compensate for it or not.

Agree. Although, it looks like it's not something that partial is doing, it seems like this call logic is implemented somewhere way deeper.

> The other comment is about ==/__hash__ [...]
Agree.
msg215328 - (view) Author: Yury Selivanov (yselivanov) * (Python committer) Date: 2014-04-01 15:55
> Although, it looks like it's not something that partial is doing, it seems like this call logic is implemented somewhere way deeper.

Forget about what I said.

Yes, it's a "bug" in partial. Fixing it, will require having the code from "Signature.bind" reflected to C -- IOW you need to introspect the callable, get information about its parameters, and then compute the actual args & kwargs. Which will make functools.partial much much slower.
msg215329 - (view) Author: R. David Murray (r.david.murray) * (Python committer) Date: 2014-04-01 15:58
Oh, the error message comes from deep in the guts of python, yes.  I'm saying that the fact that partial lets you write partial(foo, a='bar') when a is a positional argument is a bug.  Even if other people agree with me (and they may not, "consenting adults" and all that), it may be a bug we are stuck with.
msg215340 - (view) Author: Nick Coghlan (ncoghlan) * (Python committer) Date: 2014-04-01 21:34
Huh, that actually sounds like a possible design flaw in the core argument binding semantics. I'll have to think about that one some more.

In the meantime, as far as this issue goes, I'm inclined to say that signature should throw an exception to be clear that such a malformed object has *no* usable signature, as attempting to call it will always fail.
msg215341 - (view) Author: Nick Coghlan (ncoghlan) * (Python committer) Date: 2014-04-01 21:36
Oh, wait, it *does* have a usable signature. It's just that all the *subsequent* positional-or-keyword parameters also have to be marked as keyword-only.
msg215347 - (view) Author: Yury Selivanov (yselivanov) * (Python committer) Date: 2014-04-02 00:02
> Oh, wait, it *does* have a usable signature. It's just that all the *subsequent* positional-or-keyword parameters also have to be marked as keyword-only.

Interesting idea. I'll incorporate this logic into the patch.
msg215402 - (view) Author: Yury Selivanov (yselivanov) * (Python committer) Date: 2014-04-02 21:08
Please review the attached patch.

Here's the new partial signature semantics:

   foo(a, b, /, c, d, *args, e)

   partial(foo, 10) -> (b, /, c, d, *args, e)
   partial(foo, 10, c=11) -> (b, /, *, c=11, d, e)
   partial(foo, 10, 20, 30) -> (d, *args, e)
   partial(foo, 10, 20, 30, 40, 50) -> (*args, e)
   partial(foo, 10, 20, c=20) -> (*, c=20, d, e)

Good news: 

1. no more special attributes and other hidden hacks.

2. only with this patch we properly support functools.partial. So this is definitely something we can classify as a bug fix and push in 3.4.1.
msg215527 - (view) Author: Yury Selivanov (yselivanov) * (Python committer) Date: 2014-04-04 14:41
Any comments on the patch?
msg215767 - (view) Author: Roundup Robot (python-dev) Date: 2014-04-08 15:31
New changeset acbdbf2b06e0 by Yury Selivanov in branch '3.4':
inspect.signautre: Fix functools.partial support. Issue #21117
http://hg.python.org/cpython/rev/acbdbf2b06e0

New changeset 21709cb4a8f5 by Yury Selivanov in branch 'default':
inspect.signautre: Fix functools.partial support. Issue #21117
http://hg.python.org/cpython/rev/21709cb4a8f5
History
Date User Action Args
2014-04-08 15:32:44yselivanovsetstatus: open -> closed
resolution: fixed
2014-04-08 15:31:10python-devsetnosy: + python-dev
messages: + msg215767
2014-04-04 14:41:44yselivanovsetmessages: + msg215527
2014-04-02 21:08:52yselivanovsetfiles: + signature_partial_fix_01.patch
keywords: + patch
messages: + msg215402
2014-04-02 00:02:19yselivanovsetmessages: + msg215347
2014-04-01 21:36:22ncoghlansetmessages: + msg215341
2014-04-01 21:34:48ncoghlansetmessages: + msg215340
2014-04-01 15:58:35r.david.murraysetmessages: + msg215329
2014-04-01 15:55:29yselivanovsetmessages: + msg215328
2014-04-01 15:50:20yselivanovsetmessages: + msg215327
2014-04-01 15:32:41r.david.murraysetmessages: + msg215325
2014-04-01 15:16:04yselivanovsetmessages: + msg215324
2014-04-01 15:14:32yselivanovsetmessages: + msg215323
2014-04-01 14:02:37r.david.murraysetmessages: + msg215318
2014-04-01 13:59:25r.david.murraysetmessages: + msg215317
2014-04-01 05:38:29ncoghlansetversions: + Python 3.4
title: inspect: PartialSignature and PartialParameter classes -> inspect.signature: inaccuracies for partial functions
messages: + msg215295

type: enhancement -> behavior
stage: needs patch
2014-04-01 01:53:16yselivanovsetmessages: + msg215290
2014-04-01 00:01:43yselivanovsetmessages: + msg215286
2014-03-31 22:29:52ncoghlansetmessages: + msg215277
2014-03-31 22:17:14yselivanovsetmessages: + msg215276
2014-03-31 21:37:36ncoghlansetmessages: + msg215271
2014-03-31 21:36:08ncoghlansetmessages: + msg215270
2014-03-31 21:28:58r.david.murraysetnosy: + r.david.murray
messages: + msg215269
2014-03-31 20:30:36yselivanovcreate