[BEAM-9488] Ensure we pass through PCollection ids instead of attempting to fix them up. by lukecwik · Pull Request #11514 · apache/beam

lukecwik · 2020-04-23T23:26:51Z

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
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.

…ing to fix them up.

lukecwik · 2020-04-23T23:29:21Z

R: @HuangLED @robertwb

lukecwik · 2020-04-23T23:36:50Z

sdks/python/apache_beam/runners/worker/bundle_processor.py

-      inject_pcollection(mi)
-
-    return infos_list
+      pcollection_ids = self.process_bundle_descriptor.transforms[


I believe the id order aligns with the receiver order since transform_consumers built above iterates the outputs map in the same order and this gets plumbed down through to Operation.

In practice this might be OK (dicts have undefined, but I think when modified deterministic, iteration order), but seems rather brittle to me. Could we instead passed the tag -> pcollection_id mapping here?

lukecwik · 2020-04-24T00:17:45Z

Run Portable_Python PreCommit

robertwb

I suppose this would be to finish the transition from reporting counters on PTransform outputs to recording them on the various PCollections.

LGTM if you can change to use a mapping of tags to pcoll ids rather than relying on ordering being the same.

robertwb · 2020-04-24T00:06:37Z

sdks/python/apache_beam/runners/worker/operations.py

+          (self.receivers, pcollection_ids))
+
+    all_monitoring_infos = {}
+    for i in range(len(self.receivers)):


This will change if you use a mapping, but zip would be the idiom to use here.

+1 using zip

robertwb · 2020-04-24T00:12:37Z

sdks/python/apache_beam/runners/worker/bundle_processor.py

-      inject_pcollection(mi)
-
-    return infos_list
+      pcollection_ids = self.process_bundle_descriptor.transforms[


In practice this might be OK (dicts have undefined, but I think when modified deterministic, iteration order), but seems rather brittle to me. Could we instead passed the tag -> pcollection_id mapping here?

robertwb · 2020-04-24T00:18:38Z

sdks/python/apache_beam/runners/worker/bundle_processor.py

-        tag_label = monitoring_info.labels[monitoring_infos.TAG_LABEL]
-
-        if not ptransform_label in self.process_bundle_descriptor.transforms:
-          return


This would be a bug, right?

robertwb · 2020-04-24T00:19:33Z

sdks/python/apache_beam/runners/worker/bundle_processor.py

-        if not ptransform_label in self.process_bundle_descriptor.transforms:
-          return
-        if not tag_label in self.process_bundle_descriptor.transforms[
-            ptransform_label].outputs:


Likewise this.

Actually, this can happen, and might be what's happening here. There is no PCollection for this tag, but the user outputted a value to this tag. It would make sense to record this output even if we didn't use it. This is another downside of attaching these counters to PCollections themselves rather than to PTransform outputs.

unknown outputs should probably be reported another way

lukecwik · 2020-04-24T03:33:21Z

I suppose this would be to finish the transition from reporting counters on PTransform outputs to recording them on the various PCollections.

LGTM if you can change to use a mapping of tags to pcoll ids rather than relying on ordering being the same.

I'll try it out but I worry that the consumers/receivers are either using indices or the post string converted tag names since python does some post processing converting the string tags to non string tags.

lukecwik · 2020-04-24T20:38:05Z

I looked through the implementation and it seems as though adding the pcollection id to the ConsumerSet doesn't work out since operations don't have that level of visibility in pipeline proto and consumers works off of a index -> receiver map and expects tags to get mapped to indices so we would need to go through all three layers. I suggest that we stick with this brittle approach until we can delete the non-portable Python worker implementation which would make a lot of the layers simpler.

HuangLED

LGTM. Thanks for the fix.

robertwb

Thanks, LGTM. Can't wait 'till we can get rid of the legacy worker and clean this up!

lukecwik · 2020-04-25T00:06:07Z

Run Python PreCommit

pabloem · 2020-04-28T01:39:51Z

I am suspecting that this made precommits flaky

pabloem · 2020-04-28T01:40:16Z

see: https://issues.apache.org/jira/browse/BEAM-9832?page=com.atlassian.jira.plugin.system.issuetabpanels%3Aall-tabpanel

…ing to fix them up. (apache#11514) * [BEAM-9488] Ensure we pass through PCollection ids instead of attempting to fix them up. * fixup! Convert to list since values() isn't subscriptable * fixup! Use zip * fixup! Migrate to use tag -> pcollection id * fixup! lint * fixup! Fix comparison

probot-autolabeler bot added the python label Apr 23, 2020

[BEAM-9488] Ensure we pass through PCollection ids instead of attempt…

96cc611

…ing to fix them up.

lukecwik force-pushed the metrics branch from 79cd39d to 96cc611 Compare April 23, 2020 23:29

lukecwik commented Apr 23, 2020

View reviewed changes

robertwb approved these changes Apr 24, 2020

View reviewed changes

fixup! Convert to list since values() isn't subscriptable

808b4b9

HuangLED approved these changes Apr 24, 2020

View reviewed changes

lukecwik added 4 commits April 24, 2020 14:01

fixup! Use zip

04c6334

fixup! Migrate to use tag -> pcollection id

051c094

fixup! lint

4c03658

fixup! Fix comparison

1297a0b

robertwb approved these changes Apr 24, 2020

View reviewed changes

lukecwik merged commit 4a52fb4 into apache:master Apr 26, 2020

ibzib mentioned this pull request May 12, 2020

[BEAM-9488] [release-2.21.0] Ensure we pass through PCollection ids instead of attempt… #11687

Merged

4 tasks

Conversation

lukecwik commented Apr 23, 2020

Post-Commit Tests Status (on master branch)

Pre-Commit Tests Status (on master branch)

Uh oh!

lukecwik commented Apr 23, 2020

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

lukecwik commented Apr 24, 2020

Uh oh!

robertwb left a comment

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!

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!

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!

lukecwik commented Apr 24, 2020

Uh oh!

lukecwik commented Apr 24, 2020

Uh oh!

HuangLED left a comment

Choose a reason for hiding this comment

Uh oh!

robertwb left a comment

Choose a reason for hiding this comment

Uh oh!

lukecwik commented Apr 25, 2020

Uh oh!

pabloem commented Apr 28, 2020

Uh oh!

pabloem commented Apr 28, 2020

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants