-
Notifications
You must be signed in to change notification settings - Fork 1.7k
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
Conversation
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. |
rsync -arvp "../../predict"/ ./build/ | ||
cp ../../../license.sh ./build | ||
cp ../../../third_party_licenses.csv ./build | ||
rsync -arvp "../tfdv/src"/ ./build/ |
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
9ba0887
to
263bc39
Compare
CLAs look good, thanks! |
/lgtm |
Are any sample test using these components? |
Yes, all these components are tested by the sample tests. |
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/ |
There was a problem hiding this comment.
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.
/lgtm |
/approve |
[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 |
[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 |
/test presubmit-e2e-test |
/lgtm |
…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
* Add best practice for tekton and other argo executors * Update README.md * Apply suggestions from code review Co-authored-by: Animesh Singh <[email protected]> Co-authored-by: Animesh Singh <[email protected]>
This change is