Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

inspect.signature: inaccuracies for partial functions #65316

Closed
1st1 opened this issue Mar 31, 2014 · 23 comments
Closed

inspect.signature: inaccuracies for partial functions #65316

1st1 opened this issue Mar 31, 2014 · 23 comments
Assignees
Labels
stdlib Python modules in the Lib dir type-bug An unexpected behavior, bug, or error

Comments

@1st1
Copy link
Member

1st1 commented Mar 31, 2014

BPO 21117
Nosy @brettcannon, @ncoghlan, @larryhastings, @bitdancer, @1st1
Files
  • signature_partial_fix_01.patch
  • Note: these values reflect the state of the issue at the time it was migrated and might not reflect the current state.

    Show more details

    GitHub fields:

    assignee = 'https://github.com/1st1'
    closed_at = <Date 2014-04-08.15:32:44.058>
    created_at = <Date 2014-03-31.20:30:36.388>
    labels = ['type-bug', 'library']
    title = 'inspect.signature: inaccuracies for partial functions'
    updated_at = <Date 2014-04-08.15:32:44.057>
    user = 'https://github.com/1st1'

    bugs.python.org fields:

    activity = <Date 2014-04-08.15:32:44.057>
    actor = 'yselivanov'
    assignee = 'yselivanov'
    closed = True
    closed_date = <Date 2014-04-08.15:32:44.058>
    closer = 'yselivanov'
    components = ['Library (Lib)']
    creation = <Date 2014-03-31.20:30:36.388>
    creator = 'yselivanov'
    dependencies = []
    files = ['34706']
    hgrepos = []
    issue_num = 21117
    keywords = ['patch']
    message_count = 23.0
    messages = ['215265', '215269', '215270', '215271', '215276', '215277', '215286', '215290', '215295', '215317', '215318', '215323', '215324', '215325', '215327', '215328', '215329', '215340', '215341', '215347', '215402', '215527', '215767']
    nosy_count = 6.0
    nosy_names = ['brett.cannon', 'ncoghlan', 'larry', 'r.david.murray', 'python-dev', 'yselivanov']
    pr_nums = []
    priority = 'normal'
    resolution = 'fixed'
    stage = 'needs patch'
    status = 'closed'
    superseder = None
    type = 'behavior'
    url = 'https://bugs.python.org/issue21117'
    versions = ['Python 3.4', 'Python 3.5']

    @1st1
    Copy link
    Member Author

    1st1 commented Mar 31, 2014

    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?

    @1st1 1st1 added the type-feature A feature request or enhancement label Mar 31, 2014
    @1st1 1st1 self-assigned this Mar 31, 2014
    @1st1 1st1 added the stdlib Python modules in the Lib dir label Mar 31, 2014
    @bitdancer
    Copy link
    Member

    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.

    @ncoghlan
    Copy link
    Contributor

    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.

    @ncoghlan
    Copy link
    Contributor

    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.

    @1st1
    Copy link
    Member Author

    1st1 commented Mar 31, 2014

    @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.

    @ncoghlan
    Copy link
    Contributor

    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.

    @1st1
    Copy link
    Member Author

    1st1 commented Apr 1, 2014

    @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.

    @1st1
    Copy link
    Member Author

    1st1 commented Apr 1, 2014

    @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.

    @ncoghlan
    Copy link
    Contributor

    ncoghlan commented Apr 1, 2014

    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.

    @ncoghlan ncoghlan changed the title inspect: PartialSignature and PartialParameter classes inspect.signature: inaccuracies for partial functions Apr 1, 2014
    @ncoghlan ncoghlan added type-bug An unexpected behavior, bug, or error and removed type-feature A feature request or enhancement labels Apr 1, 2014
    @bitdancer
    Copy link
    Member

    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.

    @bitdancer
    Copy link
    Member

    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 :)

    @1st1
    Copy link
    Member Author

    1st1 commented Apr 1, 2014

    @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?

    @1st1
    Copy link
    Member Author

    1st1 commented Apr 1, 2014

    @r. David:

    Yes, thank you, David. Too bad I haven't seen your last messages before I started working on the patch...

    @bitdancer
    Copy link
    Member

    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.

    @1st1
    Copy link
    Member Author

    1st1 commented Apr 1, 2014

    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.

    @1st1
    Copy link
    Member Author

    1st1 commented Apr 1, 2014

    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.

    @bitdancer
    Copy link
    Member

    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.

    @ncoghlan
    Copy link
    Contributor

    ncoghlan commented Apr 1, 2014

    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.

    @ncoghlan
    Copy link
    Contributor

    ncoghlan commented Apr 1, 2014

    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.

    @1st1
    Copy link
    Member Author

    1st1 commented Apr 2, 2014

    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.

    @1st1
    Copy link
    Member Author

    1st1 commented Apr 2, 2014

    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.

    @1st1
    Copy link
    Member Author

    1st1 commented Apr 4, 2014

    Any comments on the patch?

    @python-dev
    Copy link
    Mannequin

    python-dev mannequin commented Apr 8, 2014

    New changeset acbdbf2b06e0 by Yury Selivanov in branch '3.4':
    inspect.signautre: Fix functools.partial support. Issue bpo-21117
    http://hg.python.org/cpython/rev/acbdbf2b06e0

    New changeset 21709cb4a8f5 by Yury Selivanov in branch 'default':
    inspect.signautre: Fix functools.partial support. Issue bpo-21117
    http://hg.python.org/cpython/rev/21709cb4a8f5

    @1st1 1st1 closed this as completed Apr 8, 2014
    @ezio-melotti ezio-melotti transferred this issue from another repository Apr 10, 2022
    Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
    Labels
    stdlib Python modules in the Lib dir type-bug An unexpected behavior, bug, or error
    Projects
    None yet
    Development

    No branches or pull requests

    3 participants