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

Changes in the inspect module for PEP 570 #80932

Closed
pablogsal opened this issue Apr 29, 2019 · 30 comments
Closed

Changes in the inspect module for PEP 570 #80932

pablogsal opened this issue Apr 29, 2019 · 30 comments
Labels
3.8 only security fixes interpreter-core (Objects, Python, Grammar, and Parser dirs)

Comments

@pablogsal
Copy link
Member

BPO 36751
Nosy @brettcannon, @ncoghlan, @vstinner, @ambv, @serhiy-storchaka, @zooba, @hroncok, @pablogsal
PRs
  • bpo-36751: Deprecate getfullargspec and report positional-only args as regular args #13016
  • bpo-36751: Undeprecate getfullargspec #13245
  • 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 = None
    closed_at = <Date 2019-05-17.11:23:42.943>
    created_at = <Date 2019-04-29.12:13:17.185>
    labels = ['interpreter-core', '3.8']
    title = 'Changes in the inspect module for PEP 570'
    updated_at = <Date 2019-05-22.12:21:34.359>
    user = 'https://github.com/pablogsal'

    bugs.python.org fields:

    activity = <Date 2019-05-22.12:21:34.359>
    actor = 'ncoghlan'
    assignee = 'none'
    closed = True
    closed_date = <Date 2019-05-17.11:23:42.943>
    closer = 'pablogsal'
    components = ['Interpreter Core']
    creation = <Date 2019-04-29.12:13:17.185>
    creator = 'pablogsal'
    dependencies = []
    files = []
    hgrepos = []
    issue_num = 36751
    keywords = ['patch', '3.7regression']
    message_count = 30.0
    messages = ['341070', '341075', '341081', '341082', '341083', '341098', '341101', '341102', '341104', '341115', '341124', '341128', '341130', '342156', '342157', '342158', '342197', '342295', '342296', '342362', '342426', '342666', '342667', '342669', '342670', '342679', '342683', '342685', '342698', '343187']
    nosy_count = 9.0
    nosy_names = ['brett.cannon', 'ncoghlan', 'vstinner', 'zzzeek', 'lukasz.langa', 'serhiy.storchaka', 'steve.dower', 'hroncok', 'pablogsal']
    pr_nums = ['13016', '13245']
    priority = 'normal'
    resolution = 'fixed'
    stage = 'resolved'
    status = 'closed'
    superseder = None
    type = None
    url = 'https://bugs.python.org/issue36751'
    versions = ['Python 3.8']

    @pablogsal
    Copy link
    Member Author

    This issue is to discuss how to handle the changes in the inspect module for PEP-570. In particular, how to handle:

    • getfullargspec
    • formatargspec

    for positional-only parameters.

    @pablogsal pablogsal added 3.8 only security fixes interpreter-core (Objects, Python, Grammar, and Parser dirs) labels Apr 29, 2019
    @vstinner
    Copy link
    Member

    Context: see #12701 discussion.

    @zooba
    Copy link
    Member

    zooba commented Apr 29, 2019

    Marking as a release blocker and adding RM. The 3.8 branch now contains breaking API changes to these functions that we have to resolve before the next release (or revert while deciding how to handle the change) - PR 12701 has been merged.

    @zooba
    Copy link
    Member

    zooba commented Apr 29, 2019

    My bad, there were PR changes that GitHub didn't show me. As you were

    @zooba
    Copy link
    Member

    zooba commented Apr 29, 2019

    Nope, I was right the first time. The FullArgSpec tulle has changed indexes, and formatargspec has additional (undocumented) arguments.

    Since formatargspec is deprecated, it should probably just not change.

    The FullArgSpec tuple might have to become a concrete class implementation so that it can have positional-only args as a named attribute but not part of unpacking.

    As for where the new information *can* go, I suspect the Signature object is the only place. Though that has limitations, and for C APIs in particular much of my code only uses the more reliable getfullargspec.

    @brettcannon
    Copy link
    Member

    Steve's right that we can't change the indexes on those functions for pre-existing values, so appending to the end is the only real option in that case. I still think they should both be deprecated due to how limiting their APIs are.

    Obviously signature() can be updated cleanly.

    @vstinner
    Copy link
    Member

    IMHO getargs(), getargspec(), getfullargspec() should be deprecated in favor of signature() which is future-proof. But Nick Coghlan undeprecated these functions in 2017 https://bugs.python.org/issue20438 whereas these were deprecated since 2015. I didn't understand the subtle issues. That's why I asked Pablo to open a separated issue, to get Nick and others involved to get the context.

    My proposal was to raise an exception if the input function has positonal arguments. There is good way to fix these legacy functions, their design cannot be extended, whereas signature() is fine.

    @zooba
    Copy link
    Member

    zooba commented Apr 29, 2019

    My proposal was to raise an exception if the input function has positonal arguments

    Counter-proposal - just report them as regular position-or-name arguments.

    Users of inspect are most likely to be IDEs or editors getting information to present to users. And in my experience, most just ignore all exceptions because bugs cause some function objects to raise errors when trying to inspect them. Which means a new exception will just result in a worse experience for users and no indication of what's wrong.

    On the other hand, if there is no change made, they'll just report a normal looking signature until they migrate to using updated Signature objects. Anyone using current Signature objects should similarly get the argument count right, even if they totally ignore the positional-only count. Since one of the arguments for supporting positional-only parameters is that they're for parameters that you'd never consider passing by name, but these tools still need to show a name, very few people should ever be tripped up by them being misreported. And when they do they'll be tripped up very quickly.

    Basically, this is not a scenario that *requires* existing users to be broken. So we should avoid breaking them at all costs.

    @brettcannon
    Copy link
    Member

    If I remember correctly the un-deprecation was because 2/3 code could only use getfullargspec() to get all relevant details. Since we're so close to being done w/ Python 2 I think this might be the last change to make to them, document their deprecation, tack this info on to the end of the tuple, and then start raising DeprecationWarning in 3.9.

    @ambv
    Copy link
    Contributor

    ambv commented Apr 29, 2019

    I'm with Steve on this one. Report positional-only args as regular arguments in the old functions.

    @pablogsal
    Copy link
    Member Author

    PR 13016 adds the positional-only arguments together with the regular arguments as suggested by Steve and Łukasz and deprecates getfullargspec() in favour of inspect.signature.

    @pablogsal
    Copy link
    Member Author

    New changeset d5d2b45 by Pablo Galindo in branch 'master':
    bpo-36751: Deprecate getfullargspec and report positional-only args as regular args (GH-13016)
    d5d2b45

    @pablogsal
    Copy link
    Member Author

    PR 13016 is merged, and I will remove the release blocker, but I will leave it open for now until we can check if there is consensus on the status and future of the inspect module after PEP-570 :)

    @ncoghlan
    Copy link
    Contributor

    This PR needs to be reverted - we previously deprecated this API, but then undeprecated it again in bpo-27172, as the consequence of deprecating it was projects copying & pasting the getfullargspec() implementation into their own code, not switching to inspect.Signature.

    @ncoghlan
    Copy link
    Contributor

    And no, the undeprecation wasn't because of Python 2 (Py2 doesn't have getfullargspec() - it's a Py3 only API).

    The undeprecation was because there are a lot of 3rd party projects for whom the getfullargspec() representation is good enough, and switching to inspect.Signature instead requires a significant rewrite vs just wrapping inspect.Signature to produce getfullargspec() style output.

    So getfullargspec() doesn't need to change at all for PEP-570 and should instead keep handling positional only arguments the same way it has since it was switched over to being based on inspect.Signature: reporting them the same way as positional-or-keyword parameters.

    @ncoghlan
    Copy link
    Contributor

    Also note bpo-32190 regarding changing the way these APIs are documented, without introducing any programmatic deprecation warnings.

    @pablogsal
    Copy link
    Member Author

    Opened PR 13245 to un-deprecate getfullargspec.

    @hroncok
    Copy link
    Mannequin

    hroncok mannequin commented May 13, 2019

    Just a quick idea: What if a warning happened iff positional only argument are there, but not with functions that don't use those? Might be a different kind of warning.

    @vstinner
    Copy link
    Member

    And no, the undeprecation wasn't because of Python 2 (Py2 doesn't have getfullargspec() - it's a Py3 only API).

    Python 3.8.0 is scheduled for: "3.8.0 final: Monday, 2019-10-21"

    Something like 2 months before 2.7 end of support: 2019-12-31.

    Does the "because of Python 2" still stand for Python 3.8?

    A deprecating warning doesn't hurt: you are still able to run your code. Moreover, deprecating warnings are ignored by default (except in the __main__ module). I don't see what is the *practical* issue of deprecating getfullargspec().

    getfullargspec() has to die sometimes, it's good to warn users, no :-)

    Or is the *long term plan* to keep getfullargspec() forever?

    --

    I basically have no opinion on these questions. I'm more curious about the long term plan ;-)

    @zzzeek
    Copy link
    Mannequin

    zzzeek mannequin commented May 13, 2019

    if a function continues to work correctly throughout the span of a Python X version, why does it need to be removed? I have a foggy memory but I don't seem to recall functions that were technically redundant being aggressively deprecated in the 2.x series. This warning is causing a lot of downstream pain right now and this is after we already all had to deal with getargspec() going away.

    @brettcannon
    Copy link
    Member

    RE: "if a function continues to work correctly throughout the span of a Python X version, why does it need to be removed?"

    In this case the argument is whether inspect.getfullargspec() works correctly in the face of positional-only parameters. There's also the usual maintenance burden like any other open source project has.

    I know for me the thing is that getfullargspec() keeps getting patched up to support any changes we make to function signatures that aren't the smoothest or more accurate way to represent new semantics and so it hasn't been maintenance-free. And in the face of inspect.Signature which feels like a cleaner solution that is easier to improve upon as function signatures change it makes folks not want to keep putting work into the older functions.

    I personally think we should at least document the deprecation of the functions to strongly encourage people to not use it in new code. I don't find the "Note that signature() and Signature Object provide the recommended API for callable introspection" message "loud" enough to get the message across.

    @zzzeek
    Copy link
    Mannequin

    zzzeek mannequin commented May 16, 2019

    A deprecating warning doesn't hurt: you are still able to run your code.

    this is very much untrue in modern testing environments, as it is common that test suites fail on deprecation warnings, especially in libraries, to ensure downstream compatibility. My observation is that as Python 3 has been generally a lot more aggressive about new language features than things used to be in the py2k days, a lot more of these deprecations have been coming through.

    I've looked at PEP-570 and this new syntax does not affect the four libraries for which I have to rewrite getfullargspec, so while I'm looking a bit at things like the "Signature" backport for py2.7 (funcsigs), the pure-Python complexity of Signature is very hard to take on as well as a py2k-only dependency so I'm still leaning towards producing the most minimal getfullargspec() I can use that continues to work for "traditional" Python signatures.

    i understand that Signature is "future-proof", but it is heavy on the pure Python and object creation and does not provide for any simple py2k/py3k cross-compatibility solution. I'd very much prefer that Signature at least be written in C for a few major versions, as well as the community has had time to move further python-3-only, before getfullargspec is so aggressively deprecated.

    @hroncok
    Copy link
    Mannequin

    hroncok mannequin commented May 16, 2019

    As a downstream maintainers of Python in Fedora, this is a great PITA for us. A lot of projects unfortunately treat DeprecationWarnings as errors and we run upstream test suites. It appears that this particular DeprecationWarning is now failing the test suites of the majority of important Python libraries. The warning is there from pluggy (a pytest dependency, already fixed), hypothesis, etc... We mitigate this from the testing framework dependencies, only to realize the tested library itself uses it as well. Current list of problematic packages includes pytest, tornado, sqlalchemy, numpy, dateutil and others.

    I'm not saying that should be the only reason not to deprecate stuff (nothing could be deprecated if we stretch that argument too far), but clearly "deprecating warning doesn't hurt" is not an argument either.

    @pablogsal
    Copy link
    Member Author

    For now, I am going to proceed to merge PR13245 and un-deprecate getfullargspec().

    Being said that I would want to remark that if test suites are broken when deprecation warnings are emitted that is (1) their own choice, (2) precisely to detect when some deprecation is happening. If we apply some version of this reasoning to everything the standard library will not be able to deprecate anything (not even raise deprecation warnings!). We are talking again and again that we have a lot of old things in the standard library but it seems that removing them is also a problem.

    I want to make clear that I understand the arguments here and this is why I am going to merge PR13245 and I also apologize for any pain this may cause, but I also would ask people for empathy towards the standard library, Python and core developers.

    Regarding PEP-570, getfullargspec() will report positional-only arguments as regular arguments.

    @pablogsal
    Copy link
    Member Author

    New changeset aee19f5 by Pablo Galindo in branch 'master':
    bpo-36751: Undeprecate getfullargspec (GH-13245)
    aee19f5

    @ncoghlan
    Copy link
    Contributor

    Thanks Pablo.

    Noting for the record: positional-only arguments aren't new *semantically", since extension modules have always allowed them, and you've long been able to emulate them in Python code by accepting "*args".

    Prior to the introduction of argument clinic and __text_signature__, the inspect module wouldn't report *anything* particularly useful for affected signatures, so folks have long built argument introspection based systems around the assumption that they will only be passed functions that they can successfully introspect.

    The introduction of a Python level syntax for positional-only arguments doesn't change that logic, as all a consuming application has to do to be compatible with them is to adopt the principle "if an argument can be supplied positionally, it will be supplied positionally", and then only use keywords for keyword-only arguments.

    Alternatively, consuming frameworks are also free to make "don't use positional-only arguments" a constraint on the callable inputs they accept.

    That said, we *may* want to add a "num_position_only" attribute to FullArgSpec that isn't part of the tuple unpacking, for similar reasons to why we undeprecated getfullargspec in the first place: the easiest way to migrate from getfullargspec to Signature is to write a wrapper API, and if the existence of a wrapper API is inevitable (as I now believe it is, after our experience with the first attempted deprecation), we may as well maintain a properly tested one in the standard library, rather than seeing multiple variants of the same code spring up elsewhere.

    @zzzeek
    Copy link
    Mannequin

    zzzeek mannequin commented May 17, 2019

    We are talking again and again that we have a lot of old things in the standard library but it seems that removing them is also a problem.

    I agree that the reason we have these deprecation warnings is so that we do get notified and we do fix them. I think Signature is tough here because it is so different from how getfullargspec() worked which makes it difficult to port towards, and its very long and complicated invocation steps, with many loops, function and method calls, object creation overhead, none of which existed in Python 3.3's 20 line getfullargspec() implementation, is making me very hesitant to switch to it and I'm continuing to consider just writing my own thing that keeps the overhead as low as possible; but I will run timing tests on all versions of things before I do anything like that.

    @zzzeek
    Copy link
    Mannequin

    zzzeek mannequin commented May 17, 2019

    Just did some benchmarks, and while Signature has apparently had a big speedup in Python 3.6, it is still much less performant than either the Python 2.7 or Python 3.3 implementations, anywhere from 6-18 times slower approximately depending on the function. For those of us that need a getargspec that is only used on selected, internal functions where we don't need the newer language features, it's likely worth it to vendor the Python 3.3 getfullargspec() implementation which is very simple:

    https://gist.github.com/zzzeek/0eb0636fa3917f36ffd887d9f765c208

    @vstinner
    Copy link
    Member

    Can this issue be closed now? This issue was specific to the impact of the PEP-570 on the inspect module. It's now fixed, right?

    If someone wants to continue the discussion about a specific aspect of the inspect module, I suggest to start a thread on python-dev, or open a more specific issue, for example to discuss inspect performance.

    @ncoghlan
    Copy link
    Contributor

    I split https://bugs.python.org/issue37010 out as a separate performance issue in case anyone is inclined to explore the idea of reversing the relationship between inspect.signature and inspect.getfullargspec, such that the latter becomes a fast building block for the former operation, rather than a relatively inefficient compatibility wrapper the way it is now.

    @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
    3.8 only security fixes interpreter-core (Objects, Python, Grammar, and Parser dirs)
    Projects
    None yet
    Development

    No branches or pull requests

    6 participants