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/09/01 00:09:39 UTC
[GitHub] [beam] ryanthompson591 opened a new pull request, #22984: Changes CoGroupByKey typehint from List to Iterable
ryanthompson591 opened a new pull request, #22984:
URL: https://github.com/apache/beam/pull/22984
Changes CoGroupByKey to output an Iterable instead of a List.
This will be a breaking change and I put together [a document](https://docs.google.com/document/d/1RIzm8-g-0CyVsPb6yasjwokJQFoKHG4NjRUcKHKINu0/edit#) to describe it.
The documentation of CoGroupByKey is very specific that it returns iterable and not a lists.
------------------------
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`).
- [ ] Mention the appropriate issue in your description (for example: `addresses #123`), if applicable. This will automatically add a link to the pull request in the issue. If you would like the issue to automatically close on merging the pull request, comment `fixes #<ISSUE NUMBER>` instead.
- [ ] 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/get-started-contributing/#make-the-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)
------------------------------------------------------------------------------------------------
[](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)
[](https://github.com/apache/beam/actions?query=workflow%3A%22Go+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] codecov[bot] commented on pull request #22984: Changes CoGroupByKey typehint from List to Iterable
Posted by GitBox <gi...@apache.org>.
codecov[bot] commented on PR #22984:
URL: https://github.com/apache/beam/pull/22984#issuecomment-1233603234
# [Codecov](https://codecov.io/gh/apache/beam/pull/22984?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 [#22984](https://codecov.io/gh/apache/beam/pull/22984?src=pr&el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation) (a09cdfc) into [master](https://codecov.io/gh/apache/beam/commit/d615b624e9ff211e857d026d541c4d56fd18e2d3?el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation) (d615b62) will **increase** coverage by `0.00%`.
> The diff coverage is `n/a`.
```diff
@@ Coverage Diff @@
## master #22984 +/- ##
========================================
Coverage 73.69% 73.70%
========================================
Files 713 714 +1
Lines 94988 95140 +152
========================================
+ Hits 70005 70123 +118
- Misses 23682 23716 +34
Partials 1301 1301
```
| Flag | Coverage Δ | |
|---|---|---|
| python | `83.51% <ø> (-0.02%)` | :arrow_down: |
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/22984?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/python/apache\_beam/transforms/util.py](https://codecov.io/gh/apache/beam/pull/22984/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-c2Rrcy9weXRob24vYXBhY2hlX2JlYW0vdHJhbnNmb3Jtcy91dGlsLnB5) | `96.06% <ø> (ø)` | |
| [sdks/python/apache\_beam/typehints/\_\_init\_\_.py](https://codecov.io/gh/apache/beam/pull/22984/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-c2Rrcy9weXRob24vYXBhY2hlX2JlYW0vdHlwZWhpbnRzL19faW5pdF9fLnB5) | `77.77% <0.00%> (-22.23%)` | :arrow_down: |
| [sdks/python/apache\_beam/io/source\_test\_utils.py](https://codecov.io/gh/apache/beam/pull/22984/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-c2Rrcy9weXRob24vYXBhY2hlX2JlYW0vaW8vc291cmNlX3Rlc3RfdXRpbHMucHk=) | `88.01% <0.00%> (-1.39%)` | :arrow_down: |
| [sdks/python/apache\_beam/dataframe/schemas.py](https://codecov.io/gh/apache/beam/pull/22984/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-c2Rrcy9weXRob24vYXBhY2hlX2JlYW0vZGF0YWZyYW1lL3NjaGVtYXMucHk=) | `96.62% <0.00%> (-1.05%)` | :arrow_down: |
| [...hon/apache\_beam/runners/worker/bundle\_processor.py](https://codecov.io/gh/apache/beam/pull/22984/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-c2Rrcy9weXRob24vYXBhY2hlX2JlYW0vcnVubmVycy93b3JrZXIvYnVuZGxlX3Byb2Nlc3Nvci5weQ==) | `93.30% <0.00%> (-0.38%)` | :arrow_down: |
| [sdks/python/apache\_beam/runners/common.py](https://codecov.io/gh/apache/beam/pull/22984/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-c2Rrcy9weXRob24vYXBhY2hlX2JlYW0vcnVubmVycy9jb21tb24ucHk=) | `88.59% <0.00%> (-0.13%)` | :arrow_down: |
| [...thon/apache\_beam/ml/inference/pytorch\_inference.py](https://codecov.io/gh/apache/beam/pull/22984/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-c2Rrcy9weXRob24vYXBhY2hlX2JlYW0vbWwvaW5mZXJlbmNlL3B5dG9yY2hfaW5mZXJlbmNlLnB5) | `0.00% <0.00%> (ø)` | |
| [...apache\_beam/typehints/pandas\_type\_compatibility.py](https://codecov.io/gh/apache/beam/pull/22984/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-c2Rrcy9weXRob24vYXBhY2hlX2JlYW0vdHlwZWhpbnRzL3BhbmRhc190eXBlX2NvbXBhdGliaWxpdHkucHk=) | `94.95% <0.00%> (ø)` | |
| [...eam/runners/interactive/interactive\_environment.py](https://codecov.io/gh/apache/beam/pull/22984/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-c2Rrcy9weXRob24vYXBhY2hlX2JlYW0vcnVubmVycy9pbnRlcmFjdGl2ZS9pbnRlcmFjdGl2ZV9lbnZpcm9ubWVudC5weQ==) | `92.02% <0.00%> (+0.30%)` | :arrow_up: |
| [...hon/apache\_beam/runners/direct/test\_stream\_impl.py](https://codecov.io/gh/apache/beam/pull/22984/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-c2Rrcy9weXRob24vYXBhY2hlX2JlYW0vcnVubmVycy9kaXJlY3QvdGVzdF9zdHJlYW1faW1wbC5weQ==) | `94.02% <0.00%> (+0.74%)` | :arrow_up: |
| ... and [4 more](https://codecov.io/gh/apache/beam/pull/22984/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) | |
:mega: We’re building smart automated test selection to slash your CI/CD build times. [Learn more](https://about.codecov.io/iterative-testing/?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
[GitHub] [beam] github-actions[bot] commented on pull request #22984: Changes CoGroupByKey typehint from List to Iterable
Posted by GitBox <gi...@apache.org>.
github-actions[bot] commented on PR #22984:
URL: https://github.com/apache/beam/pull/22984#issuecomment-1234569510
Stopping reviewer notifications for this pull request: review requested by someone other than the bot, ceding control
--
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 pull request #22984: Changes CoGroupByKey typehint from List to Iterable
Posted by GitBox <gi...@apache.org>.
damccorm commented on PR #22984:
URL: https://github.com/apache/beam/pull/22984#issuecomment-1245552611
> Fixes issue #21556
I think for this to take effect it has to be in the PR description (which you can edit to include it) - https://docs.github.com/en/issues/tracking-your-work-with-issues/linking-a-pull-request-to-an-issue#linking-a-pull-request-to-an-issue-using-a-keyword
--
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] ryanthompson591 commented on a diff in pull request #22984: Changes CoGroupByKey typehint from List to Iterable
Posted by GitBox <gi...@apache.org>.
ryanthompson591 commented on code in PR #22984:
URL: https://github.com/apache/beam/pull/22984#discussion_r972405446
##########
CHANGES.md:
##########
@@ -66,7 +66,7 @@
## Breaking Changes
-* X behavior was changed ([#X](https://github.com/apache/beam/issues/X)).
+* Python SDK CoGroupByKey outputs an iterable allowing for arbitrarily large results. [#21556](https://github.com/apache/beam/issues/21556) See [document](https://docs.google.com/document/d/1RIzm8-g-0CyVsPb6yasjwokJQFoKHG4NjRUcKHKINu0) for suggested fixes for different error cases that assumed a list type was returned.
Review Comment:
```suggestion
* Python SDK CoGroupByKey outputs an iterable allowing for arbitrarily large results. [#21556](https://github.com/apache/beam/issues/21556) Beam users maybe see a type error or a run time on transforms downstream from CoGroupByKey. To fix, change methods expecting a List to expect an Iterable. See [document](https://docs.google.com/document/d/1RIzm8-g-0CyVsPb6yasjwokJQFoKHG4NjRUcKHKINu0) for suggested fixes for different error cases that assumed a list type was returned.
```
--
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] ryanthompson591 commented on a diff in pull request #22984: Changes CoGroupByKey typehint from List to Iterable
Posted by GitBox <gi...@apache.org>.
ryanthompson591 commented on code in PR #22984:
URL: https://github.com/apache/beam/pull/22984#discussion_r980404789
##########
CHANGES.md:
##########
@@ -66,7 +66,7 @@
## Breaking Changes
-* X behavior was changed ([#X](https://github.com/apache/beam/issues/X)).
+* Python SDK CoGroupByKey now outputs an iterable instead of a list. [#21556](https://github.com/apache/beam/issues/21556)
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.
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 #22984: Changes CoGroupByKey typehint from List to Iterable
Posted by GitBox <gi...@apache.org>.
github-actions[bot] commented on PR #22984:
URL: https://github.com/apache/beam/pull/22984#issuecomment-1233632091
Assigning reviewers. If you would like to opt out of this review, comment `assign to next reviewer`:
R: @TheNeuralBit for label python.
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] ryanthompson591 commented on pull request #22984: Changes CoGroupByKey typehint from List to Iterable
Posted by GitBox <gi...@apache.org>.
ryanthompson591 commented on PR #22984:
URL: https://github.com/apache/beam/pull/22984#issuecomment-1245726917
Run Python 3.9 PostCommit
--
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] ryanthompson591 commented on pull request #22984: Changes CoGroupByKey typehint from List to Iterable
Posted by GitBox <gi...@apache.org>.
ryanthompson591 commented on PR #22984:
URL: https://github.com/apache/beam/pull/22984#issuecomment-1234545744
@TheNeuralBit Thanks I'll update the doc to reflect that, discouraging changing the iterable to a list.
--
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] ryanthompson591 commented on a diff in pull request #22984: Changes CoGroupByKey typehint from List to Iterable
Posted by GitBox <gi...@apache.org>.
ryanthompson591 commented on code in PR #22984:
URL: https://github.com/apache/beam/pull/22984#discussion_r972405446
##########
CHANGES.md:
##########
@@ -66,7 +66,7 @@
## Breaking Changes
-* X behavior was changed ([#X](https://github.com/apache/beam/issues/X)).
+* Python SDK CoGroupByKey outputs an iterable allowing for arbitrarily large results. [#21556](https://github.com/apache/beam/issues/21556) See [document](https://docs.google.com/document/d/1RIzm8-g-0CyVsPb6yasjwokJQFoKHG4NjRUcKHKINu0) for suggested fixes for different error cases that assumed a list type was returned.
Review Comment:
```suggestion
* Python SDK CoGroupByKey outputs an iterable allowing for arbitrarily large results. [#21556](https://github.com/apache/beam/issues/21556) Beam users may see a type or other runtime error on transforms downstream from CoGroupByKey. Users must change methods expecting a List to expect an Iterable going forward. See [document](https://docs.google.com/document/d/1RIzm8-g-0CyVsPb6yasjwokJQFoKHG4NjRUcKHKINu0) for information and fixes.
```
--
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] ryanthompson591 commented on a diff in pull request #22984: Changes CoGroupByKey typehint from List to Iterable
Posted by GitBox <gi...@apache.org>.
ryanthompson591 commented on code in PR #22984:
URL: https://github.com/apache/beam/pull/22984#discussion_r974693970
##########
CHANGES.md:
##########
@@ -66,7 +66,7 @@
## Breaking Changes
-* X behavior was changed ([#X](https://github.com/apache/beam/issues/X)).
+* Python SDK CoGroupByKey outputs an iterable allowing for arbitrarily large results. [#21556](https://github.com/apache/beam/issues/21556) Beam users may see a type or other runtime error on transforms downstream from CoGroupByKey. Users must change methods expecting a List to expect an Iterable going forward. See [document](https://docs.google.com/document/d/1RIzm8-g-0CyVsPb6yasjwokJQFoKHG4NjRUcKHKINu0) for information and fixes.
Review Comment:
```suggestion
* Python SDK CoGroupByKey outputs an iterable allowing for arbitrarily large results. [#21556](https://github.com/apache/beam/issues/21556) Beam users may see an error on transforms downstream from CoGroupByKey. Users must change methods expecting a List to expect an Iterable going forward. See [document](https://docs.google.com/document/d/1RIzm8-g-0CyVsPb6yasjwokJQFoKHG4NjRUcKHKINu0) for information and fixes.
```
--
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] TheNeuralBit commented on pull request #22984: Changes CoGroupByKey typehint from List to Iterable
Posted by GitBox <gi...@apache.org>.
TheNeuralBit commented on PR #22984:
URL: https://github.com/apache/beam/pull/22984#issuecomment-1234508177
@ryanthompson591 I can't comment on your doc so I'll mention here - it encourages users to materialize the iterable result to a list, which can lead to OOMs. I think it's ok to mention casting to a list as a last resort, but it should also encourage users to iterate over the iterable without materializing it if at all possible.
--
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] ryanthompson591 commented on pull request #22984: Changes CoGroupByKey typehint from List to Iterable
Posted by GitBox <gi...@apache.org>.
ryanthompson591 commented on PR #22984:
URL: https://github.com/apache/beam/pull/22984#issuecomment-1233578413
R: tvalentyn
I want to talk about this before I submit. The coGroupByKey implementation specifically does make a dictionary with a [Key to list pairing](https://github.com/apache/beam/blob/cebf7388fc7a63f138f194ec0ed39e882ebfc72c/sdks/python/apache_beam/transforms/util.py#L245).
However, the documentation is very explicit that [users should expect an iterable](https://github.com/apache/beam/blob/cebf7388fc7a63f138f194ec0ed39e882ebfc72c/sdks/python/apache_beam/transforms/util.py#L148).
I want to double check one last time we are not making a breaking change unless we have to.
--
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] ryanthompson591 commented on pull request #22984: Changes CoGroupByKey typehint from List to Iterable
Posted by GitBox <gi...@apache.org>.
ryanthompson591 commented on PR #22984:
URL: https://github.com/apache/beam/pull/22984#issuecomment-1245590355
@lukecwik I updated the CHANGES.md file. I think that since the 2.42 release is cut, it is now a good time to add this change.
--
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] ryanthompson591 commented on pull request #22984: Changes CoGroupByKey typehint from List to Iterable
Posted by GitBox <gi...@apache.org>.
ryanthompson591 commented on PR #22984:
URL: https://github.com/apache/beam/pull/22984#issuecomment-1245592680
Run Python 3.9 PostCommit
--
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 #22984: Changes CoGroupByKey typehint from List to Iterable
Posted by GitBox <gi...@apache.org>.
damccorm commented on code in PR #22984:
URL: https://github.com/apache/beam/pull/22984#discussion_r969750281
##########
CHANGES.md:
##########
@@ -66,7 +66,7 @@
## Breaking Changes
-* X behavior was changed ([#X](https://github.com/apache/beam/issues/X)).
+* Python SDK CoGroupByKey now outputs an iterable instead of a list. [#21556](https://github.com/apache/beam/issues/21556)
Review Comment:
It would be nice to link to your docs on how to fix upgrade issues here as well (https://docs.google.com/document/d/1RIzm8-g-0CyVsPb6yasjwokJQFoKHG4NjRUcKHKINu0/edit)
--
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] ryanthompson591 commented on pull request #22984: Changes CoGroupByKey typehint from List to Iterable
Posted by GitBox <gi...@apache.org>.
ryanthompson591 commented on PR #22984:
URL: https://github.com/apache/beam/pull/22984#issuecomment-1258486137
Run Portable_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.
To unsubscribe, e-mail: github-unsubscribe@beam.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [beam] TheNeuralBit commented on pull request #22984: Changes CoGroupByKey typehint from List to Iterable
Posted by GitBox <gi...@apache.org>.
TheNeuralBit commented on PR #22984:
URL: https://github.com/apache/beam/pull/22984#issuecomment-1234502971
R: @tvalentyn
CC: @robertwb
--
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] ryanthompson591 commented on a diff in pull request #22984: Changes CoGroupByKey typehint from List to Iterable
Posted by GitBox <gi...@apache.org>.
ryanthompson591 commented on code in PR #22984:
URL: https://github.com/apache/beam/pull/22984#discussion_r972405446
##########
CHANGES.md:
##########
@@ -66,7 +66,7 @@
## Breaking Changes
-* X behavior was changed ([#X](https://github.com/apache/beam/issues/X)).
+* Python SDK CoGroupByKey outputs an iterable allowing for arbitrarily large results. [#21556](https://github.com/apache/beam/issues/21556) See [document](https://docs.google.com/document/d/1RIzm8-g-0CyVsPb6yasjwokJQFoKHG4NjRUcKHKINu0) for suggested fixes for different error cases that assumed a list type was returned.
Review Comment:
```suggestion
* Python SDK CoGroupByKey outputs an iterable allowing for arbitrarily large results. [#21556](https://github.com/apache/beam/issues/21556) Beam users may see a type or runtime error on transforms downstream from CoGroupByKey. Change methods expecting a List to expect an Iterable going forward. See [document](https://docs.google.com/document/d/1RIzm8-g-0CyVsPb6yasjwokJQFoKHG4NjRUcKHKINu0) for information and fixes.
```
--
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] ryanthompson591 commented on pull request #22984: Changes CoGroupByKey typehint from List to Iterable
Posted by GitBox <gi...@apache.org>.
ryanthompson591 commented on PR #22984:
URL: https://github.com/apache/beam/pull/22984#issuecomment-1251547145
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.
To unsubscribe, e-mail: github-unsubscribe@beam.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [beam] lukecwik commented on a diff in pull request #22984: Changes CoGroupByKey typehint from List to Iterable
Posted by GitBox <gi...@apache.org>.
lukecwik commented on code in PR #22984:
URL: https://github.com/apache/beam/pull/22984#discussion_r969832917
##########
CHANGES.md:
##########
@@ -66,7 +66,7 @@
## Breaking Changes
-* X behavior was changed ([#X](https://github.com/apache/beam/issues/X)).
+* Python SDK CoGroupByKey now outputs an iterable instead of a list. [#21556](https://github.com/apache/beam/issues/21556) See [document](https://docs.google.com/document/d/1RIzm8-g-0CyVsPb6yasjwokJQFoKHG4NjRUcKHKINu0) for suggested fixes for different error cases.
Review Comment:
```suggestion
* Python SDK CoGroupByKey outputs an iterable allowing for arbitrarily large results. [#21556](https://github.com/apache/beam/issues/21556) See [document](https://docs.google.com/document/d/1RIzm8-g-0CyVsPb6yasjwokJQFoKHG4NjRUcKHKINu0) for suggested fixes for different error cases that assumed a list type was returned.
```
--
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] ryanthompson591 commented on pull request #22984: Changes CoGroupByKey typehint from List to Iterable
Posted by GitBox <gi...@apache.org>.
ryanthompson591 commented on PR #22984:
URL: https://github.com/apache/beam/pull/22984#issuecomment-1245726223
Thanks for the comments, I think I address the major concerns and added some new sample code.
Feel free to request edit access to that doc.
--
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] ryanthompson591 commented on pull request #22984: Changes CoGroupByKey typehint from List to Iterable
Posted by GitBox <gi...@apache.org>.
ryanthompson591 commented on PR #22984:
URL: https://github.com/apache/beam/pull/22984#issuecomment-1245541226
Fixes issue https://github.com/apache/beam/issues/21556
--
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] ryanthompson591 commented on a diff in pull request #22984: Changes CoGroupByKey typehint from List to Iterable
Posted by GitBox <gi...@apache.org>.
ryanthompson591 commented on code in PR #22984:
URL: https://github.com/apache/beam/pull/22984#discussion_r972405446
##########
CHANGES.md:
##########
@@ -66,7 +66,7 @@
## Breaking Changes
-* X behavior was changed ([#X](https://github.com/apache/beam/issues/X)).
+* Python SDK CoGroupByKey outputs an iterable allowing for arbitrarily large results. [#21556](https://github.com/apache/beam/issues/21556) See [document](https://docs.google.com/document/d/1RIzm8-g-0CyVsPb6yasjwokJQFoKHG4NjRUcKHKINu0) for suggested fixes for different error cases that assumed a list type was returned.
Review Comment:
```suggestion
* Python SDK CoGroupByKey outputs an iterable allowing for arbitrarily large results. [#21556](https://github.com/apache/beam/issues/21556) Beam users may see a type or runtime error on transforms downstream from CoGroupByKey. Change methods expecting a List to expect an Iterable going forward. See [document](https://docs.google.com/document/d/1RIzm8-g-0CyVsPb6yasjwokJQFoKHG4NjRUcKHKINu0) for more information and ways to fix these errors.
```
--
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] TheNeuralBit commented on pull request #22984: Changes CoGroupByKey typehint from List to Iterable
Posted by GitBox <gi...@apache.org>.
TheNeuralBit commented on PR #22984:
URL: https://github.com/apache/beam/pull/22984#issuecomment-1234509219
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.
To unsubscribe, e-mail: github-unsubscribe@beam.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [beam] tvalentyn commented on pull request #22984: Changes CoGroupByKey typehint from List to Iterable
Posted by GitBox <gi...@apache.org>.
tvalentyn commented on PR #22984:
URL: https://github.com/apache/beam/pull/22984#issuecomment-1258894629
LGTM
--
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] tvalentyn merged pull request #22984: Changes CoGroupByKey typehint from List to Iterable
Posted by GitBox <gi...@apache.org>.
tvalentyn merged PR #22984:
URL: https://github.com/apache/beam/pull/22984
--
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