Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

restructure dataflow component structure #338

Merged
merged 3 commits into from
Nov 21, 2018

Conversation

gaoning777
Copy link
Contributor

@gaoning777 gaoning777 commented Nov 20, 2018

This change is Reviewable

@googlebot
Copy link
Collaborator

We found a Contributor License Agreement for you (the sender of this pull request), but were unable to find agreements for all the commit author(s) or Co-authors. If you authored these, maybe you used a different email address in the git commits than was used to sign the CLA (login here to double check)? If these were authored by someone else, then they will need to sign a CLA as well, and confirm that they're okay with these being contributed to Google.
In order to pass this check, please resolve this problem and have the pull request author add another comment and the bot will run again. If the bot doesn't comment, it means it doesn't think anything has changed.

rsync -arvp "../../predict"/ ./build/
cp ../../../license.sh ./build
cp ../../../third_party_licenses.csv ./build
rsync -arvp "../tfdv/src"/ ./build/
Copy link
Contributor

Choose a reason for hiding this comment

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

In future we should probably try to separate the component images. For now it's fine.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

are you talking about separating the component directory from the pipeline repo?

Copy link
Contributor

Choose a reason for hiding this comment

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

No. I'm talking about having one image per dataflow component. Right now they're using a single image. But this is low-priority.

Copy link
Contributor

Choose a reason for hiding this comment

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

We do have one image per dataflow component. This is the base image that captures the dependencies. The source code is built into leaf images.

@googlebot
Copy link
Collaborator

CLAs look good, thanks!

@Ark-kun
Copy link
Contributor

Ark-kun commented Nov 20, 2018

/lgtm

@Ark-kun
Copy link
Contributor

Ark-kun commented Nov 20, 2018

Are any sample test using these components?

@gaoning777
Copy link
Contributor Author

Yes, all these components are tested by the sample tests.

@gaoning777 gaoning777 changed the title [WIP] restructure dataflow component structure restructure dataflow component structure Nov 21, 2018
@gaoning777
Copy link
Contributor Author

Tested on local cloudbuild; it works fine.

rsync -arvp "../../predict"/ ./build/
cp ../../../license.sh ./build
cp ../../../third_party_licenses.csv ./build
rsync -arvp "../tfdv/src"/ ./build/
Copy link
Contributor

Choose a reason for hiding this comment

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

We do have one image per dataflow component. This is the base image that captures the dependencies. The source code is built into leaf images.

@qimingj
Copy link
Contributor

qimingj commented Nov 21, 2018

/lgtm

@qimingj
Copy link
Contributor

qimingj commented Nov 21, 2018

/approve

@k8s-ci-robot
Copy link
Contributor

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: qimingj

The full list of commands accepted by this bot can be found here.

The pull request process is described here

Needs approval from an approver in each of these files:

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

@k8s-ci-robot
Copy link
Contributor

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: qimingj

The full list of commands accepted by this bot can be found here.

The pull request process is described here

Needs approval from an approver in each of these files:

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

@Ark-kun
Copy link
Contributor

Ark-kun commented Nov 21, 2018

/test presubmit-e2e-test

@qimingj
Copy link
Contributor

qimingj commented Nov 21, 2018

/lgtm

@k8s-ci-robot k8s-ci-robot merged commit da6ff9c into master Nov 21, 2018
@gaoning777 gaoning777 deleted the ngao/restructure-dataflow branch November 21, 2018 19:12
Linchin pushed a commit to Linchin/pipelines that referenced this pull request Apr 11, 2023
…ubeflow#338)

* Add a playbook and describe how to deal with the CI infrastructure running
  out of GCP quota.

* The cron/batch job for the CI system should not be pinned to checkout
  the code at PR 300; we should be using master.

* We are seeing socket errors contacting the DM service so add some retries
  and in the event of permanent failure try to keep going.

Related to: kubeflow#337
HumairAK pushed a commit to red-hat-data-services/data-science-pipelines that referenced this pull request Mar 11, 2024
* Add best practice for tekton and other argo executors

* Update README.md

* Apply suggestions from code review

Co-authored-by: Animesh Singh <singhan@us.ibm.com>

Co-authored-by: Animesh Singh <singhan@us.ibm.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants