8000 Keyword-only arguments in compat/_inspect.py · Issue #16542 · numpy/numpy · GitHub
[go: up one dir, main page]
Skip to content

Keyword-only arguments in compat/_inspect.py #16542

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.

Sign up for GitHub

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

Jump to bottom
Closed
surculus12 opened this issue Jun 9, 2020 · 7 comments
Closed

Keyword-only arguments in compat/_inspect.py #16542

surculus12 opened this issue Jun 9, 2020 · 7 comments

Comments

@surculus12
Copy link
surculus12 commented Jun 9, 2020 •
edited
Loading

There was an issue (#16299) that was caused by the get functions in numpy.compat._inspect not handling keyword-only arguments (PEP 3102 asterisk style). The functions would ignore the keyword-only arguments, which either lead to incomplete or incorrect output.

#16541 fixed this issue partially. However, I missed two issues:

  1. verify_matching_signatures(lambda a, *, foo=1: 0, lambda a, foo=None: 0) will pass because it treats foo the same, regardless of whether it is kwonly. This is probably not the intended behaviour. As such, arguments that are keyword-only would need to be treated differently from those that are not, which would require changing the return of the function.

  2. formatargspec and formatargvalues do not handle kwonly arguments, similar to above. As such, format(get(...)) would strip out the kwonly property of arguments. Since this method is used to establish signatures elsewhere, it is probably not the intended behaviour. Fixing this would require changing the arguments of the format functions to match the change discussed above.

Since this would require changing the signatures of these functions, I'd like some input on whether this should be done. The proposal would be to modify the functions as follows:

# Old returns
def getargs(co):
    # ...
    return args, varargs, varkw

# New returns
def getargs(co):
    # ...
    return args, kwonlyargs, varargs, varkw


# Old returns
def getargspec(func):
    # ...
    return args, varargs, varkw, func.__defaults__

# New returns
def getargspec(func):
    # ...
    return args, kwonlyargs, varargs, varkw, func.__defaults__, tuple(func.__kwdefaults__.values())

And similarly, the signatures of format:

# Old signature
def formatargspec(args, varargs=None, varkw=None, defaults=None,
                  formatarg=str,
                  formatvarargs=lambda name: '*' + name,
                  formatvarkw=lambda name: '**' + name,
                  formatvalue=lambda value: '=' + repr(value),
                  join=joinseq):
    # ...
# New signature
def formatargspec(args, kwonlyargs=None, varargs=None, varkw=None, defaults=None, kwdefaults=None,
                  formatarg=str,
                  formatvarargs=lambda name: '*' + name,
                  formatvarkw=lambda name: '**' + name,
                  formatvalue=lambda value: '=' + repr(value),
                  join=joinseq):
    # ...

# Old signature
def formatargvalues(args, varkw, varargs, locals,
                    formatarg=str,
                    formatvarargs=lambda name: '*' + name,
                    formatvarkw=lambda name: '**' + name,
                    formatvalue=lambda value: '=' + repr(value),
                    join=joinseq)
    # ...

# New signature
def formatargvalues(args, kwonlyargs, varargs, varkw, locals,
                    formatarg=str,
                    formatvarargs=lambda name: '*' + name,
                    formatvarkw=lambda name: '**' + name,
                    formatvalue=lambda value: '=' + repr(value),
                    join=joinseq)
    # ...

This should not interfere with anything, unless it was relying on incorrect behaviour by ignoring kwonly arguments. The usages that pycharm detects are:

formatargspec:

  • numpy.ma.core.get_object_signature which is simply formatargspec(*getargspec(obj)) and will remain unaffected.

formatargvalues:

  • No usage found

getargspec:

getargs:

  • numpy.compat._inspect.getargspec is being fixed, changing returns
  • numpy.compat._inspect.getargvalues would probably also need to have returns changed, though I cannot see any usage

I'm not familiar enough with the project to know whether there are could be any usages that can't be detected.

If this seems like a reasonable change I will add it and create a PR.
@surculus12 surculus12 mentioned this issue Jun 9, 2020
Closed
@eric-wieser
Copy link
Member
eric-wieser commented Jun 9, 2020 •
edited
Loading

Since this method is used to establish signatures elsewhere,

Can you point to those uses?

Nevermind, I see now.

@surculus12
Copy link
Author
surculus12 commented Jun 9, 2020 •
edited
Loading

surculus12/numpy@edd97f9 is the proposed change:

  • keyword-only args are now kept in kwonlyargs, while their defaults are kept in kwonlydefaults.
  • formatting is done by injecting kwonly args after * (or the *args/varargs). Added a helper function for formatting default args to avoid performing the logic twice
  • added tests for getargspec and formatargspec to make sure this worked properly, can remove if you don't want tests there
  • added the kwonly variables (& defaults) to the ArgSpec tuple and the if-block in verify_matching_signatures
  • fixed a verify_matching_signatures test that was passing for the wrong reason
  • updated docstrings

@rossbar
Copy link
Contributor
rossbar commented Jun 12, 2020

The docstring for numpy.compat._inspect mentions that it is included instead of the Python inspect module due to import time concerns. However this was recently revisited in #16311 . Does this have an impact on numpy.compat._inspect? Can it perhaps be replaced altogether?

@surculus12
Copy link
Author

Sorry about the late reply.

Should I replace the usages of _inspect for the sake of Python's inspect? If the impact of import is acceptable it seems more robust to use the official package.

Would you like to deprecate _inspect altogether or where the keyword-only argument is known to cause issues?

@rossbar
Copy link
Contributor
rossbar commented Jun 29, 2020

@surculus12 I haven't investigated myself so I can't say, but if you're willing to look into it and see if there aren't any other sticking-points/descrepancies I think it would be an improvement if we could move towards the Python inspect module.

@shoyer
Copy link
Member
shoyer commented Jun 29, 2020

+1 for revisiting using the builtin inspect. Maintaining our own version of this code is probably not worthwhile.

@seberg
Copy link
Member
seberg commented Jan 28, 2024 •
edited
Loading

Closing, the module is removed. (Or at least being removed)

EDIT: Seems we still use the incorrect version internally, should probaly just use inspect, the import seems not so bad. I think the other issue is enough to track that part.

@seberg seberg closed this as completed Jan 28, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

No branches or pull requests
5 participants
@seberg @eric-wieser @shoyer @rossbar @surculus12
0