[BEAM-6067] In Python SDK, specify pipeline_proto_coder_id property in non-Beam-standard CloudObject coders #7081

CraigChambersG · 2018-11-19T21:13:02Z

Post-Commit Tests Status (on master branch)

Lang	Apex	Dataflow	Flink	Gearpump	Samza	Spark
Go	---	---	---	---	---	---
Java
Python	---			---	---	---

lukecwik · 2018-11-19T21:19:36Z

Run Python PostCommit

robertwb

This change looks fine to me.

sdks/python/apache_beam/coders/coders.py

angoenka · 2018-11-20T19:47:08Z

@CraigChambersG Can you please rebase the PR so that we can run the test on it?

CraigChambersG · 2018-11-20T21:25:23Z

@angoenka How do I do that? Can you give me specific git command(s) to run? Thanks.

robertwb · 2018-11-20T21:51:03Z

Specific command you can run are

git fetch upstream          # Assuming you followed https://cwiki.apache.org/confluence/display/BEAM/Git+Tips
git rebase upstream/master  # at any time you can git rebase --abort
git push -f

…n non-Beam-standard CloudObject coders

CraigChambersG · 2018-11-20T23:34:05Z

Run Python PostCommit

CraigChambersG · 2018-11-21T01:42:05Z

how can i figure out why the postcommit test failed? i think it's a build failure of something, but i don't know what.

kennknowles · 2018-11-21T03:04:25Z

The most informative UI for that is to click "details" in the row that failed and then click "Gradle Build Scan". Here it is: https://scans.gradle.com/s/fgs32qpduwaug. It appears that the Python postcommit at https://scans.gradle.com/s/fgs32qpduwaug/console-log?task=:beam-sdks-python:postCommitIT runs in a way that is not reported/parsed as a collection of test methods, so you just have to scrape the logs.

kennknowles · 2018-11-21T03:04:50Z

test_bigquery_tornadoes_it (apache_beam.examples.cookbook.bigquery_tornadoes_it_test.BigqueryTornadoesIT) ... FAIL

Expected checksum is 83789a7c1bca7959dcf23d3bc37e9204e594330f Actual checksum is d860e636050c559a16a791aff40d6ad809d4daf0

robertwb · 2018-11-21T08:57:05Z

Python SDK PostCommit Tests

robertwb · 2018-11-21T08:57:27Z

Run Python PostCommit

CraigChambersG · 2018-11-21T17:41:39Z

Thanks. It's hard to see how my change would affect just that one BigQuery integration test. Maybe it's flaky, or sick for some other reason? But Robert looks to have rerun the python postcommit tests, and got a failure, so maybe there's something real here. But I'll try running the tests again.

CraigChambersG · 2018-11-21T17:41:49Z

Run Python PostCommit

…ther the FnAPI is being used, to avoid changing earlier behaviors

CraigChambersG · 2018-11-22T16:55:33Z

Run Python PostCommit

CraigChambersG · 2018-11-22T16:59:24Z

sdks/python/apache_beam/runners/dataflow/dataflow_runner.py

      # propagated everywhere. Returning an 'Any' as type hint will trigger
      # usage of the fallback coder (i.e., cPickler).
      element_type = typehints.Any
+      use_fnapi = False  # TODO(chambers): XXX do the right thing for this


This is unfortunate. Is there something better I can/should do here?

In general, passing around the use_fnapi flag is yucky. I'd much rather have the pipeline or the pipeline options be available in an instance variable. Is there a way to do that? Or a reason not to do that?

If we make options into an instance variable than that cuts off the option for runners to run multiple pipelines with different options. Unconditionally setting it to false here seems the wrong thing to do though; do we have any idea why this is needed (other than that the test fails otherwise?)

This here is just a placeholder. I don't know how to get access to the pipeline options otherwise. If you tell me how, I'll fix it.

I'd like to understand why setting the pipeline_proto_coder_id attribute unconditionally breaks things. If that's not workable, I'd rather name this something other than use_fnapi if we don't need the coder id in this case.

One post-commit test failed, on a checksum comparison. I don't have any deeper understanding of what the test is doing or why there was a failure. I have had other experiences where tests were (brittlely) checking for equivalence against some expected representation which can be adversely affected by adding an otherwise unused property to CloudObjects.

To be clear, we do need the coder id in this case, at least when we support multi-output DoFns over the fnapi using the worker code that reads this property. We're not running such tests now. I need advice on how to get a hold of the pipeline object in this branch in order to put in the proper code. Also, the TODO in this branch suggests that the branch may be going away, so maybe it doesn't need to be fixed.

Ping? What should I do to make progress on this PR? I'm OK submitting something that (a) doesn't break anything that already exists, and (b) makes some incremental progress on runners using Beam portability. I'm also happy to improve this CL, if given guidance on what to do.

I'd like to understand why this particular test failed, and not others, which may be indicative of other problems, rather than adding a bunch of code to work around it, but at this point we probably shouldn't be blocking on that.

Let's just rename this something like emit_coder_ids and get it in.

(I also hope this code in this whole file live on much longer.)

