[BEAM-7746] Add python type hints (part 1) #9915
Conversation
|
R: @robertwb |
|
Run Python PreCommit |
|
Just a friendly reminder to nudge this along. |
You're almost 2/3 of the way through. Almost there :) |
|
We're so close. Just a few more. Thanks for your hard work getting through all of this! |
| @@ -505,7 +527,8 @@ def get_window_coder(self): | |||
| return coders.IntervalWindowCoder() | |||
|
|
|||
| def merge(self, merge_context): | |||
| to_merge = [] | |||
| # type: (WindowFn.MergeContext) -> None | |||
| to_merge = [] # type: List[Union[IntervalWindow, GlobalWindow]] | |||
There was a problem hiding this comment.
Changing this and the other use of Union[IntervalWindow, GlobalWindow] to BoundedWindow results in these errors:
apache_beam/transforms/window.py:541: error: "BoundedWindow" has no attribute "start" [attr-defined]
apache_beam/transforms/window.py:543: error: "BoundedWindow" has no attribute "start" [attr-defined]
apache_beam/transforms/window.py:550: error: "BoundedWindow" has no attribute "start" [attr-defined]
apache_beam/transforms/window.py:557: error: "BoundedWindow" has no attribute "start" [attr-defined]
The docs for BoundedWindow say that it's for timestamps in the range (-infinity, end).
How about we add a start property to BoundedWindow:
class BoundedWindow(object):
"""A window for timestamps in range (-infinity, end).
Attributes:
end: End of window.
"""
def __init__(self, end):
# type: (Union[int, float, Timestamp]) -> None
self._end = Timestamp.of(end)
@property
def start(self):
# type: () -> Timestamp
return MAX_TIMESTAMP| @@ -136,14 +140,17 @@ def to_rfc3339(self): | |||
| return self.to_utc_datetime().isoformat() + 'Z' | |||
|
|
|||
| def __float__(self): | |||
| # type: () -> float | |||
There was a problem hiding this comment.
I'm surprised mypy doesn't know these.
There was a problem hiding this comment.
It may, but I didn't test it.
chadrik
force-pushed
the
python-static-typing-part1
branch
6 times, most recently
from
5657f26
to
d7963d4
Compare
|
Run Python PreCommit |
|
@udim pytest is causing failures in the setup of the portable runner tests. It has something to do with the use of I've created a PR to try to modernize and simplify the python build process, especially as it relates to build requirements: #10038. I'm still ironing out some details. Let me know what you think. In the meantime I think we should remove the use of pytest_runner and setup_requires. |
|
Run Python PreCommit |
2 similar comments
|
Run Python PreCommit |
|
Run Python PreCommit |
|
Run Python PreCommit |
chadrik
force-pushed
the
python-static-typing-part1
branch
2 times, most recently
from
c1a02d1
to
7a40e82
Compare
|
Run Java PreCommit |
|
Run Python PreCommit |
|
all tests are passing and there are no conflicts with master! |
|
@robertwb I addressed all of your notes in a final commit so that you can compare them against your notes and ensure they are resolved to your liking. |
|
merge conflicts again.... |
Many of these (those marked with [misc]) are due to mypy taking issue with: - the use of invalid type objects (typehints not typing) - the use of type objects at runtime
`None` is only safe to pass as the context to Coder.to_runner_api() when we know that the Coder class does not have components, otherwise the context is used to find and convert them. We could remove the need for special case `ignores` by reorganizing our Coder class hierarchy to add a CompoundCoder base class or mixin. Right now this is the only place in the code that we need to ignore this problem, so it's not worth tackling now.
This problem will be fixed when the next version of mypy is released
The test runs but error status is ignored.
This resolves a pip conflict that was happening between setupVirtualenv task and the tox task by ensuring that everything agrees on the same set of versions.
There are no meaningful runtime changes in this commit.
This seems to be fine to do on an instance as long as the types match, but not on a class.
chadrik
force-pushed
the
python-static-typing-part1
branch
from
c32cf2e
to
5c48091
Compare
|
Run Java PreCommit |
|
Run Python PreCommit |
1 similar comment
|
Run Python PreCommit |
|
I believe this PR broke :sdks:python:test-suites:direct:py37:hdfsIntegrationTest |
|
Is that not part of the precommit suite? The tests for this PR definitely
ran and passed. The traceback seemed to indicate that google.protobuf is
not installed...
…On Fri, Dec 13, 2019 at 11:31 AM Udi Meiri ***@***.***> wrote:
I believe this PR broke
:sdks:python:test-suites:direct:py37:hdfsIntegrationTest
Opened https://issues.apache.org/jira/browse/BEAM-8966
—
You are receiving this because you modified the open/close state.
Reply to this email directly, view it on GitHub
<#9915?email_source=notifications&email_token=AAAPOE3TOM63EE2P72YWOTTQYPPHNA5CNFSM4JGC3N42YY3PNVWWK3TUL52HS4DFVREXG43VMVBW63LNMVXHJKTDN5WW2ZLOORPWSZGOEG3AO5Y#issuecomment-565577591>,
or unsubscribe
<https://github.com/notifications/unsubscribe-auth/AAAPOE3HEVMZEZOXV2MHHM3QYPPHNANCNFSM4JGC3N4Q>
.
|
Not it is not part of precommits. |
* Address issues with a few missing modules/objects * Ignore some objects missing from typeshed * Ignore an issue regarding asymmetrical property getter/setter * Ignore unavoidable errors Many of these (those marked with [misc]) are due to mypy taking issue with: - the use of invalid type objects (typehints not typing) - the use of type objects at runtime * Ignore some conditional imports in tests * Ignore functions missing from typeshed * Add tests for Timestamp/Duration comparisons * Ignore errors about dynamic base classes This problem will be fixed when the next version of mypy is released * Ignore some monkey-patching for tests * Consolidate python build requirements This resolves a pip conflict that was happening between setupVirtualenv task and the tox task by ensuring that everything agrees on the same set of versions. * Ignore a difficult scenario where None is sometimes unsafe `None` is only safe to pass as the context to Coder.to_runner_api() when we know that the Coder class does not have components, otherwise the context is used to find and convert them. We could remove the need for special case `ignores` by reorganizing our Coder class hierarchy to add a CompoundCoder base class or mixin. Right now this is the only place in the code that we need to ignore this problem, so it's not worth tackling now. * Add a mypy lint job The test runs but error status is ignored. * Workaround a bug in pylint pylint-dev/pylint#3217 * Add type annotations to bulk of python codebase There are no meaningful runtime changes in this commit. * Silence errors about overriding methods. This seems to be fine to do on an instance as long as the types match, but not on a class. * Address review notes * Rebase fixup
| @@ -178,6 +179,7 @@ indent-after-paren=4 | |||
| ignore-long-lines=(?x) | |||
| (^\s*(import|from)\s | |||
| |^\s*(\#\ )?<?(https?|ftp):\/\/[^\s\/$.?#].[^\s]*>?$ | |||
| |# type: | |||
There was a problem hiding this comment.
This line disabled line-too-long errors across the board: https://issues.apache.org/jira/browse/BEAM-9058.
This is part 1 of #9056
Thank you for your contribution! Follow this checklist to help us incorporate your contribution quickly and easily:
R: @username).[BEAM-XXX] Fixes bug in ApproximateQuantiles, where you replaceBEAM-XXXwith the appropriate JIRA issue, if applicable. This will automatically link the pull request to the issue.See the Contributor Guide for more tips on how to make review process smoother.
Post-Commit Tests Status (on master branch)
Pre-Commit Tests Status (on master branch)
See .test-infra/jenkins/README for trigger phrase, status and link of all Jenkins jobs.