[BEAM-5878] support DoFns with Keyword-only arguments #9237

lazylynx · 2019-08-03T00:19:33Z

support DoFns with Keyword-only arguments in Python 3 and add test reverted in #8750 with no syntax error in Python 2

tests condition are fixed as follows due to errors:

test_side_input_keyword_only_args

result2 = pcol | 'compute2' >> beam.FlatMap(
  sort_with_side_inputs,
  beam.pvalue.AsIter(side))

to

result2 = pcol | 'compute2' >> beam.FlatMap(
  sort_with_side_inputs,
  beam.pvalue.AsList(side))

test_combine_keyword_only_args

assert_that(result2, equal_to([23]), label='assert2')

to

assert_that(result2, equal_to([49]), label='assert2')

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)

Lang	Apex	Dataflow	Gearpump	Samza
Go	---	---	---	---
Java
Python	---		---	---

Pre-Commit Tests Status (on master branch)

---	Java	Python	Go	Website
Non-portable
Portable	---		---	---

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

aaltay · 2019-08-03T00:28:10Z

R: @tvalentyn

…test added in apache#8505

tvalentyn · 2019-08-05T07:11:43Z

Hi @lazylynx , is this change ready for review?

lazylynx · 2019-08-05T08:20:08Z

@tvalentyn sorry for no notification, please start reviewing

sdks/python/apache_beam/transforms/transforms_keyword_only_args_py3_only_test.py

tvalentyn · 2019-08-06T13:59:23Z

Run Python 3.5 PostCommit

tvalentyn · 2019-08-06T13:59:34Z

Run Python 2 PostCommit

lazylynx · 2019-08-07T17:54:52Z

Run Python PreCommit

… in Python 2 and avoid using exec

lazylynx · 2019-08-08T15:31:59Z

@tvalentyn PTAL

tvalentyn

Thanks a lot, @lazylynx ! Left a few small comments.

Have you considered making a change in Dill (uqfoundation/dill#313)? Your patch can translate to dill codebase, and it would be a cleaner change there. We can still keep the patch in Beam until Dill comes up with a release. Dill maintainers can provide you with additional feedback.

Also, I will not be unavailable for next 2 weeks. If you would like the PR review to finish before then, you can reach out to https://github.com/udim or https://github.com/aaltay.

Thank you.

sdks/python/apache_beam/internal/pickler.py

sdks/python/apache_beam/transforms/transforms_keyword_only_args_test_py3.py

sdks/python/apache_beam/internal/pickler.py

tvalentyn · 2019-08-10T07:05:57Z

sdks/python/apache_beam/transforms/transforms_keyword_only_args_test_py3.py

+  def test_combine_keyword_only_args(self):
+    pipeline = TestPipeline()
+
+    def bounded_sum(values, *s, bound=500):


The reason you had to change test_combine_keyword_only_args is that this scenario is actually nondeterministic.
From https://beam.apache.org/documentation/programming-guide/#combine

Because the input data (including the value collection) may be distributed across multiple workers, the combining function might be called multiple times to perform partial combining on subsets of the value collection.

By asserting that result2 is [49] we imply that a runner will compute partial sum 3 times, which will add 13 three times. So we rely on an implementation detail of the Direct Runner (which executes this test).
The test would be deterministic if we add zeros, e.g. beam.CombineGlobally(bounded_sum, 0, 0). This would make the test more stable towards future changes in direct runner, and fit the purpose of this PR to exercise keyword-only arguments. We can also come up with a better example.

And, thanks a lot for calling out changes to the tests!

…yle and None check

lazylynx · 2019-08-20T23:33:39Z

Run Portable_Python PreCommit

lazylynx · 2019-08-21T13:24:00Z

@tvalentyn PTAL
Thank you for comments, especially with combine function!
And I have started working on uqfoundation/dill#313 and will create PR before long.

aaltay · 2019-08-21T16:19:22Z

sdks/python/apache_beam/internal/pickler.py

+                          state=state, listitems=listitems, dictitems=dictitems,
+                          obj=obj)
+    else:
+      pickler.save_reduce(func, args, state, listitems, dictitems, obj)


Would not this call back into new_save_reduce since you patched it in L164?

This call back calls save_reduce function of pickle.Pickler, super class of dill.dill.Pickler.
So new_save_reduce would not be called.

sdks/python/apache_beam/internal/pickler.py

lazylynx · 2019-08-24T00:00:07Z

Run Portable_Python PreCommit

lazylynx · 2019-08-24T00:09:43Z

Run Python PreCommit

lazylynx · 2019-08-25T23:47:39Z

@aaltay @tvalentyn PTAL

tvalentyn · 2019-08-26T23:11:30Z

sdks/python/apache_beam/internal/pickler.py

