Windowing Support for the DaskRunner #27618
Conversation
- CoGroupByKey is broken due to how tags are used with GroupByKey
- GroupByKey should output `[('0', None), ('1', 1)]`, however it actually outputs: [(None, ('1', 1)), (None, ('0', None))]
- Once that is fixed, we may have test pipelines work on Dask.
…initial tests pass.
This reverts commit 09365f6.
…n infinite loop." This reverts commit 676d752.
|
Status:
This finally feels like it's getting close. (There is still some upstream work on Dask that needs to go in before this, which I'll push forward as well, as mentioned above.) |
|
I have now gotten this as far as it can go without the above-referenced fix in dask going in, so going to shift focus back there, and will re-ping here once that goes in and this is ready for review. |
|
Reminder, please take a look at this pr: @riteshghorse @damccorm |
riteshghorse
added
Next Action: Author
and removed
Next Action: Reviewers
slow-review
labels
|
After a hiatus, I am working on this again now. Today I will complete the upstream fix dask/dask#10734 in dask. Once that goes in, I'll complete this. Thanks all for your patience. |
|
The necessary upstream fix in Dask was merged! 🎉 Once we get a new Dask release that includes this change, I will finish this PR! |
|
This pull request has been marked as stale due to 60 days of inactivity. It will be closed in 1 week if no further activity occurs. If you think that’s incorrect or this pull request requires a review, please simply write any comment. If closed, you can revive the PR at any time and @mention a reviewer or discuss it on the [email protected] list. Thank you for your contributions. |
|
This pull request has been closed due to lack of activity. If you think that is incorrect, or the pull request requires review, you can revive the PR at any time. |
|
Wow, @cisaacstern, you've done a heroic effort in getting this change ready for merge! I was thinking of picking this one back up. Is there anything I should going forward? I think that the upstream bugfix has been released? |
😃 👋 @alxmrs thanks!! Yes, the upstream fix was merged, and then I think the only/primary thing left was just to pin against a newer release of dask, and get tests to pass here. I dropped the ball on this after that upstream fix went in, as other responsibilities arose. I'd love to pass this back to you and see our mutual efforts land in Beam! Let me know if/how I can support you! Good luck! |
This CL adds basic Windowing support to this runner, including a few tests for side inputs.
Take two of #23913. It looks like some of the issues in CI causing this dask change to fail were resolved on their own with changes in master.
Reviewers: @jrmccluskey
Thank you for your contribution! Follow this checklist to help us incorporate your contribution quickly and easily:
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, commentfixes #<ISSUE NUMBER>instead.CHANGES.mdwith noteworthy changes.See the Contributor Guide for more tips on how to make review process smoother.
To check the build health, please visit https://github.com/apache/beam/blob/master/.test-infra/BUILD_STATUS.md
GitHub Actions Tests Status (on master branch)
See CI.md for more information about GitHub Actions CI.