Issue21117
This issue tracker has been migrated to GitHub,
and is currently read-only.
For more information,
see the GitHub FAQs in the Python's Developer Guide.
Created on 2014-03-31 20:30 by yselivanov, last changed 2022-04-11 14:58 by admin. 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) * | 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) * | 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: Alyssa Coghlan (ncoghlan) * | 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: Alyssa Coghlan (ncoghlan) * | 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) * | 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: Alyssa Coghlan (ncoghlan) * | 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) * | 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) * | 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: Alyssa Coghlan (ncoghlan) * | 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) * | 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) * | 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) * | 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) * | 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) * | 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) * | 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) * | 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) * | 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: Alyssa Coghlan (ncoghlan) * | 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: Alyssa Coghlan (ncoghlan) * | 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) * | 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) * | 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) * | 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 |
2022-04-11 14:58:01 | admin | set | github: 65316 |
2014-04-08 15:32:44 | yselivanov | set | status: open -> closed resolution: fixed |
2014-04-08 15:31:10 | python-dev | set | nosy:
+ python-dev messages: + msg215767 |
2014-04-04 14:41:44 | yselivanov | set | messages: + msg215527 |
2014-04-02 21:08:52 | yselivanov | set | files:
+ signature_partial_fix_01.patch keywords: + patch messages: + msg215402 |
2014-04-02 00:02:19 | yselivanov | set | messages: + msg215347 |
2014-04-01 21:36:22 | ncoghlan | set | messages: + msg215341 |
2014-04-01 21:34:48 | ncoghlan | set | messages: + msg215340 |
2014-04-01 15:58:35 | r.david.murray | set | messages: + msg215329 |
2014-04-01 15:55:29 | yselivanov | set | messages: + msg215328 |
2014-04-01 15:50:20 | yselivanov | set | messages: + msg215327 |
2014-04-01 15:32:41 | r.david.murray | set | messages: + msg215325 |
2014-04-01 15:16:04 | yselivanov | set | messages: + msg215324 |
2014-04-01 15:14:32 | yselivanov | set | messages: + msg215323 |
2014-04-01 14:02:37 | r.david.murray | set | messages: + msg215318 |
2014-04-01 13:59:25 | r.david.murray | set | messages: + msg215317 |
2014-04-01 05:38:29 | ncoghlan | set | versions:
+ 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:16 | yselivanov | set | messages: + msg215290 |
2014-04-01 00:01:43 | yselivanov | set | messages: + msg215286 |
2014-03-31 22:29:52 | ncoghlan | set | messages: + msg215277 |
2014-03-31 22:17:14 | yselivanov | set | messages: + msg215276 |
2014-03-31 21:37:36 | ncoghlan | set | messages: + msg215271 |
2014-03-31 21:36:08 | ncoghlan | set | messages: + msg215270 |
2014-03-31 21:28:58 | r.david.murray | set | nosy:
+ r.david.murray messages: + msg215269 |
2014-03-31 20:30:36 | yselivanov | create |