[BEAM-8335] Make TestStream to/from runner_api include the output_tags property. by rohdesamuel · Pull Request #10892 · apache/beam

rohdesamuel · 2020-02-18T22:03:56Z

The TestStream has the "output_tags" property which keeps track of which events go to which PCollection. A TestStream going through a round-trip to/from proto won't have these fields set. To do this, a modification to the from_runner_api_parameter is needed to include the parent PTransform proto to retrieve the information from the outputs.

Thank you for your contribution! Follow this checklist to help us incorporate your contribution quickly and easily:

Choose reviewer(s) and mention them in a comment (R: @username).
Format the pull request title like [BEAM-XXX] Fixes bug in ApproximateQuantiles, where you replace BEAM-XXX with the appropriate JIRA issue, if applicable. This will automatically link the pull request to the issue.
Update CHANGES.md with noteworthy changes.
If this contribution is large, please file an Apache Individual Contributor License Agreement.

See the Contributor Guide for more tips on how to make review process smoother.

Post-Commit Tests Status (on master branch)

Lang	SDK	Apex	Dataflow	Gearpump	Samza	Spark
Go		---	---	---	---
Java
Python		---		---	---
XLang	---	---	---	---	---	---

Pre-Commit Tests Status (on master branch)

---	Java	Python	Go	Website
Non-portable
Portable	---		---	---

See .test-infra/jenkins/README for trigger phrase, status and link of all Jenkins jobs.

rohdesamuel · 2020-02-19T19:08:05Z

R: @davidyan74

sdks/python/apache_beam/testing/test_stream.py

davidyan74 · 2020-02-19T19:49:56Z

sdks/python/apache_beam/testing/test_stream.py

Is there a reason why the string 'None' is used as the tag here (instead of just None)? Is there a way to distinguish between a non-existent tag and a tag named "None"?

Looking through the codebase, it seems that 'None' is the special keyword used in the Python SDK to represent a tag that is specifically None. This keeps with the current style of the rest of the Python SDK. @lukecwik is this true?

This is a general problem with python tag naming where there is ambiguity. It was worked around for Dataflow by using out for None and out_<tag> for tag until it was removed in #10971

Unfortunately this name mangling was applied inconsistently throughout the codebase which lead to arbitrary fix-ups and bugs.

lukecwik

Looks pretty good, some questions about TestStream constructor contract.

sdks/python/apache_beam/testing/test_stream.py

sdks/python/apache_beam/testing/test_stream_test.py

lukecwik · 2020-02-19T19:41:08Z

sdks/python/apache_beam/testing/test_stream.py

Why do we need to declare output_tags here?
Are you trying to allow for outputs that have no events, otherwise shouldn't the tags come from the list of events?

The answer here impacts what we should be doing in expand in the no output_tags case in expand below.

Are you trying to allow for outputs that have no events, otherwise shouldn't the tags come from the list of events?

Yep! The TestStreamService will allow users to define a TestStream with the output_tags specified at creation time and the events supplied at runtime.

sdks/python/apache_beam/testing/test_stream.py

sdks/python/apache_beam/runners/direct/direct_runner.py

sdks/python/apache_beam/runners/direct/test_stream_impl.py

lukecwik · 2020-02-19T19:56:47Z

retest this please

rohdesamuel

Thanks for the quick review!

sdks/python/apache_beam/runners/direct/test_stream_impl.py

sdks/python/apache_beam/runners/direct/direct_runner.py

rohdesamuel · 2020-02-19T21:03:55Z

sdks/python/apache_beam/testing/test_stream.py

Looking through the codebase, it seems that 'None' is the special keyword used in the Python SDK to represent a tag that is specifically None. This keeps with the current style of the rest of the Python SDK. @lukecwik is this true?

rohdesamuel · 2020-02-19T21:08:21Z

sdks/python/apache_beam/testing/test_stream.py

Are you trying to allow for outputs that have no events, otherwise shouldn't the tags come from the list of events?

Yep! The TestStreamService will allow users to define a TestStream with the output_tags specified at creation time and the events supplied at runtime.

sdks/python/apache_beam/testing/test_stream.py

sdks/python/apache_beam/testing/test_stream_test.py

sdks/python/apache_beam/testing/test_stream.py

* This also moves the DirectRunner's TestStream implementation to a replacement transform. This is because the TestStream relies on getting the output_tags from the PTransform. Change-Id: Ibd80b0d25cd8cc5ff5c28e127f7313638e6664da

rohdesamuel · 2020-02-24T22:41:09Z

retest this please

EDIT: dang, doesn't work yet

lukecwik · 2020-02-26T04:22:12Z

retest this please

