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 2020/11/25 23:57:50 UTC
[GitHub] [beam] robertwb opened a new pull request #13431: [BEAM-11354] Allow DoFn itself to be used as the restriction provider.
robertwb opened a new pull request #13431:
URL: https://github.com/apache/beam/pull/13431
------------------------
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).
Post-Commit Tests Status (on master branch)
------------------------------------------------------------------------------------------------
Lang | SDK | Dataflow | Flink | Samza | Spark | Twister2
--- | --- | --- | --- | --- | --- | ---
Go | [](https://ci-beam.apache.org/job/beam_PostCommit_Go/lastCompletedBuild/) | --- | [](https://ci-beam.apache.org/job/beam_PostCommit_Go_VR_Flink/lastCompletedBuild/) | --- | [](https://ci-beam.apache.org/job/beam_PostCommit_Go_VR_Spark/lastCompletedBuild/) | ---
Java | [](https://ci-beam.apache.org/job/beam_PostCommit_Java/lastCompletedBuild/) | [](https://ci-beam.apache.org/job/beam_PostCommit_Java_ValidatesRunner_Dataflow/lastCompletedBuild/)<br>[](https://ci-beam.apache.org/job/beam_PostCommit_Java_VR_Dataflow_V2/lastCompletedBuild/)<br>[](https://ci-beam.apache.org/job/beam_PostCommit_Java_ValidatesRunner_Dataflow_Java11/lastCompletedBuild/) | [](https://ci-beam
.apache.org/job/beam_PostCommit_Java_ValidatesRunner_Flink/lastCompletedBuild/)<br>[](https://ci-beam.apache.org/job/beam_PostCommit_Java_ValidatesRunner_Flink_Java11/lastCompletedBuild/)<br>[](https://ci-beam.apache.org/job/beam_PostCommit_Java_PVR_Flink_Batch/lastCompletedBuild/)<br>[](https://ci-beam.apache.org/job/beam_PostCommit_Java_PVR_Flink_Streaming/lastCompletedBuild/) | [](https://ci-beam.apache.org/job/beam_PostCommit_Java_ValidatesRunner_Samza/lastCompletedBuild/) | [](https://ci-beam.apache.org/job/beam_PostCommit_Java_ValidatesRunner_Spark/lastCompletedBuild/)<br>[](https://ci-beam.apache.org/job/beam_PostCommit_Java_PVR_Spark_Batch/lastCompletedBuild/)<br>[](https://ci-beam.apache.org/job/beam_PostCommit_Java_ValidatesRunner_SparkStructuredStreaming/lastCompletedBuild/) | [](https://ci-beam.apache.org/job/beam_PostCommit_Java_ValidatesRunner_Twister2/lastCompletedBuild/)
Python | [](https://ci-beam.apache.org/job/beam_PostCommit_Python36/lastCompletedBuild/)<br>[](https://ci-beam.apache.org/job/beam_PostCommit_Python37/lastCompletedBuild/)<br>[](https://ci-beam.apache.org/job/beam_PostCommit_Python38/lastCompletedBuild/) | [](https://ci-beam.apache.org/job/beam_PostCommit_Py_VR_Dataflow/lastCompletedBuild/)<br>[](https://ci-beam.apache.org/job/beam_PostCommit_Py_VR_Dataflow_V2/lastCompletedBuild/)<br>[](https://ci-beam.apache.org/job/beam_PostCommit_Py_ValCont/lastCompletedBuild/) | [](https://ci-beam.apache.org/job/beam_PreCommit_Python_PVR_Flink_Cron/lastCompletedBuild/)<br>[](https://ci-beam.apache.org/job/beam_PostCommit_Python_VR_Flink/lastCompletedBuild/) | --- | [](https://ci-beam.apache.org/job/beam_PostCommit_Python_VR_Spark/lastCompletedBuild/) | ---
XLang | [](https://ci-beam.apache.org/job/beam_PostCommit_XVR_Direct/lastCompletedBuild/) | [](https://ci-beam.apache.org/job/beam_PostCommit_XVR_Dataflow/lastCompletedBuild/) | [](https://ci-beam.apache.org/job/beam_PostCommit_XVR_Flink/lastCompletedBuild/) | --- | [](https://ci-beam.apache.org/job/beam_PostCommit_XVR_Spark/lastCompletedBuild/) | ---
Pre-Commit Tests Status (on master branch)
------------------------------------------------------------------------------------------------
--- |Java | Python | Go | Website | Whitespace | Typescript
--- | --- | --- | --- | --- | --- | ---
Non-portable | [](https://ci-beam.apache.org/job/beam_PreCommit_Java_Cron/lastCompletedBuild/) | [](https://ci-beam.apache.org/job/beam_PreCommit_Python_Cron/lastCompletedBuild/)<br>[](https://ci-beam.apache.org/job/beam_PreCommit_PythonLint_Cron/lastCompletedBuild/)<br>[](https://ci-beam.apache.org/job/beam_PreCommit_PythonDocker_Cron/lastCompletedBuild/) <br>[](https://ci-beam.apache.org/job/beam_PreCommit_PythonDocs_Cron/lastCompletedBuild/) | [](https://ci-beam.apache.org/job/beam_PreCommit_Go_Cron/lastCompletedBuild/) | [](https://ci-beam.apache.org/job/beam_PreCommit_Website_Cron/lastCompletedBuild/) | [](https://ci-beam.apache.org/job/beam_PreCommit_Whitespace_Cron/lastCompletedBuild/) | [](https://ci-beam.apache.org/job/beam_PreCommit_Typescript_Cron/lastCompletedBuild/)
Portable | --- | [](https://ci-beam.apache.org/job/beam_PreCommit_Portable_Python_Cron/lastCompletedBuild/) | --- | --- | --- | ---
See [.test-infra/jenkins/README](https://github.com/apache/beam/blob/master/.test-infra/jenkins/README.md) for trigger phrase, status and link of all Jenkins jobs.
GitHub Actions Tests Status (on master branch)
------------------------------------------------------------------------------------------------
[](https://github.com/apache/beam/actions?query=workflow%3A%22Build+python+source+distribution+and+wheels%22+branch%3Amaster+event%3Aschedule)
[](https://github.com/apache/beam/actions?query=workflow%3A%22Python+Tests%22+branch%3Amaster+event%3Aschedule)
[](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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [beam] boyuanzz commented on a change in pull request #13431: [BEAM-11354] Allow DoFn itself to be used as the restriction provider.
Posted by GitBox <gi...@apache.org>.
boyuanzz commented on a change in pull request #13431:
URL: https://github.com/apache/beam/pull/13431#discussion_r533751926
##########
File path: sdks/python/apache_beam/runners/common.py
##########
@@ -218,10 +218,11 @@ def __init__(self, obj_to_invoke, method_name):
elif core.DoFn.KeyParam == v:
self.key_arg_name = kw
elif isinstance(v, core.DoFn.RestrictionParam):
- self.restriction_provider = v.restriction_provider
+ self.restriction_provider = v.restriction_provider or obj_to_invoke
Review comment:
I understand. The case I'm wondering is like:
```python
class MySDF(beam.DoFn, TypeARestrictionTracker):
def process(restriction_tracker=RestrictionParam(TypeBRestrictionTracker()))
```
In this case, the sdk will pick up `TypeBRestrictionTracker` implementation. Is it ideal? I thought we want to pick up `TypeARestrictionTracker` in this case or just say the DoFn is not valid.
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [beam] boyuanzz commented on a change in pull request #13431: [BEAM-11354] Allow DoFn itself to be used as the restriction provider.
Posted by GitBox <gi...@apache.org>.
boyuanzz commented on a change in pull request #13431:
URL: https://github.com/apache/beam/pull/13431#discussion_r533679188
##########
File path: sdks/python/apache_beam/runners/common.py
##########
@@ -218,10 +218,11 @@ def __init__(self, obj_to_invoke, method_name):
elif core.DoFn.KeyParam == v:
self.key_arg_name = kw
elif isinstance(v, core.DoFn.RestrictionParam):
- self.restriction_provider = v.restriction_provider
+ self.restriction_provider = v.restriction_provider or obj_to_invoke
Review comment:
It might be less likely to happen but I'm curious what if the sdk author provides different `RestrictionProvider` from both ways, which one should be dominant. From implementation here, it seems like we will pick the param one.
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [beam] boyuanzz commented on pull request #13431: [BEAM-11354] Allow DoFn itself to be used as the restriction provider.
Posted by GitBox <gi...@apache.org>.
boyuanzz commented on pull request #13431:
URL: https://github.com/apache/beam/pull/13431#issuecomment-734009499
Thanks for the explanation. I'm kind of concerned about keeping both APIs. It seems like not clear enough and might be confusing for beam users. Besides, we also need to update related documentation if we decide that way.
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [beam] robertwb commented on a change in pull request #13431: [BEAM-11354] Allow DoFn itself to be used as the restriction provider.
Posted by GitBox <gi...@apache.org>.
robertwb commented on a change in pull request #13431:
URL: https://github.com/apache/beam/pull/13431#discussion_r533016487
##########
File path: sdks/python/apache_beam/runners/portability/fn_api_runner/fn_runner_test.py
##########
@@ -510,6 +510,21 @@ def process(
actual = (p | beam.Create(data) | beam.ParDo(ExpandingStringsDoFn()))
assert_that(actual, equal_to(list(''.join(data))))
+ def test_sdf_with_dofn_as_restriction_provider(self):
Review comment:
Done.
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [beam] robertwb commented on pull request #13431: [BEAM-11354] Allow DoFn itself to be used as the restriction provider.
Posted by GitBox <gi...@apache.org>.
robertwb commented on pull request #13431:
URL: https://github.com/apache/beam/pull/13431#issuecomment-735986540
I've updated the documentation.
Java provides meaning to these specific methods by using annotations, but I still think we want a type in Python (unless we wanted to make *all* DoFns into RestrictionProviders and WatermarkEstimatorProviders, with their corresponding default methods). Not being able to bind these things based on DoFn properties is very constraining though (as you discovered 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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [beam] boyuanzz commented on pull request #13431: [BEAM-11354] Allow DoFn itself to be used as the restriction provider.
Posted by GitBox <gi...@apache.org>.
boyuanzz commented on pull request #13431:
URL: https://github.com/apache/beam/pull/13431#issuecomment-736211057
> When I looked at the programming guide, it pointed to the python docs for the details, which have been updated.
I mean we may also want to provide the code example that is using DoFn as the RestrictionProvider and WatermarkEstimatorProvider.
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [beam] robertwb merged pull request #13431: [BEAM-11354] Allow DoFn itself to be used as the restriction provider.
Posted by GitBox <gi...@apache.org>.
robertwb merged pull request #13431:
URL: https://github.com/apache/beam/pull/13431
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [beam] robertwb commented on pull request #13431: [BEAM-11354] Allow DoFn itself to be used as the restriction provider.
Posted by GitBox <gi...@apache.org>.
robertwb commented on pull request #13431:
URL: https://github.com/apache/beam/pull/13431#issuecomment-736160446
When I looked at the programming guide, it pointed to the python docs for the details, which have been updated.
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [beam] robertwb commented on pull request #13431: [BEAM-11354] Allow DoFn itself to be used as the restriction provider.
Posted by GitBox <gi...@apache.org>.
robertwb commented on pull request #13431:
URL: https://github.com/apache/beam/pull/13431#issuecomment-734006158
Yes, this is more similar to Java. Good point about doing this for the
watermark provider too--I'll do that. (Removing the old way would be
backwards incompatible and a more invasive change.)
On Wed, Nov 25, 2020 at 4:20 PM Boyuan Zhang <no...@github.com>
wrote:
> It seems like we want to do the similar thing to java? Java SDK uses DoFn
> as both RestrictionProvider and WatemarkEstimatorProvider. If so, it
> would be better for python sdk to either do the same thing as java sdk or
> keep using RestrictionTracker and WatermarkEstimatorProvider classes.
>
> —
> You are receiving this because you authored the thread.
> Reply to this email directly, view it on GitHub
> <https://github.com/apache/beam/pull/13431#issuecomment-734004182>, or
> unsubscribe
> <https://github.com/notifications/unsubscribe-auth/AADWVAJXBI64PMALKQCNTF3SRWNORANCNFSM4UDA4RDQ>
> .
>
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [beam] boyuanzz edited a comment on pull request #13431: [BEAM-11354] Allow DoFn itself to be used as the restriction provider.
Posted by GitBox <gi...@apache.org>.
boyuanzz edited a comment on pull request #13431:
URL: https://github.com/apache/beam/pull/13431#issuecomment-734004182
It seems like we want to do the similar thing to java? Java SDK uses `DoFn` as both `RestrictionProvider` and `WatemarkEstimatorProvider`. If so, it would be better for python sdk to either do the same thing as java sdk or keep using `RestrictionTracker` and `WatermarkEstimatorProvider` classes. It might be confusing if we keep both.
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [beam] boyuanzz commented on a change in pull request #13431: [BEAM-11354] Allow DoFn itself to be used as the restriction provider.
Posted by GitBox <gi...@apache.org>.
boyuanzz commented on a change in pull request #13431:
URL: https://github.com/apache/beam/pull/13431#discussion_r532992606
##########
File path: sdks/python/apache_beam/runners/portability/fn_api_runner/fn_runner_test.py
##########
@@ -510,6 +510,21 @@ def process(
actual = (p | beam.Create(data) | beam.ParDo(ExpandingStringsDoFn()))
assert_that(actual, equal_to(list(''.join(data))))
+ def test_sdf_with_dofn_as_restriction_provider(self):
Review comment:
Can we add WatermarkEstimatorProvider for testing 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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [beam] robertwb commented on a change in pull request #13431: [BEAM-11354] Allow DoFn itself to be used as the restriction provider.
Posted by GitBox <gi...@apache.org>.
robertwb commented on a change in pull request #13431:
URL: https://github.com/apache/beam/pull/13431#discussion_r533683975
##########
File path: sdks/python/apache_beam/runners/common.py
##########
@@ -218,10 +218,11 @@ def __init__(self, obj_to_invoke, method_name):
elif core.DoFn.KeyParam == v:
self.key_arg_name = kw
elif isinstance(v, core.DoFn.RestrictionParam):
- self.restriction_provider = v.restriction_provider
+ self.restriction_provider = v.restriction_provider or obj_to_invoke
Review comment:
`v.restriction_provider` will be `None` iff `None` was passed to `DoFn.RestrictionParam` (or omitted, as `None` is the default value).
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [beam] boyuanzz commented on pull request #13431: [BEAM-11354] Allow DoFn itself to be used as the restriction provider.
Posted by GitBox <gi...@apache.org>.
boyuanzz commented on pull request #13431:
URL: https://github.com/apache/beam/pull/13431#issuecomment-734004182
It seems like we want to do the similar thing to java? Java SDK uses `DoFn` as both `RestrictionProvider` and `WatemarkEstimatorProvider`. If so, it would be better for python sdk to either do the same thing as java sdk or keep using `RestrictionTracker` and `WatermarkEstimatorProvider` classes.
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [beam] kennknowles commented on pull request #13431: [BEAM-11354] Allow DoFn itself to be used as the restriction provider.
Posted by GitBox <gi...@apache.org>.
kennknowles commented on pull request #13431:
URL: https://github.com/apache/beam/pull/13431#issuecomment-736016678
Drive by comment: Java users who want to build instances of DoFns programmatically (such as DSL authors) suffer when annotations are the only way to define something. Ideally, we would have both.
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [beam] robertwb commented on pull request #13431: [BEAM-11354] Allow DoFn itself to be used as the restriction provider.
Posted by GitBox <gi...@apache.org>.
robertwb commented on pull request #13431:
URL: https://github.com/apache/beam/pull/13431#issuecomment-733997195
R: @boyuanzz
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org