Skip to content

[BEAM-115] Represent a Pipeline via a list of Top-level Transforms#2505

Closed
tgroh wants to merge 2 commits intoapache:masterfrom
tgroh:top_level_transforms
Closed

[BEAM-115] Represent a Pipeline via a list of Top-level Transforms#2505
tgroh wants to merge 2 commits intoapache:masterfrom
tgroh:top_level_transforms

Conversation

@tgroh
Copy link
Member

@tgroh tgroh commented Apr 11, 2017

Be sure to do all of the following to help us incorporate your contribution
quickly and easily:

  • Make sure the PR title is formatted like:
    [BEAM-<Jira issue #>] Description of pull request
  • Make sure tests pass via mvn clean verify. (Even better, enable
    Travis-CI on your fork and ensure the whole test matrix passes).
  • Replace <Jira issue #> in the title with the actual Jira issue
    number, if there is one.
  • If this contribution is large, please file an Apache
    Individual Contributor License Agreement.

The root node is a synthetic transform which does not appear within the
graph, as it never has any components of note. Instead of referring to a
single "root node" in the Pipeline message, refer to the top-level nodes
which do not have an enclosing PTransform.

The root node is a synthetic transform which does not appear within the
graph, as it never has any components of note. Instead of referring to a
single "root node" in the Pipeline message, refer to the top-level nodes
which do not have an enclosing PTransform.
@tgroh
Copy link
Member Author

tgroh commented Apr 11, 2017

R: @kennknowles

CC: @lukecwik @dhalperi

@kennknowles
Copy link
Member

Seems reasonable to me. Is there any particular roadblock that prompted it? (I may have missed some dev list discussion or PR commentary)

@tgroh
Copy link
Member Author

tgroh commented Apr 12, 2017

Converting a Pipeline is close, but the Components (as I've built them) go through the AppliedPTransform class to register PTransforms, and there's no way to construct one for the root node (at least in Java)

@coveralls
Copy link

Coverage Status

Coverage increased (+0.003%) to 70.182% when pulling 31d5c27 on tgroh:top_level_transforms into ccf247c on apache:master.

@asfbot
Copy link

asfbot commented Apr 12, 2017

Refer to this link for build results (access rights to CI server needed):
https://builds.apache.org/job/beam_PreCommit_Java_MavenInstall/9429/
--none--

Copy link
Member

@lukecwik lukecwik left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Minor comment and LGTM

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

root_transform_ids

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done.

@coveralls
Copy link

Coverage Status

Coverage increased (+0.4%) to 70.572% when pulling cf6946f on tgroh:top_level_transforms into ccf247c on apache:master.

@lukecwik
Copy link
Member

LGTM

@asfbot
Copy link

asfbot commented Apr 12, 2017

Refer to this link for build results (access rights to CI server needed):
https://builds.apache.org/job/beam_PreCommit_Java_MavenInstall/9458/

Build result: ABORTED

[...truncated 2.72 MB...]2017-04-12T19:23:12.674 [INFO] Apache Beam :: SDKs :: Java :: Java 8 Tests ........ SUCCESS [ 17.935 s]2017-04-12T19:23:12.674 [INFO] Apache Beam :: SDKs :: Python ...................... SUCCESS [12:25 min]2017-04-12T19:23:12.674 [INFO] Apache Beam :: Runners :: Flink .................... SUCCESS [ 31.356 s]2017-04-12T19:23:12.674 [INFO] Apache Beam :: Runners :: Flink :: Core ............ SUCCESS [01:47 min]2017-04-12T19:23:12.674 [INFO] Apache Beam :: Runners :: Flink :: Examples ........ SUCCESS [01:28 min]2017-04-12T19:23:12.674 [INFO] Apache Beam :: Runners :: Apex ..................... SUCCESS [01:55 min]2017-04-12T19:23:12.674 [INFO] Apache Beam :: Examples ............................ SUCCESS [ 23.082 s]2017-04-12T19:23:12.674 [INFO] Apache Beam :: Examples :: Java .................... SUCCESS [16:52 min]2017-04-12T19:23:12.674 [INFO] Apache Beam :: Examples :: Java 8 .................. SUCCESS [ 47.890 s]2017-04-12T19:23:12.674 [INFO] Apache Beam :: SDKs :: Java :: Aggregated Javadoc .. SUCCESS [01:27 min]2017-04-12T19:23:12.674 [INFO] ------------------------------------------------------------------------2017-04-12T19:23:12.674 [INFO] BUILD SUCCESS2017-04-12T19:23:12.674 [INFO] ------------------------------------------------------------------------2017-04-12T19:23:12.675 [INFO] Total time: 01:24 h2017-04-12T19:23:12.675 [INFO] Finished at: 2017-04-12T19:23:12+00:002017-04-12T19:23:14.018 [INFO] Final Memory: 284M/2101M2017-04-12T19:23:14.019 [INFO] ------------------------------------------------------------------------Waiting for Jenkins to finish collecting dataBuild timed out (after 100 minutes). Marking the build as aborted.AbortedBuild was abortedchannel stoppedSetting status of cf6946f to FAILURE with url https://builds.apache.org/job/beam_PreCommit_Java_MavenInstall/9458/ and message: 'Build finished. 'Using context: Jenkins: Maven clean install
--none--

@tgroh
Copy link
Member Author

tgroh commented Apr 12, 2017

retest this please

@coveralls
Copy link

Coverage Status

Coverage increased (+0.4%) to 70.573% when pulling cf6946f on tgroh:top_level_transforms into ccf247c on apache:master.

@asfbot
Copy link

asfbot commented Apr 12, 2017

@tgroh
Copy link
Member Author

tgroh commented Apr 12, 2017

retest this please

@coveralls
Copy link

Coverage Status

Coverage increased (+0.4%) to 70.576% when pulling cf6946f on tgroh:top_level_transforms into ccf247c on apache:master.

@asfbot
Copy link

asfbot commented Apr 12, 2017

Refer to this link for build results (access rights to CI server needed):
https://builds.apache.org/job/beam_PreCommit_Java_MavenInstall/9479/
--none--

@asfgit asfgit closed this in 8f1b4e4 Apr 13, 2017
@robertwb
Copy link
Contributor

I actually prefer the pipeline having a single root note. Semantically (especially now that we're removing pipeline options) it's cleaner if Pipeline == PTransform<PBegin, ?> (and eventually we may even want to reify this in the API, i.e. have Runner.run(PTransform<PBegin, ?> and move away from the Pipeline object altogether (which is an odd ducke, e.g. it's not a PValue but has an apply)). This would reduce the manual (and error-prone) scope of pairing each pipeline construction with a run(). This is a ways off, but could be reflected in the proto representation right away.)

As for being "empty" it would contain parts and the pipeline-level display_data. If we decide to attach other information to PTransforms (such as resource requirements) that would naturally live there as well.

robertwb added a commit to robertwb/incubator-beam that referenced this pull request Apr 21, 2017
robertwb added a commit to robertwb/incubator-beam that referenced this pull request Apr 21, 2017
robertwb added a commit to robertwb/incubator-beam that referenced this pull request Apr 24, 2017
asfgit pushed a commit that referenced this pull request Apr 24, 2017
peihe pushed a commit to peihe/incubator-beam that referenced this pull request Jul 21, 2017
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

6 participants

Comments