lukecwik · 2020-02-26T18:44:02Z

sdks/python/apache_beam/runners/direct/test_stream_impl.py

+
+  def expand(self, pbegin):
+    """Expands the TestStream into the DirectRunner implementation.
+


Suggested change

lukecwik · 2020-02-26T18:49:42Z

sdks/python/apache_beam/testing/test_stream.py

This is a general problem with python tag naming where there is ambiguity. It was worked around for Dataflow by using out for None and out_<tag> for tag until it was removed in #10971

Unfortunately this name mangling was applied inconsistently throughout the codebase which lead to arbitrary fix-ups and bugs.

lukecwik · 2020-02-26T19:00:35Z

sdks/python/apache_beam/testing/test_stream.py

  def to_runner_api(self, unused_element_coder):
+    tag = 'None' if self.tag is None else self.tag
+
+    # Assert that no prevision is lost.


Suggested change

# Assert that no prevision is lost.

# Assert that no precision is lost.

lukecwik · 2020-02-26T19:01:48Z

sdks/python/apache_beam/testing/test_stream.py

+
+    # Assert that no prevision is lost.
+    assert 1000 * (
+        self.new_watermark.micros // 1000) == self.new_watermark.micros


nit: This would be clearer by checking % has no remainder

sdks/python/apache_beam/testing/test_stream.py

lukecwik · 2020-02-26T19:08:13Z

sdks/python/apache_beam/testing/test_stream.py

  output.
  """
-  def __init__(self, coder=coders.FastPrimitivesCoder(), events=None):
+  def __init__(


Please add pydoc comments mentioning the few important pieces:

specifying the output_tags allows for adding outputs that produce no events

output_tags must be a superset of tags found in events if events is specified

lukecwik · 2020-02-26T19:11:00Z

sdks/python/apache_beam/testing/test_stream.py

@@ -171,13 +184,20 @@ class TestStream(PTransform):
  time. After all of the specified elements are emitted, ceases to produce


State that if only the default output or only one output tag has been specified, then a PCollection will be returned otherwise a dictionary of output name to PCollection.

probot-autolabeler bot added io python labels Feb 18, 2020

rohdesamuel force-pushed the teststream_merge branch 7 times, most recently from 677e777 to c5fb23e Compare February 19, 2020 18:35

rohdesamuel marked this pull request as ready for review February 19, 2020 19:01

rohdesamuel force-pushed the teststream_merge branch from c5fb23e to adbc861 Compare February 19, 2020 19:06

rohdesamuel requested a review from lukecwik February 19, 2020 19:08

davidyan74 reviewed Feb 19, 2020

View reviewed changes

lukecwik reviewed Feb 19, 2020

View reviewed changes

rohdesamuel commented Feb 19, 2020

View reviewed changes

rohdesamuel force-pushed the teststream_merge branch from adbc861 to 4dbedcb Compare February 19, 2020 21:42

Move TestStream implementation to replacement transform

437c88f

* This also moves the DirectRunner's TestStream implementation to a replacement transform. This is because the TestStream relies on getting the output_tags from the PTransform. Change-Id: Ibd80b0d25cd8cc5ff5c28e127f7313638e6664da

rohdesamuel force-pushed the teststream_merge branch from 4dbedcb to 437c88f Compare February 24, 2020 22:01

lukecwik approved these changes Feb 26, 2020

View reviewed changes

lukecwik merged commit 7a4cdec into apache:master Feb 26, 2020

boyuanzz mentioned this pull request Feb 27, 2020

[BEAM-9381] Adding display data to BoundedSource SDF #10968

Merged

4 tasks


		def expand(self, pbegin):
		"""Expands the TestStream into the DirectRunner implementation.

	# Assert that no prevision is lost.
	# Assert that no precision is lost.

		@@ -171,13 +184,20 @@ class TestStream(PTransform):
		time. After all of the specified elements are emitted, ceases to produce

Conversation

rohdesamuel commented Feb 18, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Post-Commit Tests Status (on master branch)

Pre-Commit Tests Status (on master branch)

Uh oh!

rohdesamuel commented Feb 19, 2020

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

lukecwik Feb 26, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

lukecwik left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

lukecwik commented Feb 19, 2020

Uh oh!

rohdesamuel left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

rohdesamuel commented Feb 24, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

lukecwik commented Feb 26, 2020

Uh oh!

Choose a reason for hiding this comment

Uh oh!

lukecwik Feb 26, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

rohdesamuel commented Feb 18, 2020 •

edited

Loading

lukecwik Feb 26, 2020 •

edited

Loading

rohdesamuel commented Feb 24, 2020 •

edited

Loading

lukecwik Feb 26, 2020 •

edited

Loading