[BEAM-7746] Add typing for try_split #10593
Conversation
chadrik
force-pushed
the
python-typing-try-split
branch
3 times, most recently
from
cafbbf5
to
6abebd9
Compare
| @@ -61,6 +61,9 @@ | |||
| from apache_beam.transforms import sideinputs | |||
| from apache_beam.transforms.core import TimerSpec | |||
|
|
|||
| SplitResultType = Tuple[ | |||
There was a problem hiding this comment.
Feels like it's time to make a named tuple here...
There was a problem hiding this comment.
I agree it would help a bit. I searched around for places where we access individual elements of this tuple, and there aren't many cases. Here's the one I found:
def bundle_application(self,
op, # type: operations.DoOperation
primary # type: common.SplitResultType
):
((element_and_restriction, output_watermark),
_) = primary
return self.construct_bundle_application(
op, output_watermark, element_and_restriction)What name would you give to each item?
SplitResult = NamedTuple(
'SplitResult', [
('???', Tuple[WindowedValue, Optional[Timestamp]]),
('???', Optional[Timestamp])
])There was a problem hiding this comment.
Maybe WatermarkedElement and CallbackTimestamp? Talking to @boyuanzz this code is actively being developed, so it might make sense to hold off a bit. In particular, the splits are not entirely symmetric (e.g. a primary only ever has the WindowedValue, while the Residual is more properly a Tuple[WindowedValue, Optiona[Timestamp], Optional[Timestamp]].
| @@ -205,16 +205,17 @@ def process_encoded(self, encoded_windowed_values): | |||
| self.output(decoded_value) | |||
|
|
|||
| def try_split(self, fraction_of_remainder, total_buffer_size): | |||
| # type: (...) -> Optional[Tuple[int, Optional[Tuple[operations.DoOperation, common.SplitResultType]], Optional[Tuple[operations.DoOperation, common.SplitResultType]], int]] | |||
There was a problem hiding this comment.
Some named tuples, or even a named type, could help for Optional[Tuple[operations.DoOperation, common.SplitResultType]]
chadrik
force-pushed
the
python-typing-try-split
branch
from
6abebd9
to
892d2d2
Compare
|
Hey, is there any plan to merge this PR? |
|
I've been meaning to check in on this. @robertwb had some concerns:
@boyuanzz I'd love to get this merged and have you incorporate typing into your changes, but it may take a bit of effort on your part to properly annotate your changes, depending on how familiar you are with mypy and typing. |
|
OK, I'm going to merge this and let @boyuanzz build on top of it. |
|
Fantastic. That's what I was hoping for. Once some of the outstanding typing PRs get merged the noise in the mypy output should be greatly reduced, so that should make things easier. |
|
Just to let you know that we've just introduced Python autoformatter. Your merge conflict might be a result of this. |
chadrik
force-pushed
the
python-typing-try-split
branch
from
892d2d2
to
60e60b2
Compare
|
Hi Chad, do you happen to have a chance to resolve these file conflicts? Once these conflicts are resolved, I can help you to merge it. |
chadrik
force-pushed
the
python-typing-try-split
branch
from
60e60b2
to
a2451d4
Compare
|
done! |
|
Thank you, Chad! I'm going to merge it. |
This is a followup to #9056.
R: @robertwb
R: @udim
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.