You are viewing a plain text version of this content. The canonical link for it is here.
Posted to github@beam.apache.org by GitBox <gi...@apache.org> on 2022/06/01 00:47:14 UTC

[GitHub] [beam] lostluck opened a new pull request, #17790: [BEAM-14470] Use lifecycle method names directly.

lostluck opened a new pull request, #17790:
URL: https://github.com/apache/beam/pull/17790

   Use lifecycle method names directly, rather than excluding all exported methods on DoFns. 
   
   Breaks certain usecases with extreme number (10+) of emitter parameters, when the method doesn't have a generic representation. The static code generator works in those cases, but it isn't recommended.
   
   The ideal work around would be to have a tagged API instead of the positional one, but that's significantly more work.
   
   ------------------------
   
   Thank you for your contribution! Follow this checklist to help us incorporate your contribution quickly and easily:
   
    - [ ] [**Choose reviewer(s)**](https://beam.apache.org/contribute/#make-your-change) 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.
    - [ ] Update `CHANGES.md` with noteworthy changes.
    - [ ] If this contribution is large, please file an Apache [Individual Contributor License Agreement](https://www.apache.org/licenses/icla.pdf).
   
   See the [Contributor Guide](https://beam.apache.org/contribute) for more tips on [how to make review process smoother](https://beam.apache.org/contribute/#make-reviewers-job-easier).
   
   To check the build health, please visit [https://github.com/apache/beam/blob/master/.test-infra/BUILD_STATUS.md](https://github.com/apache/beam/blob/master/.test-infra/BUILD_STATUS.md)
   
   GitHub Actions Tests Status (on master branch)
   ------------------------------------------------------------------------------------------------
   [![Build python source distribution and wheels](https://github.com/apache/beam/workflows/Build%20python%20source%20distribution%20and%20wheels/badge.svg?branch=master&event=schedule)](https://github.com/apache/beam/actions?query=workflow%3A%22Build+python+source+distribution+and+wheels%22+branch%3Amaster+event%3Aschedule)
   [![Python tests](https://github.com/apache/beam/workflows/Python%20tests/badge.svg?branch=master&event=schedule)](https://github.com/apache/beam/actions?query=workflow%3A%22Python+Tests%22+branch%3Amaster+event%3Aschedule)
   [![Java tests](https://github.com/apache/beam/workflows/Java%20Tests/badge.svg?branch=master&event=schedule)](https://github.com/apache/beam/actions?query=workflow%3A%22Java+Tests%22+branch%3Amaster+event%3Aschedule)
   
   See [CI.md](https://github.com/apache/beam/blob/master/CI.md) for more information about GitHub Actions CI.
   


-- 
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

To unsubscribe, e-mail: github-unsubscribe@beam.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org

[GitHub] [beam] lostluck commented on pull request #17790: [BEAM-14470] Use lifecycle method names directly.

Posted by GitBox <gi...@apache.org>.
lostluck commented on PR #17790:
URL: https://github.com/apache/beam/pull/17790#issuecomment-1143907081

   As discussed offline, we decided to completely remove the method restriction entirely. While this may make things a little harder for novices, it will make things harder for novices with typos in the optional methods. These should be detectable by users via testing however.
   
   Unfortunately there's no good way of validating this on our end short of doing some sort of typo/proximity algorithm, which is likely more trouble than better documentation and clearer examples, hence the initial strict approach.


-- 
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

To unsubscribe, e-mail: github-unsubscribe@beam.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org

[GitHub] [beam] lostluck commented on a diff in pull request #17790: [BEAM-14470] Use lifecycle method names directly.

Posted by GitBox <gi...@apache.org>.
lostluck commented on code in PR #17790:
URL: https://github.com/apache/beam/pull/17790#discussion_r887129346


##########
sdks/go/pkg/beam/core/graph/fn_test.go:
##########
@@ -378,6 +380,129 @@ func TestNewCombineFn(t *testing.T) {
 	})
 }
 
+func TestNewFn_DoFn(t *testing.T) {
+	// Validate wrap fallthrough
+	reflectx.RegisterStructWrapper(reflect.TypeOf((*GoodDoFn)(nil)).Elem(), func(fn interface{}) map[string]reflectx.Func {
+		gdf := fn.(*GoodDoFn)
+		return map[string]reflectx.Func{
+			processElementName: reflectx.MakeFunc1x1(func(v int) int {
+				return gdf.ProcessElement(v)
+			}),
+		}
+	})
+
+	userFn := &GoodDoFn{}
+	fn, err := NewFn(userFn)
+	if err != nil {
+		t.Errorf("NewFn(%T) failed:\n%v", userFn, err)
+	}
+	dofn, err := AsDoFn(fn, MainSingle)
+	if err != nil {
+		t.Errorf("AsDoFn(%v, MainSingle) failed:\n%v", fn.Name(), err)
+	}
+	// Check that we get expected values for all the methods.
+	if got, want := dofn.Name(), "GoodDoFn"; !strings.HasSuffix(got, want) {
+		t.Errorf("(%v).Name() = %q, want suffix %q", dofn.Name(), got, want)
+	}
+	if dofn.SetupFn() == nil {
+		t.Errorf("(%v).SetupFn() == nil, want value", dofn.Name())
+	}
+	if dofn.StartBundleFn() == nil {
+		t.Errorf("(%v).StartBundleFn() == nil, want value", dofn.Name())
+	}
+	if dofn.ProcessElementFn() == nil {
+		t.Errorf("(%v).ProcessElementFn() == nil, want value", dofn.Name())
+	}
+	if dofn.FinishBundleFn() == nil {
+		t.Errorf("(%v).FinishBundleFn() == nil, want value", dofn.Name())
+	}
+	if dofn.TeardownFn() == nil {
+		t.Errorf("(%v).TeardownFn() == nil, want value", dofn.Name())
+	}
+	if dofn.IsSplittable() {
+		t.Errorf("(%v).IsSplittable() = true, want false", dofn.Name())
+	}
+}
+
+func TestNewFn_SplittableDoFn(t *testing.T) {

Review Comment:
   Ah, I knew I missed something. Thank you!
   
   We also have a vestigial "CompactFn" It's intended to allow CombineFns to do compression of the accumulators, but it seems it's not documented or called anywhere. Out of scope to delete it in this PR, but it could be worth implementing for user use eventually.



-- 
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

To unsubscribe, e-mail: github-unsubscribe@beam.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org

[GitHub] [beam] lostluck merged pull request #17790: [BEAM-14470] Use lifecycle method names directly.

Posted by GitBox <gi...@apache.org>.
lostluck merged PR #17790:
URL: https://github.com/apache/beam/pull/17790


-- 
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

To unsubscribe, e-mail: github-unsubscribe@beam.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org

[GitHub] [beam] github-actions[bot] commented on pull request #17790: [BEAM-14470] Use lifecycle method names directly.

Posted by GitBox <gi...@apache.org>.
github-actions[bot] commented on PR #17790:
URL: https://github.com/apache/beam/pull/17790#issuecomment-1143013041

   Assigning reviewers. If you would like to opt out of this review, comment `assign to next reviewer`:
   
   R: @jrmccluskey for label go.
   
   Available commands:
   - `stop reviewer notifications` - opt out of the automated review tooling
   - `remind me after tests pass` - tag the comment author after tests pass
   - `waiting on author` - shift the attention set back to the author (any comment or push by the author will return the attention set to the reviewers)
   
   The PR bot will only process comments in the main thread (not review comments).


-- 
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

To unsubscribe, e-mail: github-unsubscribe@beam.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org

[GitHub] [beam] damccorm commented on a diff in pull request #17790: [BEAM-14470] Use lifecycle method names directly.

Posted by GitBox <gi...@apache.org>.
damccorm commented on code in PR #17790:
URL: https://github.com/apache/beam/pull/17790#discussion_r887085549


##########
sdks/go/pkg/beam/core/graph/fn_test.go:
##########
@@ -378,6 +380,129 @@ func TestNewCombineFn(t *testing.T) {
 	})
 }
 
+func TestNewFn_DoFn(t *testing.T) {
+	// Validate wrap fallthrough
+	reflectx.RegisterStructWrapper(reflect.TypeOf((*GoodDoFn)(nil)).Elem(), func(fn interface{}) map[string]reflectx.Func {
+		gdf := fn.(*GoodDoFn)
+		return map[string]reflectx.Func{
+			processElementName: reflectx.MakeFunc1x1(func(v int) int {
+				return gdf.ProcessElement(v)
+			}),
+		}
+	})
+
+	userFn := &GoodDoFn{}
+	fn, err := NewFn(userFn)
+	if err != nil {
+		t.Errorf("NewFn(%T) failed:\n%v", userFn, err)
+	}
+	dofn, err := AsDoFn(fn, MainSingle)
+	if err != nil {
+		t.Errorf("AsDoFn(%v, MainSingle) failed:\n%v", fn.Name(), err)
+	}
+	// Check that we get expected values for all the methods.
+	if got, want := dofn.Name(), "GoodDoFn"; !strings.HasSuffix(got, want) {
+		t.Errorf("(%v).Name() = %q, want suffix %q", dofn.Name(), got, want)
+	}
+	if dofn.SetupFn() == nil {
+		t.Errorf("(%v).SetupFn() == nil, want value", dofn.Name())
+	}
+	if dofn.StartBundleFn() == nil {
+		t.Errorf("(%v).StartBundleFn() == nil, want value", dofn.Name())
+	}
+	if dofn.ProcessElementFn() == nil {
+		t.Errorf("(%v).ProcessElementFn() == nil, want value", dofn.Name())
+	}
+	if dofn.FinishBundleFn() == nil {
+		t.Errorf("(%v).FinishBundleFn() == nil, want value", dofn.Name())
+	}
+	if dofn.TeardownFn() == nil {
+		t.Errorf("(%v).TeardownFn() == nil, want value", dofn.Name())
+	}
+	if dofn.IsSplittable() {
+		t.Errorf("(%v).IsSplittable() = true, want false", dofn.Name())
+	}
+}
+
+func TestNewFn_SplittableDoFn(t *testing.T) {

Review Comment:
   It would probably be good to add an extra similar test case for combineFns as well.



-- 
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

To unsubscribe, e-mail: github-unsubscribe@beam.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org

[GitHub] [beam] lostluck commented on pull request #17790: [BEAM-14470] Use lifecycle method names directly.

Posted by GitBox <gi...@apache.org>.
lostluck commented on PR #17790:
URL: https://github.com/apache/beam/pull/17790#issuecomment-1143910092

   @damccorm PTAL


-- 
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

To unsubscribe, e-mail: github-unsubscribe@beam.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org

[GitHub] [beam] codecov[bot] commented on pull request #17790: [BEAM-14470] Use lifecycle method names directly.

Posted by GitBox <gi...@apache.org>.
codecov[bot] commented on PR #17790:
URL: https://github.com/apache/beam/pull/17790#issuecomment-1142909372

   # [Codecov](https://codecov.io/gh/apache/beam/pull/17790?src=pr&el=h1&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation) Report
   > Merging [#17790](https://codecov.io/gh/apache/beam/pull/17790?src=pr&el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation) (a25256d) into [master](https://codecov.io/gh/apache/beam/commit/6774b747ba42fdc187d4ba8dc34f48a3e1cc9368?el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation) (6774b74) will **decrease** coverage by `0.00%`.
   > The diff coverage is `87.50%`.
   
   ```diff
   @@            Coverage Diff             @@
   ##           master   #17790      +/-   ##
   ==========================================
   - Coverage   74.00%   74.00%   -0.01%     
   ==========================================
     Files         695      695              
     Lines       91798    91856      +58     
   ==========================================
   + Hits        67938    67979      +41     
   - Misses      22612    22630      +18     
   + Partials     1248     1247       -1     
   ```
   
   | Flag | Coverage Δ | |
   |---|---|---|
   | go | `50.46% <87.50%> (+0.04%)` | :arrow_up: |
   
   Flags with carried forward coverage won't be shown. [Click here](https://docs.codecov.io/docs/carryforward-flags?utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#carryforward-flags-in-the-pull-request-comment) to find out more.
   
   | [Impacted Files](https://codecov.io/gh/apache/beam/pull/17790?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation) | Coverage Δ | |
   |---|---|---|
   | [sdks/go/pkg/beam/core/graph/fn.go](https://codecov.io/gh/apache/beam/pull/17790/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-c2Rrcy9nby9wa2cvYmVhbS9jb3JlL2dyYXBoL2ZuLmdv) | `76.41% <87.50%> (-0.41%)` | :arrow_down: |
   | [sdks/go/pkg/beam/io/textio/textio.go](https://codecov.io/gh/apache/beam/pull/17790/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-c2Rrcy9nby9wa2cvYmVhbS9pby90ZXh0aW8vdGV4dGlvLmdv) | `55.15% <0.00%> (-10.42%)` | :arrow_down: |
   | [sdks/go/pkg/beam/pardo.go](https://codecov.io/gh/apache/beam/pull/17790/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-c2Rrcy9nby9wa2cvYmVhbS9wYXJkby5nbw==) | `47.41% <0.00%> (-3.03%)` | :arrow_down: |
   | [...o/pkg/beam/io/rtrackers/offsetrange/offsetrange.go](https://codecov.io/gh/apache/beam/pull/17790/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-c2Rrcy9nby9wa2cvYmVhbS9pby9ydHJhY2tlcnMvb2Zmc2V0cmFuZ2Uvb2Zmc2V0cmFuZ2UuZ28=) | `75.70% <0.00%> (ø)` | |
   | [sdks/go/pkg/beam/io/textio/sdf.go](https://codecov.io/gh/apache/beam/pull/17790/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-c2Rrcy9nby9wa2cvYmVhbS9pby90ZXh0aW8vc2RmLmdv) | | |
   | [sdks/go/pkg/beam/core/sdf/wrappedbounded.go](https://codecov.io/gh/apache/beam/pull/17790/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-c2Rrcy9nby9wa2cvYmVhbS9jb3JlL3NkZi93cmFwcGVkYm91bmRlZC5nbw==) | `0.00% <0.00%> (ø)` | |
   | [sdks/go/pkg/beam/core/runtime/graphx/translate.go](https://codecov.io/gh/apache/beam/pull/17790/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-c2Rrcy9nby9wa2cvYmVhbS9jb3JlL3J1bnRpbWUvZ3JhcGh4L3RyYW5zbGF0ZS5nbw==) | `43.13% <0.00%> (+0.11%)` | :arrow_up: |
   | [sdks/go/pkg/beam/core/runtime/exec/sdf.go](https://codecov.io/gh/apache/beam/pull/17790/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-c2Rrcy9nby9wa2cvYmVhbS9jb3JlL3J1bnRpbWUvZXhlYy9zZGYuZ28=) | `71.79% <0.00%> (+0.78%)` | :arrow_up: |
   | [...ks/go/pkg/beam/runners/dataflow/dataflowlib/job.go](https://codecov.io/gh/apache/beam/pull/17790/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-c2Rrcy9nby9wa2cvYmVhbS9ydW5uZXJzL2RhdGFmbG93L2RhdGFmbG93bGliL2pvYi5nbw==) | `21.55% <0.00%> (+5.28%)` | :arrow_up: |
   | ... and [1 more](https://codecov.io/gh/apache/beam/pull/17790/diff?src=pr&el=tree-more&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation) | |
   
   ------
   
   [Continue to review full report at Codecov](https://codecov.io/gh/apache/beam/pull/17790?src=pr&el=continue&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation).
   > **Legend** - [Click here to learn more](https://docs.codecov.io/docs/codecov-delta?utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation)
   > `Δ = absolute <relative> (impact)`, `ø = not affected`, `? = missing data`
   > Powered by [Codecov](https://codecov.io/gh/apache/beam/pull/17790?src=pr&el=footer&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation). Last update [6774b74...a25256d](https://codecov.io/gh/apache/beam/pull/17790?src=pr&el=lastupdated&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation). Read the [comment docs](https://docs.codecov.io/docs/pull-request-comments?utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation).
   


-- 
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

To unsubscribe, e-mail: github-unsubscribe@beam.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org