Skip to content

[BEAM-8224] Fix bug in _fn_takes_side_inputs - #9563

Merged
udim merged 3 commits into
apache:masterfrom
udim:lambda-args
Sep 17, 2019
Merged

[BEAM-8224] Fix bug in _fn_takes_side_inputs#9563
udim merged 3 commits into
apache:masterfrom
udim:lambda-args

Conversation

@udim

@udimudim commented Sep 13, 2019

Copy link
Copy Markdown
Member

Bug was introduced in the conversion to inspect.signature, and manifests
as assigning a wrapper function that accepts a single positional
argument when the wrapped function can accept an arbitrary number.


Thank you for your contribution! Follow this checklist to help us incorporate your contribution quickly and easily:

  • Choose reviewer(s) and mention them in a comment (R: @username).
  • Format the pull request title like [BEAM-XXX] Fixes bug in ApproximateQuantiles, where you replace BEAM-XXX with the appropriate JIRA issue, if applicable. This will automatically link the pull request to the issue.
  • If this contribution is large, please file an Apache Individual Contributor License Agreement.

Post-Commit Tests Status (on master branch)

LangSDKApexDataflowFlinkGearpumpSamzaSpark
GoBuild Status------Build Status------Build Status
JavaBuild StatusBuild StatusBuild StatusBuild Status
Build Status
Build Status
Build StatusBuild StatusBuild Status
Build Status
PythonBuild Status
Build Status
Build Status
Build Status
---Build Status
Build Status
Build Status------Build Status
XLang---------Build Status---------

Pre-Commit Tests Status (on master branch)

---JavaPythonGoWebsite
Non-portableBuild StatusBuild StatusBuild StatusBuild Status
Portable---Build Status------

See .test-infra/jenkins/README for trigger phrase, status and link of all Jenkins jobs.

Bug was introduced in the conversion to inspect.signature, and manifests
as assigning a wrapper function that accepts a single positional
argument when the wrapped function can accept an arbitrary number.
@udim

udim commented Sep 13, 2019

Copy link
Copy Markdown
MemberAuthor

R: @robertwb

@udim

udim commented Sep 13, 2019

Copy link
Copy Markdown
MemberAuthor

run pytthon 2 postcommit

@udim

udim commented Sep 13, 2019

Copy link
Copy Markdown
MemberAuthor

run python 2 postcommit

@udim

udim commented Sep 13, 2019

Copy link
Copy Markdown
MemberAuthor

run python 3.7 postcommit

@aaltay

aaltay commented Sep 13, 2019

Copy link
Copy Markdown
Member

Changes LGTM.

Py2 tests are failing due to: https://issues.apache.org/jira/browse/BEAM-8229

@udim

udim commented Sep 13, 2019

Copy link
Copy Markdown
MemberAuthor

Run Python 3.7 PostCommit

1 similar comment
@udim

udim commented Sep 13, 2019

Copy link
Copy Markdown
MemberAuthor

Run Python 3.7 PostCommit

@robertwbrobertwb left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This should fix it, just one comment about tests.

# Test that a lambda that accepts only a VAR_POSITIONAL can accept
# side-inputs.
result = (['a', 'b', 'c']
| beam.Map(lambda *_: 'a', 5).with_input_types(str, int))

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Let's check that it actually gets the values right, e.g. do lambda *args: args. (Similarly below.)

@markflyhigh

Copy link
Copy Markdown
Contributor

#9570 is merge which should unblock Python2_PC failure.

@udim

udim commented Sep 13, 2019

Copy link
Copy Markdown
MemberAuthor

Run Python 3.7 PostCommit

@robertwb

Copy link
Copy Markdown
Contributor

Run Portable_Python PreCommit

@aaltay

Copy link
Copy Markdown
Member

New tests are failing in the precommit tests:

17:59:06 ======================================================================
17:59:06 ERROR: test_var_positional_only_side_input_hint (apache_beam.typehints.typed_pipeline_test.SideInputTest)
17:59:06 ----------------------------------------------------------------------
17:59:06 Traceback (most recent call last):
...
17:59:06 TypeError: Expected bytes, got tuple [while running 'Map()']

@robinyqiu

Copy link
Copy Markdown
Contributor

Run Python PreCommit

@udim

udim commented Sep 16, 2019

Copy link
Copy Markdown
MemberAuthor

trivial_inference.py defaults to Any in 3.6, 3.7 since it doesn't support kwargs (added by the wrapper).
In 2.7, 3.5 these are supported, but since side-input hints are not passed along the inferred output type is str, and the coder for that fails to encode a tuple.

Even if we did pass side-input hints, there is still missing logic that would bind the arguments for the var_positional into a single tuple (like getcallargs or signature.bind).

Perhaps we should skip trivial_inference when side-inputs are present.

@aaltay

Copy link
Copy Markdown
Member

trivial_inference.py defaults to Any in 3.6, 3.7 since it doesn't support kwargs (added by the wrapper).
In 2.7, 3.5 these are supported, but since side-input hints are not passed along the inferred output type is str, and the coder for that fails to encode a tuple.

Even if we did pass side-input hints, there is still missing logic that would bind the arguments for the var_positional into a single tuple (like getcallargs or signature.bind).

Perhaps we should skip trivial_inference when side-inputs are present.

Would we need to skip it for all python version or for some python versions?

I am in favor of skipping in order to not block the release and continue with the proper fix in master.

/cc @tvalentyn in case anything needs to be documented as known issues for python 3.

Worked around an issue with trivial_inference not being passed
side-input hints.
@markflyhigh

Copy link
Copy Markdown
Contributor

Run Portable_Python PreCommit

1 similar comment
@udim

udim commented Sep 17, 2019

Copy link
Copy Markdown
MemberAuthor

Run Portable_Python PreCommit

@udim

udim commented Sep 17, 2019

Copy link
Copy Markdown
MemberAuthor

I've opened a couple of bug for the portable precommit flakes:
https://issues.apache.org/jira/browse/BEAM-8248
https://issues.apache.org/jira/browse/BEAM-8249

@udim

udim commented Sep 17, 2019

Copy link
Copy Markdown
MemberAuthor

I believe we can safely ignore the portable python precommit failure, since this PR is holding up the release.

@udim
udim merged commit af00fc4 into apache:masterSep 17, 2019
soyrice pushed a commit to soyrice/beam that referenced this pull request Sep 19, 2019
Bug was introduced in the conversion to inspect.signature, and manifests
as assigning a wrapper function that accepts a single positional
argument when the wrapped function can accept an arbitrary number.
udim added a commit to udim/beam that referenced this pull request Sep 20, 2019
Bug was introduced in the conversion to inspect.signature, and manifests
as assigning a wrapper function that accepts a single positional
argument when the wrapped function can accept an arbitrary number.
Sign up for freeto join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants

@udim@aaltay@markflyhigh@robertwb@robinyqiu