+    func.__kwdefaults__ = fkwdefaults
+    return func
+
+  def new_save_reduce(self, func, args, state=None, listitems=None,


Can we use a more generic signature of new_save_reduce, for example def new_save_reduce(self, func, args, **kwargs)? The problem is that we assume a particular version of the API for pickle.save_reduce here, and we can see that it will change in Python 3.8, see https://github.com/python/cpython/blob/c75f0e5bdee3cfaba9fd5b3a8549dec0aba01ebe/Lib/pickle.py#L619.

I think with a generic definition of new_save_reduce we can still update args list , and pass **kwargs to pickler.save_reduce.

Quite so. I'll work on.

Thanks, @lazylynx . It would be nice to add this to next Beam release that is not be cut in ~2 weeks. Thanks for your help.

@tvalentyn Sorry for late. PTAL

No worries, and thanks a lot, @lazylynx ! The change looks good to me. I'm going to re-run a few tests to make sure that they still pass with this change combined with a recent upgrade of dill version (#9419).

tvalentyn · 2019-09-03T16:11:53Z

retest this please

tvalentyn · 2019-09-03T16:12:00Z

Run Python 2 PostCommit

tvalentyn · 2019-09-03T16:12:09Z

Run Python 3.5 PostCommit

tvalentyn · 2019-09-03T17:24:54Z

Run Portable_Python PreCommit

tvalentyn · 2019-09-03T20:38:04Z

Merged, thanks again. As discussed before, consider making the change to Dill and reverting the monkeypatching here after Dill picks up the changes. @mmckerns is Dill maintainer and can probably help with the review.

tvalentyn · 2019-09-07T03:33:16Z

One Beam SDK users who use master branch reported that monkey-patch caused their pipeline to fail with what appears to be an infinite recursion from new_save_reduce.

If line 163 is removed, the problem disappears. The problem persists with the monkey patch, even if the body of new_save_reduce is changed to

StockPickler.save_reduce(self, func, args, *other_args, **kwargs)

@lazylynx I have not investigated that report yet but I'd suggest to revert this change in the mean time until we understand a better way to patch, which may not be necessary soon given your outstanding changes to dill.

This reverts commit 0c2d8be.

mmckerns · 2019-09-08T10:05:06Z

@tvalentyn: I agree, the monkey-patch is better pushed into dill. I have several 3.7/3.8 specific changes in the pipeline now, and this was one I hadn't gotten to yet. Will review the PR and move forward to get it into dill, which is a cleaner solution. Thanks @lazylynx

lazylynx · 2019-09-09T01:06:35Z

@tvalentyn Understood. Sorry for bothering.
@mmckerns Thanks. I'm waiting for your review.

mmckerns · 2019-09-09T14:30:04Z

the relevant changes are merged into dill.

tvalentyn · 2019-09-09T16:21:30Z

thanks, @lazylynx and @mmckerns .

tvalentyn · 2019-09-09T21:29:25Z

FYI, I have checked that apache_beam.transforms.transforms_keyword_only_args_test_py3 that we added in this PR passes with dill==0.3.1.dev0.
@mmckerns you by chance have an idea when dill 0.3.1 may be released? Thank you.

mmckerns · 2019-09-10T01:03:42Z

@tvalentyn: should be before the end of the month. Trying to get in some more 3.8/3.7 compat, and currently it's slated for release on Sept 22.

tvalentyn · 2019-09-10T15:21:00Z

@mmckerns Thanks for the update. Looking forward to that release. By the way, it would be helpful for Dill users to have a summary of release notes associated with releases.

mmckerns · 2019-09-10T22:47:07Z

I know (uqfoundation/dill#223) I attach a milestone to each issue, but that doesn't give a full summary as some features are not reflected in the issues. It's on the todo.

* Also add back the unit tests introduced in apache#8505 with minor changes.

…#9237)" This reverts commit 0c2d8be.

tvalentyn · 2019-09-25T15:01:42Z

@tvalentyn: should be before the end of the month. Trying to get in some more 3.8/3.7 compat, and currently it's slated for release on Sept 22.

Hi @mmckerns do you have by chance an updated timeline for next release? I don't know how complicated dill releases are, but would be happy to know if you have any suggestions for how other dill users can help with releases. Thank you!

mmckerns · 2019-09-25T16:32:20Z

The dill release was pushed back a few days in dealing with some 3.8 issues, and now will happen on the 27th. I try not to slide the release dates back, but we didn't allocate time sufficiently for the extent of the changes that came with pickling in 3.8.

tvalentyn · 2019-09-25T17:31:47Z

@mmckerns Thanks for the update.

lazylynx force-pushed the support_keyword-only-arguments branch from 60caf81 to 26d4a04 Compare August 3, 2019 09:51

[BEAM-5878] DoFns with Keyword-only arguments supported and restored …

0e5bd93

…test added in apache#8505

lazylynx force-pushed the support_keyword-only-arguments branch from 26d4a04 to 0e5bd93 Compare August 3, 2019 11:41

tvalentyn requested changes Aug 6, 2019

View reviewed changes

sdks/python/apache_beam/transforms/transforms_keyword_only_args_py3_only_test.py Outdated Show resolved Hide resolved

lazylynx force-pushed the support_keyword-only-arguments branch from db1c343 to 56fa1b6 Compare August 7, 2019 16:35

fixup: [BEAM-5878] renamed test to have appropriate suffix not to run…

d7e1212

… in Python 2 and avoid using exec

lazylynx force-pushed the support_keyword-only-arguments branch from 56fa1b6 to d7e1212 Compare August 8, 2019 14:32

tvalentyn reviewed Aug 10, 2019

View reviewed changes

sdks/python/apache_beam/internal/pickler.py Outdated Show resolved Hide resolved

sdks/python/apache_beam/transforms/transforms_keyword_only_args_test_py3.py Show resolved Hide resolved

sdks/python/apache_beam/internal/pickler.py Outdated Show resolved Hide resolved

tvalentyn reviewed Aug 10, 2019

View reviewed changes

fixup: [BEAM-5878] add test for not checked condition and mod code st…

7721417

…yle and None check

lazylynx force-pushed the support_keyword-only-arguments branch from c32aa86 to 7721417 Compare August 12, 2019 13:42

aaltay reviewed Aug 21, 2019

View reviewed changes

lazylynx added 2 commits August 23, 2019 09:02

fixup: [BEAM-5878] update comment and TODO

fef7199

fixup: [BEAM-5878] fix nit

5d7de9f

tvalentyn reviewed Aug 26, 2019

View reviewed changes

fixup: [BEAM-5878] generalize signature of new_save_reduce

0cb0be0

tvalentyn approved these changes Sep 3, 2019

View reviewed changes

tvalentyn merged commit 0c2d8be into apache:master Sep 3, 2019

tvalentyn added a commit that referenced this pull request Sep 7, 2019

Revert "[BEAM-5878] support DoFns with Keyword-only arguments (#9237)"

4eabda8

This reverts commit 0c2d8be.

tvalentyn mentioned this pull request Sep 7, 2019

Revert "[BEAM-5878] support DoFns with Keyword-only arguments" #9517

Merged

soyrice pushed a commit to soyrice/beam that referenced this pull request Sep 19, 2019

[BEAM-5878] support DoFns with Keyword-only arguments (apache#9237)

ab3eeb3

* Also add back the unit tests introduced in apache#8505 with minor changes.

soyrice pushed a commit to soyrice/beam that referenced this pull request Sep 19, 2019

Revert "[BEAM-5878] support DoFns with Keyword-only arguments (apache…

6e9dd9b

…#9237)" This reverts commit 0c2d8be.

lazylynx mentioned this pull request Sep 29, 2019

[BEAM-5878] update dill min version to 0.3.1.1 and add test for functions with Keyword-only arguments #9686

Merged

3 tasks

lazylynx deleted the support_keyword-only-arguments branch February 13, 2020 15:41

[BEAM-5878] support DoFns with Keyword-only arguments #9237

[BEAM-5878] support DoFns with Keyword-only arguments #9237

Conversation

lazylynx commented Aug 3, 2019 • edited

test_side_input_keyword_only_args

test_combine_keyword_only_args

Post-Commit Tests Status (on master branch)

Pre-Commit Tests Status (on master branch)

aaltay commented Aug 3, 2019

tvalentyn commented Aug 5, 2019

lazylynx commented Aug 5, 2019

tvalentyn commented Aug 6, 2019

tvalentyn commented Aug 6, 2019

lazylynx commented Aug 7, 2019

lazylynx commented Aug 8, 2019

tvalentyn left a comment • edited

Choose a reason for hiding this comment

tvalentyn Aug 10, 2019 • edited

Choose a reason for hiding this comment

lazylynx commented Aug 20, 2019

lazylynx commented Aug 21, 2019

aaltay Aug 21, 2019

Choose a reason for hiding this comment

lazylynx Aug 25, 2019

Choose a reason for hiding this comment

lazylynx commented Aug 24, 2019

lazylynx commented Aug 24, 2019

lazylynx commented Aug 25, 2019

tvalentyn Aug 26, 2019 • edited

Choose a reason for hiding this comment

lazylynx Aug 27, 2019

Choose a reason for hiding this comment

tvalentyn Aug 28, 2019

Choose a reason for hiding this comment

lazylynx Sep 2, 2019

Choose a reason for hiding this comment

tvalentyn Sep 3, 2019

Choose a reason for hiding this comment

tvalentyn commented Sep 3, 2019

tvalentyn commented Sep 3, 2019

tvalentyn commented Sep 3, 2019

tvalentyn commented Sep 3, 2019

tvalentyn commented Sep 3, 2019

tvalentyn commented Sep 7, 2019

mmckerns commented Sep 8, 2019

lazylynx commented Sep 9, 2019

mmckerns commented Sep 9, 2019

tvalentyn commented Sep 9, 2019

tvalentyn commented Sep 9, 2019

mmckerns commented Sep 10, 2019

tvalentyn commented Sep 10, 2019

mmckerns commented Sep 10, 2019

tvalentyn commented Sep 25, 2019

mmckerns commented Sep 25, 2019 • edited

tvalentyn commented Sep 25, 2019

lazylynx commented Aug 3, 2019 •

edited

tvalentyn left a comment •

edited

tvalentyn Aug 10, 2019 •

edited

tvalentyn Aug 26, 2019 •

edited

mmckerns commented Sep 25, 2019 •

edited