I could rename this local variable, but that would be masking the intent. The intent of the local variable is indeed whether the CloudObject is being generated for a backend using the FnAPI. This particular line is "I don't know how to figure out if we're using the FnAPI, so for now just assume we're not, to preserve behavior for non-experimental backends; TODO: figure out how to tell". The comment is intending to capture that. The rest of the code in this function is acting as intended.

The one test that failed, which motivated adding all this use_fnapi stuff, doesn't take this branch, so its failure is unrelated to this line.

(Out of curiosity, what is your wish for how this code becomes obsolete?)

I read this as "if there are multiple outputs, don't use the fn api" but I understand your intent now.

Just thinking about this again, another option would be to simply choose any output on which to check for the fn api flag. This shouldn't(?) be called if there are no outputs.

As for the test failure, it feels like we're working around a still-present bug. But as you say, it may just be brittle testing.

As for how this code becomes obsolete, the Dataflow service should just accept FnApi protos directly, rather than have each SDK translate it to cloud objects just to try to get them translated to the right DFE objects on the other end.

I've updated this code to just pick the first output to get the pipeline options from. PTAL.

CraigChambersG · 2018-11-22T17:36:34Z

Run Python PostCommit

CraigChambersG · 2018-11-22T21:28:54Z

ok, it looks like the extra conditionalizing worked to get the one failing IT test to now pass. what's the process for reviewing and merging from here?

CraigChambersG · 2018-12-03T14:53:30Z

Run Python PostCommit

CraigChambersG · 2018-12-03T15:48:27Z

Run Python PostCommit

CraigChambersG · 2018-12-04T18:23:38Z

Run Python PostCommit

robertwb · 2018-12-05T11:27:30Z

This looks OK to me. (As an aside, I wonder why the transform nodes themselves don't have a reference to the pipeline...) I resolved the merge conflict, and will merge assuming all tests passing.

CraigChambersG · 2018-12-06T17:00:02Z

Is there something I need to do at this point to get this PR checked in?

robertwb · 2018-12-06T17:08:36Z

Tests look good. Done.

robertwb approved these changes Nov 20, 2018

View reviewed changes

sdks/python/apache_beam/coders/coders.py Outdated Show resolved Hide resolved

sdks/python/apache_beam/coders/coders.py Show resolved Hide resolved

[BEAM-6067] In Python SDK, specify pipeline_proto_coder_id property i…

9db854d

…n non-Beam-standard CloudObject coders

CraigChambersG force-pushed the master branch from e873c47 to 9db854d Compare November 20, 2018 23:17

use list comprehension

f93742d

Conditionalize adding the new pipeline_proto_coder_id property on whe…

385af1b

…ther the FnAPI is being used, to avoid changing earlier behaviors

CraigChambersG commented Nov 22, 2018

View reviewed changes

fix lint issues

633a107

CraigChambersG added 2 commits December 3, 2018 06:34

Merge remote-tracking branch 'upstream/master'

b5c3a55

clean up use_fnapi for multi-output transforms

3d4ebdd

fix indentation

564acac

Merge branch 'master' into master

43a7474

robertwb merged commit 0edc85e into apache:master Dec 6, 2018

[BEAM-6067] In Python SDK, specify pipeline_proto_coder_id property in non-Beam-standard CloudObject coders #7081

[BEAM-6067] In Python SDK, specify pipeline_proto_coder_id property in non-Beam-standard CloudObject coders #7081

Uh oh!

Conversation

CraigChambersG commented Nov 19, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Post-Commit Tests Status (on master branch)

Uh oh!

lukecwik commented Nov 19, 2018

Uh oh!

robertwb left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

angoenka commented Nov 20, 2018

Uh oh!

CraigChambersG commented Nov 20, 2018

Uh oh!

robertwb commented Nov 20, 2018

Uh oh!

CraigChambersG commented Nov 20, 2018

Uh oh!

CraigChambersG commented Nov 21, 2018

Uh oh!

kennknowles commented Nov 21, 2018

Uh oh!

kennknowles commented Nov 21, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

robertwb commented Nov 21, 2018

Uh oh!

robertwb commented Nov 21, 2018

Uh oh!

CraigChambersG commented Nov 21, 2018

Uh oh!

CraigChambersG commented Nov 21, 2018

Uh oh!

CraigChambersG commented Nov 22, 2018

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!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

CraigChambersG commented Nov 22, 2018

Uh oh!

CraigChambersG commented Nov 22, 2018

Uh oh!

CraigChambersG commented Dec 3, 2018

Uh oh!

CraigChambersG commented Dec 3, 2018

Uh oh!

CraigChambersG commented Dec 4, 2018

Uh oh!

robertwb commented Dec 5, 2018

Uh oh!

CraigChambersG commented Dec 6, 2018

Uh oh!

robertwb commented Dec 6, 2018

Uh oh!

Reviewers

Assignees

Labels

Projects

CraigChambersG commented Nov 19, 2018 •

edited

Loading

kennknowles commented Nov 21, 2018 •

edited

Loading