-
Notifications
You must be signed in to change notification settings - Fork 4.3k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Update python transform catalog #30788
Conversation
Assigning reviewers. If you would like to opt out of this review, comment R: @damccorm for label python. Available commands:
The PR bot will only process comments in the main thread (not review comments). |
I'm struggling with the linter. Is there a good page for setting it up somewhere? |
Could you take a look at https://cwiki.apache.org/confluence/display/BEAM/Python+Tips#PythonTips-LintandFormattingChecks |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for doing this! I had some questions/thoughts, but think this is great overall!
Also, btw in case you haven't noticed this, beam generates a staged version of the website on PRs as part of an action - see the link here for example https://github.com/apache/beam/actions/runs/8532860668?pr=30788 - you may have seen it already, but wanted to call it out just in case
sdks/python/apache_beam/examples/snippets/transforms/aggregation/batchelements_test.py
Outdated
Show resolved
Hide resolved
sdks/python/apache_beam/examples/snippets/transforms/aggregation/tolist_test.py
Outdated
Show resolved
Hide resolved
|
||
{{< playground height="700px" >}} | ||
{{< playground_snippet language="py" path="SDK_PYTHON_ApproximateQuantiles" show="approximatequantiles" >}} | ||
{{< /playground >}} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think these won't render until the next playground release. So it may make sense to split this into 2 PRs, one for now and one for once the playground piece is released after 2.56
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ah, so the website can get upgraded within a release but playground gets redeployed with releases.
It's all a bit tangled and I'm not sure I have the time to split it apart. Would it be alright to just keep the PR as is and merge whenever it makes sense? (assuming there is such a moment)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There's not a great single time to merge since we'd want the playground on the release branch, so we'd want to merge the playground fixes, wait a week or so for the release, then merge the website changes.
With that said, if you definitely don't have time we can merge right before the release branch is cut (I'm running the release so should be easy for me to pick up) and the website will just be broken for a bit in these code samples. It isn't ideal, but would probably still be better than not doing this at all.
Up to you on how to proceed here, I'll add this as a release blocker so I remember to merge though
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Outside of the linting failure the changes look good to me, so just let me know how you'd prefer to proceed
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@hjtran I added a few suggestions that I think will clean up the linting failures based on the logs. I'm going to batch commit them to see if it helps since I'd love to get this in before the release branch cut tomorrow
sdks/python/apache_beam/examples/snippets/transforms/aggregation/approximatequantiles_test.py
Outdated
Show resolved
Hide resolved
sdks/python/apache_beam/examples/snippets/transforms/aggregation/approximateunique_test.py
Outdated
Show resolved
Hide resolved
sdks/python/apache_beam/examples/snippets/transforms/aggregation/batchelements_test.py
Outdated
Show resolved
Hide resolved
sdks/python/apache_beam/examples/snippets/transforms/aggregation/tolist_test.py
Outdated
Show resolved
Hide resolved
SGTM, thanks @damccorm ! Sorry I didn't have time to take another look at this last week. |
No problem - looks like it did fix the lint checks, so I'll merge once the coverage check finishes (assuming nothing broke unexpectedly) |
Only test failure is |
The python transform catalog page is a bit out of date and includes mentions of transforms that don't exist (PAssert, CombineWithContext, View) and outdated notes about transforms that supposedly haven't been implemented (ApproximateUnique, ApproximateQuantiles).
I've updated the page, removing the unavailable transforms since I'm not sure it's worth mentioning (but I may be missing context) and updated the Approximate* transform entries as well as adding examples for them.
BatchElements
also feels like a very useful transform that I've been using personally so I also added it to the catalog.Example 2 of GroupByKey was also not showing any python

So I tried fixing that as well
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.md
with 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 or the workflows README to see a list of phrases to trigger workflows.