[BEAM-5878] [BEAM-5490] Add partial support for functions with keyword-only arguments. #6781
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
tvalentyn
force-pushed
the
fix_getargspec
branch
5 times, most recently
from
e12def5
to
d116395
Compare
tvalentyn
changed the title
tvalentyn
force-pushed
the
fix_getargspec
branch
from
d116395
to
093aa6e
Compare
|
Run Dataflow Python ValidatesRunner |
|
Run Python Dataflow ValidatesRunner |
tvalentyn
force-pushed
the
fix_getargspec
branch
from
093aa6e
to
49f831b
Compare
|
R: @aaltay |
aaltay
reviewed
Oct 30, 2018
| varkw = argspec.keywords | ||
| kwonlyargs = [] | ||
|
|
||
| return (len(argspec.args) + len(kwonlyargs) > 1 + is_bound or |
There was a problem hiding this comment.
We preserve the original intent: to check that total number of arguments is more than 1, or 2 if the method is bound.
| # Arguments with the %unknown% prefix will be ignored in the type | ||
| # checking code. | ||
| return inspect.ArgSpec( | ||
| ['_'], '__unknown__varargs', '__unknown__keywords', ()) | ||
| try: |
There was a problem hiding this comment.
You can use _use_legacy_getargspec. You do not need another try/except.
| try: | ||
| callargs = inspect.getcallargs(func, *packed_typeargs, **typekwargs) | ||
| except TypeError as e: | ||
| raise TypeCheckError(e) | ||
| finally: | ||
| # Revert monkey-patch. |
There was a problem hiding this comment.
I think we should fully stop doing the monkey patching.
There was a problem hiding this comment.
I agree. Added a TODO to the relevant JIRA issue. We can do that in a separate change.
…d-only arguments: use getfullargspec instead of getargspec on Python 3, which recognizes kwonlyargs, avoid an irreverisble monkey-patch of inspect.getargspec.
tvalentyn
force-pushed
the
fix_getargspec
branch
from
49f831b
to
78fed78
Compare
|
@aaltay Thanks for the review! PTAL. |
aaltay
approved these changes
Oct 31, 2018
3 tasks
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Fix a crash in Beam codebase reported by TFX team that happens when a pipeline operates with functions that take keyword-only arguments (a language feature that became available in Python 3). This PR is a first step to support kwonlyargs, there are a few other places in Beam codebase captured in TODOs where additional work may be needed. We will also need to add (Python 3-only) tests where dofns/sideinputs have kwonlyargs once Python 3 test suite is in a reasonably-good shape.
getfullargspecinstead ofgetargspecon Python 3, which recognizes kwonlyargsinspect.getargspec.([BEAM-5490])Follow this checklist to help us incorporate your contribution quickly and easily:
[BEAM-XXX] Fixes bug in ApproximateQuantiles, where you replaceBEAM-XXXwith the appropriate JIRA issue, if applicable. This will automatically link the pull request to the issue.It will help us expedite review of your Pull Request if you tag someone (e.g.
@username) to look at it.Post-Commit Tests Status (on master branch)