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/06/24 16:29:00 UTC
[GitHub] [beam] InigoSJ opened a new pull request #12074: [BEAM-10313] Add without_defaults to built-in Combiners in Python
InigoSJ opened a new pull request #12074:
URL: https://github.com/apache/beam/pull/12074
Following with [BEAM-10209](https://issues.apache.org/jira/browse/BEAM-10209) that fixed `Mean`
When need Windows and built-in combiners in a Globally way, we cannot specify `without_defaults()`, forcing us to use the workaround as `CombineGlobally(CountCombineFn()).without_defaults()`. This PR fix this, adding the `without_defaults()` to built-in Combiners in Python
------------------------
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
--- | --- | --- | --- | --- | ---
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_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/)
Python | [](https://ci-beam.apache.org/job/beam_PostCommit_Python2/lastCompletedBuild/)<br>[](https://ci-beam.apache.org/job/beam_PostCommit_Python35/lastCompletedBuild/)<br>[](https://ci-beam.apache.org/job/beam_PostCommit_Python36/lastCompletedBuild/)<br>[](https://ci-beam.apache.org/job/beam_PostCommit_Python37/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_Python2_PVR_Flink_Cron/lastCompletedBuild/)<br>[](https://ci-beam.apache.org/job/beam_PostCommit_Python35_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_Flink/lastCompletedBuild/) | --- | --- | [](https://ci-beam.apache.org/job/beam_PostCommit_XVR_Spark/lastCompletedBuild/)
Pre-Commit Tests Status (on master branch)
------------------------------------------------------------------------------------------------
--- |Java | Python | Go | Website
--- | --- | --- | --- | ---
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/) | [](https://ci-beam.apache.org/job/beam_PreCommit_Go_Cron/lastCompletedBuild/) | [](https://ci-beam.apache.org/job/beam_PreCommit_Website_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.
----------------------------------------------------------------
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] InigoSJ commented on pull request #12074: [BEAM-10313] Add without_defaults to built-in Combiners in Python
Posted by GitBox <gi...@apache.org>.
InigoSJ commented on pull request #12074:
URL: https://github.com/apache/beam/pull/12074#issuecomment-658268570
retest this please
----------------------------------------------------------------
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] InigoSJ commented on pull request #12074: [BEAM-10313] Add without_defaults to built-in Combiners in Python
Posted by GitBox <gi...@apache.org>.
InigoSJ commented on pull request #12074:
URL: https://github.com/apache/beam/pull/12074#issuecomment-658244541
> I don't fully understand the errors that the checks have. It looks the error `Failed assert: ['a'] == ['a', 'b']` is the only one but repeated. Not sure how my code can affect that if the Combiners_test works fine. I'm going to retest just to be sure.
OK, I ran the test on Master and the same error appeared. Checking other PR, I see the same issue ([example](https://github.com/apache/beam/pull/12230).
For the Python2_PVR_Flink error (SNAPSHOT 2.24.0), it looks it's happening [everywhere](https://ci-beam.apache.org/job/beam_PreCommit_Python2_PVR_Flink_Cron/lastCompletedBuild/)
----------------------------------------------------------------
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] InigoSJ commented on pull request #12074: [BEAM-10313] Add without_defaults to built-in Combiners in Python
Posted by GitBox <gi...@apache.org>.
InigoSJ commented on pull request #12074:
URL: https://github.com/apache/beam/pull/12074#issuecomment-659289396
All checks passed, changing from Draft to PR.
----------------------------------------------------------------
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] pabloem commented on a change in pull request #12074: [BEAM-10313] Add without_defaults to built-in Combiners in Python
Posted by GitBox <gi...@apache.org>.
pabloem commented on a change in pull request #12074:
URL: https://github.com/apache/beam/pull/12074#discussion_r447317739
##########
File path: sdks/python/apache_beam/transforms/combiners.py
##########
@@ -62,26 +62,28 @@
TimestampType = Union[int, float, Timestamp, Duration]
+class CombinerWithoutDefaults(ptransform.PTransform):
+ def __init__(self, has_defaults=True):
+ self.has_defaults = has_defaults
+
+ def with_defaults(self, has_defaults=True):
+ self.has_defaults = has_defaults
+ return self
Review comment:
In the spirit of not modifying self, and instead creating a new instance, you can return the subclass:
```suggestion
def with_defaults(self, has_defaults=True):
new = copy.copy(self)
new.has_defaults = has_defaults
return new
```
----------------------------------------------------------------
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] pabloem merged pull request #12074: [BEAM-10313] Add without_defaults to built-in Combiners in Python
Posted by GitBox <gi...@apache.org>.
pabloem merged pull request #12074:
URL: https://github.com/apache/beam/pull/12074
----------------------------------------------------------------
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] InigoSJ commented on a change in pull request #12074: [BEAM-10313] Add without_defaults to built-in Combiners in Python
Posted by GitBox <gi...@apache.org>.
InigoSJ commented on a change in pull request #12074:
URL: https://github.com/apache/beam/pull/12074#discussion_r454285726
##########
File path: sdks/python/apache_beam/transforms/combiners.py
##########
@@ -62,26 +62,28 @@
TimestampType = Union[int, float, Timestamp, Duration]
+class CombinerWithoutDefaults(ptransform.PTransform):
+ def __init__(self, has_defaults=True):
+ self.has_defaults = has_defaults
+
+ def with_defaults(self, has_defaults=True):
+ self.has_defaults = has_defaults
+ return self
Review comment:
This looks very nice, I wanted to return a new instance by wasn't sure how, copy is a very clever way, thanks.
Commited
----------------------------------------------------------------
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] InigoSJ commented on pull request #12074: [BEAM-10313] Add without_defaults to built-in Combiners in Python
Posted by GitBox <gi...@apache.org>.
InigoSJ commented on pull request #12074:
URL: https://github.com/apache/beam/pull/12074#issuecomment-658219423
I don't fully understand the errors that the checks have. It looks the error `Failed assert: ['a'] == ['a', 'b']` is the only one but repeated. Not sure how my code can affect that if the Combiners_test works fine. I'm going to retest just to be sure.
----------------------------------------------------------------
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] pabloem commented on pull request #12074: [BEAM-10313] Add without_defaults to built-in Combiners in Python
Posted by GitBox <gi...@apache.org>.
pabloem commented on pull request #12074:
URL: https://github.com/apache/beam/pull/12074#issuecomment-658915637
Run Python2_PVR_Flink PreCommit
----------------------------------------------------------------
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] InigoSJ commented on pull request #12074: [BEAM-10313] Add without_defaults to built-in Combiners in Python
Posted by GitBox <gi...@apache.org>.
InigoSJ commented on pull request #12074:
URL: https://github.com/apache/beam/pull/12074#issuecomment-658219720
retest this please
----------------------------------------------------------------
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] InigoSJ commented on pull request #12074: [BEAM-10313] Add without_defaults to built-in Combiners in Python
Posted by GitBox <gi...@apache.org>.
InigoSJ commented on pull request #12074:
URL: https://github.com/apache/beam/pull/12074#issuecomment-648932828
@pabloem
Some questions that I would need help with:
1 - Is the name `CombinerWithoutDefaults` good enough for the parent class? Am I adding it in the right place?
2 - All my tests passed, but just to double check, am I missing any `*args, **kwargs` anywhere?
3 - I needed to add `timestamp = 0` in many tests, should I do it as a global variable? maybe just use 0 without referencing `timestamp`?
4 - Assert at *795*: I used a bigger window (180) so that there's 2 outputs. Should I keep it as the previous assert that doesn't have windows (i.e., with one output element)?
5 - Checking the file, I don't see any pydoc that's needed, but maybe you think I should add something
Thanks a lot for your help!
----------------------------------------------------------------
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] InigoSJ commented on pull request #12074: [BEAM-10313] Add without_defaults to built-in Combiners in Python
Posted by GitBox <gi...@apache.org>.
InigoSJ commented on pull request #12074:
URL: https://github.com/apache/beam/pull/12074#issuecomment-659199532
Run Python PreCommit
----------------------------------------------------------------
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