-
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
Add support for using builtins typing #25054
Add support for using builtins typing #25054
Conversation
Assigning reviewers. If you would like to opt out of this review, comment R: @AnandInguva for label python. Available commands:
The PR bot will only process comments in the main thread (not review comments). |
class TestBuiltinsTyping(unittest.TestCase): | ||
|
||
def _assert_equal_convert_to_beam_type(self, type_a, type_b): | ||
beam_type_a = native_type_compatibility.convert_to_beam_type(type_a) |
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.
These test would fail on Python 3.8 or lower since built-in type modules are supported from Python 3.9.
def test_convert_to_beam_type(self):
> self._assert_equal_convert_to_beam_type(dict[str, int], typing.Dict[str, int])
E TypeError: 'type' object is not subscriptable
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 guess the changes made are compatible with Python 3.9 or above. @jrmccluskey is working on similar(same may be) issue.
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.
The proper way to handle this is to enable the unit tests and the behavior only on Python 3.9 and higher. #24986 does this for the added unit test cases, so this should be okay to do here
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.
@DavidKatz-il Could you rebase on to HEAD and then run the tests?
@DavidKatz-il I was actually working on this and had a short design doc in-flight, but I hadn't started the work on outlining the actual conversion mechanism yet. Would you be interested in writing up your solution in that doc and becoming a co-author? |
Here's a reference for linting and formatting in the Beam repo: https://cwiki.apache.org/confluence/display/BEAM/Python+Tips#PythonTips-LintandFormattingChecks |
@jrmccluskey |
Codecov Report
@@ Coverage Diff @@
## master #25054 +/- ##
==========================================
- Coverage 73.13% 73.13% -0.01%
==========================================
Files 735 735
Lines 98151 98159 +8
==========================================
+ Hits 71782 71784 +2
- Misses 25006 25012 +6
Partials 1363 1363
Flags with carried forward coverage won't be shown. Click here to find out more.
📣 We’re building smart automated test selection to slash your CI/CD build times. Learn more |
Oh you're good! It's the nature of open source, collisions happen. The doc is at https://docs.google.com/document/d/1KgCnYlXDPi2SHtjuzkztUMpFfwgop29WishCI4dHZ30/edit?usp=sharing if you're interested in writing up your solution! Feel free to request editing access. It's a good bridge solution while we support multiple Python versions on each side of the PEP 585 implementation. Eventually we'd have to replace everything with the built-in versions but that's at least two years off. You can remove those error-checking tests since your functionality eliminates the need for that error. |
Run Python_Integration PreCommit |
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.
LGTM, thank you!
fixes #23366
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.